[PATCH] kill rtnl_exlock stubs

[PATCH] kill rtnl_exlock stubs

[Stephen Hemminger]

Rtnetlink has some macro's that are relics from earlier locking.
They are only used a couple of places so are easy to kill.

diff -Nru a/include/linux/rtnetlink.h b/include/linux/rtnetlink.h
--- a/include/linux/rtnetlink.h	2004-07-27 09:40:03 -07:00
+++ b/include/linux/rtnetlink.h	2004-07-27 09:40:03 -07:00
@@ -746,10 +746,6 @@
 
 extern struct semaphore rtnl_sem;
 
-#define rtnl_exlock()		do { } while(0)
-#define rtnl_exunlock()		do { } while(0)
-#define rtnl_exlock_nowait()	(0)
-
 #define rtnl_shlock()		down(&rtnl_sem)
 #define rtnl_shlock_nowait()	down_trylock(&rtnl_sem)
 
diff -Nru a/net/core/dev.c b/net/core/dev.c
--- a/net/core/dev.c	2004-07-27 09:40:03 -07:00
+++ b/net/core/dev.c	2004-07-27 09:40:03 -07:00
@@ -3041,7 +3041,6 @@
 	while (atomic_read(&dev->refcnt) != 0) {
 		if (time_after(jiffies, rebroadcast_time + 1 * HZ)) {
 			rtnl_shlock();
-			rtnl_exlock();
 
 			/* Rebroadcast unregister notification */
 			notifier_call_chain(&netdev_chain,
@@ -3058,7 +3057,6 @@
 				linkwatch_run_queue();
 			}
 
-			rtnl_exunlock();
 			rtnl_shunlock();
 
 			rebroadcast_time = jiffies;
diff -Nru a/net/core/link_watch.c b/net/core/link_watch.c
--- a/net/core/link_watch.c	2004-07-27 09:40:03 -07:00
+++ b/net/core/link_watch.c	2004-07-27 09:40:03 -07:00
@@ -93,9 +93,7 @@
 	clear_bit(LW_RUNNING, &linkwatch_flags);
 
 	rtnl_shlock();
-	rtnl_exlock();
 	linkwatch_run_queue();
-	rtnl_exunlock();
 	rtnl_shunlock();
 }
 
diff -Nru a/net/core/rtnetlink.c b/net/core/rtnetlink.c
--- a/net/core/rtnetlink.c	2004-07-27 09:40:03 -07:00
+++ b/net/core/rtnetlink.c	2004-07-27 09:40:03 -07:00
@@ -56,12 +56,10 @@
 void rtnl_lock(void)
 {
 	rtnl_shlock();
-	rtnl_exlock();
 }
  
 void rtnl_unlock(void)
 {
-	rtnl_exunlock();
 	rtnl_shunlock();
 
 	netdev_run_todo();
@@ -404,13 +402,8 @@
 		return -1;
 	}
 
-	if (kind != 2) {
-		if (rtnl_exlock_nowait()) {
-			*errp = 0;
-			return -1;
-		}
+	if (kind != 2) 
 		exclusive = 1;
-	}
 
 	memset(&rta, 0, sizeof(rta));
 
@@ -439,14 +432,10 @@
 		goto err_inval;
 	err = link->doit(skb, nlh, (void *)&rta);
 
-	if (exclusive)
-		rtnl_exunlock();
 	*errp = err;
 	return err;
 
 err_inval:
-	if (exclusive)
-		rtnl_exunlock();
 	*errp = -EINVAL;
 	return -1;
 }

Re: [PATCH] kill rtnl_exlock stubs

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 301 bytes --]

Stephen Hemminger wrote:
> Rtnetlink has some macro's that are relics from earlier locking.
> They are only used a couple of places so are easy to kill.

