Re: [PATCH] Relay CPU hotplug support

Hi Tom,

* Tom Zanussi ([email protected]) wrote:
> 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.
> 

Sorry, it's a leftover from my own changes where I did not use any list. You can
remove the export.

>  >  /**
>  > - *	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.
> 

We can remove the export here too. In fact, I removed the prototypes from
relay.h, but forgot to remove the export.

Now for buf->krefw :

I wanted to separate the fact that a buffer is held by a reader from the fact
that a buffer is held by a writer. By doing this, we can support the is_global
channels more efficiently :

for example, if we have 8 CPUs with a is_global channel, what we want is to wait
until the last writer closes the buffer. Only at that point do we want to
finalize the buffer (this is why the finalize and flush work is done in the
function called upon krefw final put). But, we only want to free the buffer once
the last reader released it : this is why the original kref is still there.


> [...]
> 
>  > +
>  > +/**
>  >   *	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...
> 

It's there because I need, in the client, to have access to the private_data
when the create_buf_file is called. The difference now is that this callback is
not only responsible for creating any client specific files and data structures
when relay_open is called, but it is also called when a CPU hotplug online event
occurs. So, what I could do before just after the call to relay_open (creation
of my private data) couldn't be done elegantly anymore without passing the
private_data to the relay_open call.

Thanks,

Mathieu


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