Patchwork [1/1] netstamp_needed shouldn't be jump_label_key

login
register
mail settings
Submitter Igor Maravić
Date Nov. 29, 2011, 9:03 a.m.
Message ID <fba9911084786bf04747c0350e774615.squirrel@kondor.etf.bg.ac.rs>
Download mbox | patch
Permalink /patch/128241/
State Rejected
Delegated to: David Miller
Headers show

Comments

Igor Maravić - Nov. 29, 2011, 9:03 a.m.
>
> Could you test following patch ?
>

I've just put it to compile. I'l let you know soon as it finishes compiling.

I have one proposition for your patch. To put WARN_ON before in ifdef endif,
because we don't needed if we don't have HAVE_JUMP_LABEL, and also to check it
before using jump_label_dec.

Also I have few questions :)

First - why can't we use spin_lock on jump_label?

I know that You said that is because we are using mutex_lock in
arch_jump_label_transform() or text_poke_smp(), but I don't see why couldn't
we use mutex_lock inside spin_lock.

To me using spin_lock_irqsave and spin_lock_irqrestore does sound like as most logical solution.
With that lock, we would be sure that our mutex_lock in arch_jump_label_transform() or text_poke_smp()
isn't going to be interrupted.
Please correct if I'm wrong.

Also I saw that you put calling of net_enable_timestamp outside of spin_lock_bh
Why?

BR
Igor

[PATCH 1/1] jump_label warning if_interrupt

Move warning on the top of function net_enable_timestamp,
so we would be also warn if we are going to jump_label_dec in interrupt

Signed-off-by: Igor Maravic <igorm@etf.rs>

:100644 100644 45eab03... ef23cf7... M	net/core/dev.c
Eric Dumazet - Nov. 29, 2011, 9:31 a.m.
Le mardi 29 novembre 2011 à 10:03 +0100, "Igor Maravić" a écrit :
> >
> > Could you test following patch ?
> >
> 
> I've just put it to compile. I'l let you know soon as it finishes compiling.
> 
> I have one proposition for your patch. To put WARN_ON before in ifdef endif,
> because we don't needed if we don't have HAVE_JUMP_LABEL, and also to check it
> before using jump_label_dec.
> 
> Also I have few questions :)
> 
> First - why can't we use spin_lock on jump_label?
> 
> I know that You said that is because we are using mutex_lock in
> arch_jump_label_transform() or text_poke_smp(), but I don't see why couldn't
> we use mutex_lock inside spin_lock.
> 


Try it, you'll see how bad it is.

> To me using spin_lock_irqsave and spin_lock_irqrestore does sound like as most logical solution.
> With that lock, we would be sure that our mutex_lock in arch_jump_label_transform() or text_poke_smp()
> isn't going to be interrupted.
> Please correct if I'm wrong.
> 
> Also I saw that you put calling of net_enable_timestamp outside of spin_lock_bh
> Why?
> 

Because its the same problem. We want to be allowed to sleep.

> BR
> Igor
> 
> [PATCH 1/1] jump_label warning if_interrupt
> 
> Move warning on the top of function net_enable_timestamp,
> so we would be also warn if we are going to jump_label_dec in interrupt
> 
> Signed-off-by: Igor Maravic <igorm@etf.rs>
> 
> :100644 100644 45eab03... ef23cf7... M	net/core/dev.c
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 45eab03..ef23cf7 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1453,6 +1453,7 @@ void net_enable_timestamp(void)
>  {
>  #ifdef HAVE_JUMP_LABEL
>  	int deferred = atomic_xchg(&netstamp_needed_deferred, 0);
> +	WARN_ON(in_interrupt());
> 
>  	if (deferred) {
>  		while (--deferred)
> @@ -1460,7 +1461,6 @@ void net_enable_timestamp(void)
>  		return;
>  	}
>  #endif
> -	WARN_ON(in_interrupt());
>  	jump_label_inc(&netstamp_needed);
>  }
>  EXPORT_SYMBOL(net_enable_timestamp);

Its better to have coverage of the test, even on machines with no
HAVE_JUMP_LABEL.

So move the test before the 
#ifdef HAVE_JUMP_LABEL


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/net/core/dev.c b/net/core/dev.c
index 45eab03..ef23cf7 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1453,6 +1453,7 @@  void net_enable_timestamp(void)
 {
 #ifdef HAVE_JUMP_LABEL
 	int deferred = atomic_xchg(&netstamp_needed_deferred, 0);
+	WARN_ON(in_interrupt());

 	if (deferred) {
 		while (--deferred)
@@ -1460,7 +1461,6 @@  void net_enable_timestamp(void)
 		return;
 	}
 #endif
-	WARN_ON(in_interrupt());
 	jump_label_inc(&netstamp_needed);
 }
 EXPORT_SYMBOL(net_enable_timestamp);