Re: [PATCH] (1/8) Eliminate brlock in psnap

Re: [PATCH] (1/8) Eliminate brlock in psnap

[Paul McKenney]

> On Tue, 11 Mar 2003, Stephen Hemminger wrote:
>
> >  void unregister_snap_client(struct datalink_proto *proto)
> >  {
> > -        br_write_lock_bh(BR_NETPROTO_LOCK);
> > +        static RCU_HEAD(snap_rcu);
> >
> > -        list_del(&proto->node);
> > -        kfree(proto);
> > +        spin_lock_bh(&snap_lock);
> > +        list_del_rcu(&proto->node);
> > +        spin_unlock_bh(&snap_lock);
> >
> > -        br_write_unlock_bh(BR_NETPROTO_LOCK);
> > +        call_rcu(&snap_rcu, (void (*)(void *)) kfree, proto);
> >  }
>
> Do we need the spin_lock_bh around the list_del_rcu? But also How
> about. This way we don't change the previous characteristic of block till
> done unregistering
>
> struct datalink_proto {
> ...
>            struct completion registration;
> };
>
> void __unregister_snap_client(void *__proto)
> {
>            struct datalink_proto *proto = __proto;
>            complete(&proto->registration);
> }
>
> unregister_snap_client(struct datalink_proto *proto)
> {
>            list_del_rcu(&proto->node);
>            call_rcu(&snap_rcu, __unregister_snap_client, proto);
>            wait_for_completion(&proto->registration);
>            kfree(proto);
> }

You are saying that we can omit locking because this is
called only from a module-exit function, and thus protected
by the module_mutex semaphore?  (I must defer to
others who have a better handle on modules...)

If in fact only one module-exit function can be
executing at a given time, then we should be able to
use the following approach:

void unregister_snap_client(struct datalink_proto *proto)
{
      list_del_rcu(&proto->node); /* protected by module_mutex. */
      synchronize_kernel();
      kfree(proto);
}

If multiple module-exit functions can somehow execute
concurrently, then something like the following would
be needed:

void unregister_snap_client(struct datalink_proto *proto)
{
      spin_lock_bh(&snap_lock);
      list_del_rcu(&proto->node);
      spin_unlock_bh(&snap_lock);
      synchronize_kernel();
      kfree(proto);
}

Module unloading should be rare enough to tolerate
the grace period under the module_mutex, right?

Thoughts?

                        Thanx, Paul

Re: [PATCH] (1/8) Eliminate brlock in psnap

[Zwane Mwaikambo]

On Wed, 12 Mar 2003, Paul McKenney wrote:

> You are saying that we can omit locking because this is
> called only from a module-exit function, and thus protected
> by the module_mutex semaphore?  (I must defer to
> others who have a better handle on modules...)
> 
> If in fact only one module-exit function can be
> executing at a given time, then we should be able to
> use the following approach:

Yes the ->exit call is protected by module_mutex globally.
 
> Module unloading should be rare enough to tolerate
> the grace period under the module_mutex, right?
> 
> Thoughts?

I would agree. However can't unregister_snap_client be used in other paths 
apart from module_unload? I wouldn't worry too much since if 
register_snap_client and unregister_snap_client for the same protocol 
races it's a bug in the caller's code. The safe RCU list removal and 
synchronize_kernel should protect us from sane usage.

	Zwane

Re: [PATCH] (1/8) Eliminate brlock in psnap

[Paul McKenney]

> > Module unloading should be rare enough to tolerate
> > the grace period under the module_mutex, right?
> >
> > Thoughts?
>
> I would agree. However can't unregister_snap_client be used in other
paths
> apart from module_unload? I wouldn't worry too much since if
> register_snap_client and unregister_snap_client for the same protocol
> races it's a bug in the caller's code. The safe RCU list removal and
> synchronize_kernel should protect us from sane usage.
>
>            Zwane

It is not called from elsewhere right now, but if there
is a possibility that it might be, Steve of course should
keep the locking in these functions.

                              Thanx, Paul

Re: [PATCH] (1/8) Eliminate brlock in psnap

[Zwane Mwaikambo]

On Tue, 11 Mar 2003, Stephen Hemminger wrote:

>  void unregister_snap_client(struct datalink_proto *proto)
>  {
> -	br_write_lock_bh(BR_NETPROTO_LOCK);
> +	static RCU_HEAD(snap_rcu);
>  
> -	list_del(&proto->node);
> -	kfree(proto);
> +	spin_lock_bh(&snap_lock);
> +	list_del_rcu(&proto->node);
> +	spin_unlock_bh(&snap_lock);
>  
> -	br_write_unlock_bh(BR_NETPROTO_LOCK);
> +	call_rcu(&snap_rcu, (void (*)(void *)) kfree, proto);
>  }

