Re: [PATCH] Relay CPU hotplug support

* Tom Zanussi ([email protected]) wrote:
> Mathieu Desnoyers writes:
>  > 
>  > 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.
>  > 
> 
> Hi Mathieu,
> 
> I guess I'm missing something, since I don't think cpu hotplug changes
> the way the global channels are handled much, other than move the
> is_global flag into the channel and force the global buffer to occupy
> chan->buf[0], which your patch also does.
> 
> I don't understand what you mean by being 'held by a writer' - when
> you're done with the channel and call relay_close(), that finalizes
> the buffer and decrements the kref; if there are no readers it goes
> away, if there are it waits until the file is released.
> 
> In any case, I'm assuming in my comments (following) that for global
> buffers, relay_open() creates buf[0] and and additionally sets that as
> the buf for buf[0..NCPUS], which should allow cpu hotplug events to be
> ignored for the global case.  
> 

Hi Tom,

I see what is missing here : in my first implementation, I tried to support both
cpu online and cpu offline events. There, I needed to have the ability to
add/remove a channel buffer on the fly ; when a channel was global, then the
buffer for the new CPU would point to the global buffer and the krefw refcount
would be incremented.

The krefw was only necessary to keep track of the number of CPUs pointing to the
global subbuffer in a global channel, so the finalize would only occur when the
last CPU would be down (or relay_close_buf be called for this CPU).

I made a mistake in my relay_close_buf is_global implementation : it should
iterate on each possible CPUs to close their buffers, even in is_global mode, as
the refcount has to be decremented.

Here is a patch that does it. But I guess you won't need it if you ignore CPU
hotplug events for the global case.

Regards,

Mathieu

--- a/kernel/relay.c
+++ b/kernel/relay.c
@@ -588,15 +588,10 @@ struct rchan *relay_open(const char *bas
 	return chan;
 
 free_bufs:
-	if (chan->is_global) {
-		if (chan->buf[0])
-			relay_close_buf(chan->buf[0]);
-	} else {
-		for_each_online_cpu(i) {
-			if (!chan->buf[i])
-				break;
-			relay_close_buf(chan->buf[i]);
-		}
+	for_each_online_cpu(i) {
+		if (!chan->buf[i])
+			break;
+		relay_close_buf(chan->buf[i]);
 	}
 
 	kref_put(&chan->kref, relay_destroy_channel);
@@ -705,13 +700,9 @@ void relay_close(struct rchan *chan)
 		return;
 
 	lock_cpu_hotplug();
-	if (chan->is_global) {
-		if(chan->buf[0])
-			relay_close_buf(chan->buf[0]);
-	} else
-		for_each_possible_cpu(i)
-			if (chan->buf[i])
-				relay_close_buf(chan->buf[i]);
+	for_each_possible_cpu(i)
+		if (chan->buf[i])
+			relay_close_buf(chan->buf[i]);
 
 	if (chan->last_toobig)
 		printk(KERN_WARNING "relay: one or more items not logged "



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