Re: [PATCH] Relay CPU hotplug support

Mathieu Desnoyers writes:
 > Hi Tom,
 > 
 > Here is my version of the relay patch for CPU hotplug support. It uses a
 > channel list within relay, as you suggested. The patch is based on a 2.6.18
 > kernel.
 > 
 > Comments are welcome,
 > 

Hi Mathieu,

I still need to go over the patch in detail, but there are a couple of
things I don't quite understand yet - could you explain them a little
to get me on the right track (see below)?

[...]

 >  /**
 >   *	relay_open_buf - create a new relay channel buffer
 >   *
 > - *	Internal - used by relay_open().
 > + *	used by relay_open() and CPU hotplug.
 >   */
 > -static struct rchan_buf *relay_open_buf(struct rchan *chan,
 > -					const char *filename,
 > -					struct dentry *parent,
 > -					int *is_global)
 > +struct rchan_buf *relay_open_buf(struct rchan *chan, unsigned int cpu)
 >  {
 > -	struct rchan_buf *buf;
 > +	struct rchan_buf *buf = NULL;
 >  	struct dentry *dentry;
 > +	char *tmpname;
 > 
 > -	if (*is_global)
 > +	if (chan->is_global) {
 > +		kref_get(&chan->buf[0]->krefw);
 >  		return chan->buf[0];
 > +	}
 > +
 > +	tmpname = kmalloc(NAME_MAX + 1, GFP_KERNEL);
 > +	if (!tmpname)
 > +		goto end;
 > +	sprintf(tmpname, "%s%d", chan->base_filename, cpu);
 > 
 >  	buf = relay_create_buf(chan);
 >  	if (!buf)
 > -		return NULL;
 > +		goto free_name;
 > +
 > +	buf->cpu = cpu;
 > +	__relay_reset(buf, 1);
 > 
 >  	/* Create file in fs */
 > -	dentry = chan->cb->create_buf_file(filename, parent, S_IRUSR,
 > -					   buf, is_global);
 > -	if (!dentry) {
 > -		relay_destroy_buf(buf);
 > -		return NULL;
 > +	dentry = chan->cb->create_buf_file(tmpname, chan->parent, S_IRUSR,
 > +					   buf, &chan->is_global);
 > +	if (!dentry)
 > +		goto free_buf;
 > +
 > +	if(chan->is_global) {
 > +		chan->buf[0] = buf;
 > +		buf->cpu = 0;
 >  	}
 > 
 >  	buf->dentry = dentry;
 > -	__relay_reset(buf, 1);
 > +	goto free_name;
 > 
 > +free_buf:
 > +	relay_destroy_buf(buf);
 > +free_name:
 > +	kfree(tmpname);
 > +end:
 >  	return buf;
 >  }
 > +EXPORT_SYMBOL_GPL(relay_open_buf);
 > 

I understand why you changed relay_open_buf(), which makes sense, but
I don't see why it's exported now.

 >  /**
 > - *	relay_close_buf - close a channel buffer
 > - *	@buf: channel buffer
 > + *	relay_close_write_buf - close write to a channel buffer
 > + *	@kref: buffer writers reference
 >   *
 >   *	Marks the buffer finalized and restores the default callbacks.
 >   *	The channel buffer and channel buffer data structure are then freed
 >   *	automatically when the last reference is given up.
 >   */
 > -static inline void relay_close_buf(struct rchan_buf *buf)
 > +static void relay_close_write_buf(struct kref *kref)
 >  {
 > +	struct rchan_buf *buf = container_of(kref, struct rchan_buf, krefw);
 > +
 >  	buf->finalized = 1;
 >  	cancel_delayed_work(&buf->wake_readers);
 >  	flush_scheduled_work();
 >  	kref_put(&buf->kref, relay_remove_buf);
 >  }
 > 
 > +/**
 > + *	relay_close_buf - close a channel buffer
 > + *	@buf: channel buffer
 > + *	
 > + *	Remove a writer reference.
 > + */
 > +
 > +void relay_close_buf(struct rchan_buf *buf)
 > +{
 > +	kref_put(&buf->krefw, relay_close_write_buf);
 > +}
 > +EXPORT_SYMBOL_GPL(relay_close_buf);
 > +

I don't really understand the additional kref (buf->krefw), or why you
split relay_close_buf() and exported it.

[...]

 > +
 > +/**
 >   *	relay_open - create a new relay channel
 >   *	@base_filename: base name of files to create
 >   *	@parent: dentry of parent directory, NULL for root directory
 > @@ -464,13 +550,11 @@ struct rchan *relay_open(const char *bas
 >  			 struct dentry *parent,
 >  			 size_t subbuf_size,
 >  			 size_t n_subbufs,
 > -			 struct rchan_callbacks *cb)
 > +			 struct rchan_callbacks *cb,
 > +			 void *private_data)
 >  {

Also, why you had to add private_data to relay_open() - I think it
would be better to avoid changing existing API functions if
possible...

Thanks,

Tom