[ofa-general] Re: [PATCH] opensm & osm_console: modified console framework to support multiple connections

[Sasha Khapyorsky]

On 10:48 Mon 29 Oct     , Timothy A. Meier wrote:
> 
>  I apologize for the style and submission issues - still adjusting...

No need to apologize :)

>  I was troubled with breaking this into pieces. The patch is really about
>  providing an abstract OSM Server that supports local/remote connections.
> 
>  I can break them up, but in my mind, they were tightly coupled.

I think it could be broken at least to multiconnection support and the
rest abstractions. No need to split it now only for "split", just try
to make it in smaller patches in the next version of this.

> >>  +/* TODO move along with other IO abstraction code */
> >>  +int cio_printf( CIO_t *cio, const char *format, ...);
> >>  +int cio_flush( CIO_t *cio);
> >>  +int cio_getline( char **lineptr, size_t *n, CIO_t *cio);
> >>  +int cio_open( CIO_t *cio);
> >>  +int cio_close( CIO_t *cio);
> >>  +int cio_poll(CIO_t *cio, int timeout);
> >>     
> >
> > Later I see that all cio_* and CIO_* stuff is used only in
> > osm_console.c, then I think this all should be moved to this file,
> > local function should be static, etc..
> >
> >   
>  The intent of the CIO abstraction is to support connections to the OSM
>  server. Currently, the only thing "planned" to use this connection is
>  the interactive Console. That might not always be the case.

Now it is the case. And if there are no concrete plans to use this APIs
externally I prefer to keep it local.

> >>  +typedef struct _osm_console_thread_t
> >>  +{
> >>  +  int used;
> >>  +  unsigned short int port;
> >>  +  int authorized;
> >>  +  int state;
> >>  +  char name[CIO_INFO_SIZE];
> >>  +  char in_buff[CIO_BUFSIZE];
> >>  +  char out_buff[CIO_BUFSIZE];
> >>  +  char client_type[CIO_NOTE_SIZE];  // maps to option->console  
> >> (off|local|socket)
> >>  +  char client_ip[CIO_NOTE_SIZE];
> >>  +  char client_hn[CIO_INFO_SIZE];
> >>  +  unsigned int thread_num;  // a unique ever increasing number +   
> >> osm_opensm_t *p_osm;     // the global opensm singleton (protect with 
> >> lock)
> >>  +  CIO_t io;                // the io streams for the connection
> >>  +  LoopCmd loop_command;
> >>  +  cl_thread_t  consoleThread; // a specific thread each console 
> >> connection
> >>  +  struct timeval connect_time;
> >>  +} osm_console_thread_t;
> >>     
> >
> > I think this introduces CIO_MAX_CONNECTS new threads + for loop commands.
> > What about to do all in one thread - to use select() or poll() with
> > timeout on multiple file descriptors? This will "reserve" another CPUs
> > for running another OpenSM things. Another potential problem is multi
> > thread synchronizations - we had (and still have) a lot of issues in this
> > area.
> >
> >   
>  I wasn't aware of thread synchronization issues....
> 
>  You are correct, this potentially introduces 2*CIO_MAX_CONNECTS new threads.
>  (Worst case, all connections are used, all running a loop command.)
> 
>  Currently, the only loop command is for printing status, but the software
>  was designed to support any command you may want to put in a
>  loop. If no additional commands will be "looped", then I agree its overkill
>  to put this in its own thread.
> 
>  I think each connection/session should be in its own thread.

Wouldn't poll() on multiple file descriptors (connected and listened
sockets) be simpler and more robust approach here? Why?

>  Currently those wrapper functions only provide a single implementation, but
>  I intend to extend them with additional functionality when I add SSL/TSL.

This is why I thought it would be clearer to see in a patch series..

>  The new protocol will depend on new libraries/headers. We (LLNL)
>  discussed this, and thought conditionally compiling this feature in would
>  satisfy those folks who did not want to add this dependency if they did
>  not want the feature.

That should be fine.

>  Thanks for reviewing all of this. How would you like me to move forward?
>  Would you rather me (re)submit this Patch as a series of 2?

I think we need to close threading issue first. Then patch series of 2
looks fine for me.

> I want to
>  establish this as a working baseline (no new functionality, just more
>  extensible) before adding the SSL/TSL code.

Understood. Thanks for doing this!

Sasha