Re: [PATCH] Relay CPU hotplug support

Hi Mathieu,

Mathieu Desnoyers writes:
 > Hi Tom,
 > 
 > * Tom Zanussi ([email protected]) wrote:
 > >
 > > I hadn't thought of clients using the same channel type in global or
 > > non-global modes at different times - I'd assumed they'd just use a
 > > different set of callbacks in either case.  But it would be nice to
 > > allow for that flexibility, and for the other reasons you mentioned
 > > for wanting access to the private_data, it probably does make sense to
 > > add the private_data param to relay_open(); in any case it's a simple
 > > change to the API that shouldn't cause any confusion.
 > >
 > > As for adding an is_global flag, when I added support for global
 > > buffers, I did consider adding an is_global flag to relay_open(), but
 > > decided against it in favor of avoiding changes to the main API and
 > > keeping it as simple as possible for the most common case, which is
 > > non-global buffers (that was also when the default create_buf_file()
 > > callback automatically created relayfs files i.e. before the debugfs
 > > changes, so in the common case, the client wouldn't have to look at
 > > create_buf_file() either).  I'm not sure it would simplify things that
 > > much anyway, so would rather avoid adding it.
 > >
 > 
 > I understand the decision in the context where relayfs exports default files,
 > but, as the clients now have to provide debugfs callbacks, they will have to
 > deal with the is_global flag in every case. Between passing an
argument to
 > relay_open or using the private_data to pass wether or not the channel must
 > be global, so it can be used later in create_buf_file to set the output
 > is_global variable, I think the relay_open extra parameter provides a much
 > simpler interface.
 > 
 > If we think of the parameters passed to relay_open() as the "attributes"
 > associated with the channel, it makes sense to provide them as parameter or set
 > them all in one place (just like the k*alloc memory allocation is done).
 > 
 > We could also think of alternatives seen elsewhere (inotify interface could
 > be an example), where the data structure is first created, then the attributes
 > are set, and only then is it used. The equivalent would be : allocate struct
 > rchan, set the attributes, call relay_open passing the already allocated struct
 > rchan as parameter.
 > 
 > About API changes, I believe that if we add the private_data parameter, as
 > we are already doing an API change, the timing would be good to add is_global
 > at the same time.
 > 
 > If what you are looking for is a simple relay_open, with few parameters, we
 > could think of changing the is_global flag for a byte within a "flags" integer,
 > just like k*alloc : it would leave space for expansion. We can also
make a

I think this is actually the best option.  My assumption has always
been that if we wanted to add something that would require a new
'flag' to relay_open(), it could equivalently be implemented as a new
callback.  The default implementation would implement the common case,
the normal user would never have to pay attention to it, and
relay_open() would remain unchanged and as simple as possible.  In
practice though, unless there are only one or two of these, the
flag-as-callback method probably only adds confusion, since you have a
set of attributes to think about in two different places, and it's
also different from what people are used to.  And I can think of at
least a couple other features besides is_global that would require a
flag, so it probably does makes sense to add a flags param for them
all to live in.

Tom