Do we need the spin_lock_bh around the list_del_rcu? But also How 
about. This way we don't change the previous characteristic of block till 
done unregistering

struct datalink_proto {
...
	struct completion registration;
};

void __unregister_snap_client(void *__proto)
{
	struct datalink_proto *proto = __proto;
	complete(&proto->registration);
}

unregister_snap_client(struct datalink_proto *proto)
{
	list_del_rcu(&proto->node);
	call_rcu(&snap_rcu, __unregister_snap_client, proto);
	wait_for_completion(&proto->registration);
	kfree(proto);
}

Re: [PATCH] (1/8) Eliminate brlock in psnap

[Roman Zippel]

Hi,

On 11 Mar 2003, Stephen Hemminger wrote:

>  void unregister_snap_client(struct datalink_proto *proto)
>  {
> -	br_write_lock_bh(BR_NETPROTO_LOCK);
> +	static RCU_HEAD(snap_rcu);
>  
> -	list_del(&proto->node);
> -	kfree(proto);
> +	spin_lock_bh(&snap_lock);
> +	list_del_rcu(&proto->node);
> +	spin_unlock_bh(&snap_lock);
>  
> -	br_write_unlock_bh(BR_NETPROTO_LOCK);
> +	call_rcu(&snap_rcu, (void (*)(void *)) kfree, proto);
>  }

Is this really correct? What happens with snap_rcu, if 
unregister_snap_client is called again, before the call_rcu callback 
finished?

bye, Roman

[PATCH] (1/8) Eliminate brlock in psnap

[Stephen Hemminger]

The following replaces brlock with RCU in the SNAP module.

diff -urN -X dontdiff linux-2.5.64/net/802/psnap.c linux-2.5-nobrlock/net/802/psnap.c
--- linux-2.5.64/net/802/psnap.c	2003-03-11 09:08:01.000000000 -0800
+++ linux-2.5-nobrlock/net/802/psnap.c	2003-03-10 15:48:53.000000000 -0800
@@ -21,10 +21,10 @@
 #include <linux/mm.h>
 #include <linux/in.h>
 #include <linux/init.h>
-#include <linux/brlock.h>
 
 LIST_HEAD(snap_list);
 static struct llc_sap *snap_sap;
+static spinlock_t snap_lock = SPIN_LOCK_UNLOCKED;
 
 /*
  *	Find a snap client by matching the 5 bytes.
@@ -34,17 +34,15 @@
 	struct list_head *entry;
 	struct datalink_proto *proto = NULL, *p;
 
-	if (list_empty(&snap_list))
-		goto out;
-
-	list_for_each(entry, &snap_list) {
+	rcu_read_lock();
+	list_for_each_rcu(entry, &snap_list) {
 		p = list_entry(entry, struct datalink_proto, node);
 		if (!memcmp(p->type, desc, 5)) {
 			proto = p;
 			break;
 		}
 	}
-out:
+	rcu_read_unlock();
 	return proto;
 }
 
@@ -124,8 +122,7 @@
 {
 	struct datalink_proto *proto = NULL;
 
-	br_write_lock_bh(BR_NETPROTO_LOCK);
-
+	spin_lock_bh(&snap_lock);
 	if (find_snap_client(desc))
 		goto out;
 
@@ -135,10 +132,10 @@
 		proto->rcvfunc		= rcvfunc;
 		proto->header_length	= 5 + 3; /* snap + 802.2 */
 		proto->request		= snap_request;
-		list_add(&proto->node, &snap_list);
+		list_add_rcu(&proto->node, &snap_list);
 	}
 out:
-	br_write_unlock_bh(BR_NETPROTO_LOCK);
+	spin_unlock_bh(&snap_lock);
 	return proto;
 }
 
@@ -147,12 +144,13 @@
  */
 void unregister_snap_client(struct datalink_proto *proto)
 {
-	br_write_lock_bh(BR_NETPROTO_LOCK);
+	static RCU_HEAD(snap_rcu);
 
-	list_del(&proto->node);
-	kfree(proto);
+	spin_lock_bh(&snap_lock);
+	list_del_rcu(&proto->node);
+	spin_unlock_bh(&snap_lock);
 
-	br_write_unlock_bh(BR_NETPROTO_LOCK);
+	call_rcu(&snap_rcu, (void (*)(void *)) kfree, proto);
 }
 
 MODULE_LICENSE("GPL");