Re: [PATCH] Relay CPU hotplug support

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
"simplified" version of relay_open() for the common case using a macro, if
you really want to simplify things for users, but I doubt it would be of any
help.

For internal code simplification, which is the last of my concerns, it would
permit to separate the relay_open in two clear scenarios : one with is_global
(where we allocate the buf[0] and set pointers in every other possible CPU) and
the other where we disable cpu hotplug and iterate on each online CPU.

Those are my 2 cents, the decision is yours.

Regards,

Mathieu


OpenPGP public key:              http://krystal.dyndns.org:8080/key/compudj.gpg
Key fingerprint:     8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68