Re: [PATCH] Relay CPU hotplug support

Hi Mathieu,

Mathieu Desnoyers writes:
 > >  > +
 > >  > +	if(chan->is_global) {
 > >  > +		chan->buf[0] = buf;
 > >
 > > If we added a foreach_possible_cpu() chan_buf[i] = buf here, we should
 > > then be able to ignore hotcpu events altogether for global channels.
 > >
 > 
 > We are already called from a for_each_online_cpu loop here. And we only know
 > that we are a global channel after the create_buf_file callback has been
 > called. It would would work, but it means that the chan_buf[i] will be set
 > twice : in the outer foreach_online_cpu() loop and in the inner
 > foreach_possible_cpu() loop, which I would like to avoid.

Right, good point.

 > 
 > 
 > >  > +		buf->cpu = 0;
 > >  >  	}
 > >  >
 > >
 > > Also, since we're now setting chan->buf[0] here, it might make sense
 > > to do the same for the non-global case as well and change the return
 > > value from struct rchan_buf * to an integer return code, since it no
 > > longer needs to return a pointer to the buf.  If it helps, we might
 > > also be able to move it to before create_buf_file() and have access to
 > > buf->chan->buf[], but I haven't thought about whether it would break
 > > anything.  On second thought, it's probably cleaner to leave it as it
 > > is.
 > >
 > 
 > I think that passing the is_global flag as a parameter to relay_open would
 > simplify a lot of this stuff. I do not use it in LTTng, but if I would like to,
 > that would be impossible : the private_data is not set when the create_buf_file
 > cb is called, so I have no means of specifying my preference about a specific
 > channel being global or not, which means that the callback cannot set the
 > is_global flag according the my client's preference.
 > 
 > 

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.

Tom