The variable "exclusive" in rtnetlink_rcv_msg becomes useless with
this patch. This patch (on top of Stephen's patch) removes it.

Regards
Patrick

[-- Attachment #2: useless-var.diff --]
[-- Type: text/x-patch, Size: 828 bytes --]

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2004/07/27 20:32:23+02:00 kaber@trash.net 
#   [NET]: Remove useless variable in rtnetlink_rcv_msg
#   
#   Signed-off-by: Patrick McHardy <kaber@trash.net>
# 
# net/core/rtnetlink.c
#   2004/07/27 20:32:10+02:00 kaber@trash.net +0 -4
#   [NET]: Remove useless variable in rtnetlink_rcv_msg
# 
diff -Nru a/net/core/rtnetlink.c b/net/core/rtnetlink.c
--- a/net/core/rtnetlink.c	2004-07-27 20:33:47 +02:00
+++ b/net/core/rtnetlink.c	2004-07-27 20:33:47 +02:00
@@ -335,7 +335,6 @@
 	struct rtnetlink_link *link_tab;
 	struct rtattr	*rta[RTATTR_MAX];
 
-	int exclusive = 0;
 	int sz_idx, kind;
 	int min_len;
 	int family;
@@ -401,9 +400,6 @@
 		skb_pull(skb, rlen);
 		return -1;
 	}
-
-	if (kind != 2) 
-		exclusive = 1;
 
 	memset(&rta, 0, sizeof(rta));
 

Re: [PATCH] kill rtnl_exlock stubs

[David S. Miller]

On Tue, 27 Jul 2004 20:38:42 +0200
Patrick McHardy <kaber@trash.net> wrote:

> Stephen Hemminger wrote:
> > Rtnetlink has some macro's that are relics from earlier locking.
> > They are only used a couple of places so are easy to kill.
> 
> The variable "exclusive" in rtnetlink_rcv_msg becomes useless with
> this patch. This patch (on top of Stephen's patch) removes it.

Both patches look fine, thanks guys.

I thought maybe Alexey might have had some fantastic
plan with exclusive locks, but this stuff has gone
beyond bitrot :-)

Re: [PATCH] kill rtnl_exlock stubs

[Jamal Hadi Salim]

On Tue, 2004-07-27 at 12:50, Stephen Hemminger wrote:

> @@ -404,13 +402,8 @@
>  		return -1;
>  	}
>  
> -	if (kind != 2) {
> -		if (rtnl_exlock_nowait()) {
> -			*errp = 0;
> -			return -1;
> -		}
> +	if (kind != 2) 
>  		exclusive = 1;
> -	}
>  
>  	memset(&rta, 0, sizeof(rta));
>  
> @@ -439,14 +432,10 @@
>  		goto err_inval;
>  	err = link->doit(skb, nlh, (void *)&rta);
>  
> -	if (exclusive)
> -		rtnl_exunlock();
>  	*errp = err;
>  	return err;
>  
>  err_inval:
> -	if (exclusive)
> -		rtnl_exunlock();
>  	*errp = -EINVAL;
>  	return -1;
>  }

This piece is the only one i would worry about.
I dont remember the historical reasoning for rtnl_ex* I know its not
useful as is right now. OTOH, if it was useful then the above code would
have meant something. Dave/Alexey?

cheers,
jamal

Re: [PATCH] kill rtnl_exlock stubs

[jamal]

On Thu, 2004-07-29 at 09:16, Jamal Hadi Salim wrote:
> On Tue, 2004-07-27 at 12:50, Stephen Hemminger wrote:
> 
> > @@ -404,13 +402,8 @@
> >  		return -1;
> >  	}
> >  
> > -	if (kind != 2) {
> > -		if (rtnl_exlock_nowait()) {
> > -			*errp = 0;
> > -			return -1;
> > -		}
> > +	if (kind != 2) 
> >  		exclusive = 1;
> > -	}
> >  
> >  	memset(&rta, 0, sizeof(rta));
> >  
> > @@ -439,14 +432,10 @@
> >  		goto err_inval;
> >  	err = link->doit(skb, nlh, (void *)&rta);
> >  
> > -	if (exclusive)
> > -		rtnl_exunlock();
> >  	*errp = err;
> >  	return err;
> >  
> >  err_inval:
> > -	if (exclusive)
> > -		rtnl_exunlock();
> >  	*errp = -EINVAL;
> >  	return -1;
> >  }
> 
> This piece is the only one i would worry about.
> I dont remember the historical reasoning for rtnl_ex* I know its not
> useful as is right now. OTOH, if it was useful then the above code would
> have meant something. Dave/Alexey?

I think i may have figured it out.
The rtnl_ex* seems to have been intended as more fine grain exclusive
writer locking to the ->doit() functions which do make modifications to
data. In other words if there are several rtnetlink sockets trying to
modify the routing table, this would act as a serialization point.

At the moment, the RTNL_SEM (grabbed around rtnetlink_rcv() 
viartnl_shlock_nowait ) seems to protect on this but its too 
fat a lock. So at some point we may need to clean it.

Before you take my word on it, lets wait to hear from Alexey/Dave.

cheers,
jamal