[PATCH][UPSTREAM]net,RT:Remove preemption disabling in netif_rx()

[PATCH][UPSTREAM]net,RT:Remove preemption disabling in netif_rx()

[Priyanka Jain]

1)enqueue_to_backlog() (called from netif_rx) should be
  bind to a particluar CPU. This can be achieved by
  disabling migration. No need to disable preemption

2)Fixes crash "BUG: scheduling while atomic: ksoftirqd"
  in case of RT.
  If preemption is disabled, enqueue_to_backog() is called
  in atomic context. And if backlog exceeds its count,
  kfree_skb() is called. But in RT, kfree_skb() might
  gets scheduled out, so it expects non atomic context.

3)When CONFIG_PREEMPT_RT_FULL is not defined,
 migrate_enable(), migrate_disable() maps to
 preempt_enable() and preempt_disable(), so no
 change in functionality in case of non-RT.

-Replace preempt_enable(), preempt_disable() with
 migrate_enable(), migrate_disable() respectively
-Replace get_cpu(), put_cpu() with get_cpu_light(),
 put_cpu_light() respectively

Signed-off-by: Priyanka Jain <Priyanka.Jain@freescale.com>
Acked-by: Rajan Srivastava <Rajan.Srivastava@freescale.com>
---
 Testing: Tested successfully on p4080ds(8-core SMP system)  

 net/core/dev.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 452db70..4017820 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2940,7 +2940,7 @@ int netif_rx(struct sk_buff *skb)
 		struct rps_dev_flow voidflow, *rflow = &voidflow;
 		int cpu;
 
-		preempt_disable();
+		migrate_disable();
 		rcu_read_lock();
 
 		cpu = get_rps_cpu(skb->dev, skb, &rflow);
@@ -2950,13 +2950,13 @@ int netif_rx(struct sk_buff *skb)
 		ret = enqueue_to_backlog(skb, cpu, &rflow->last_qtail);
 
 		rcu_read_unlock();
-		preempt_enable();
+		migrate_enable();
 	} else
 #endif
 	{
 		unsigned int qtail;
-		ret = enqueue_to_backlog(skb, get_cpu(), &qtail);
-		put_cpu();
+		ret = enqueue_to_backlog(skb, get_cpu_light(), &qtail);
+		put_cpu_light();
 	}
 	return ret;
 }
-- 
1.7.4.1

RE: [PATCH][UPSTREAM]net,RT:Remove preemption disabling in netif_rx()

[Jain Priyanka-B32167]

Waiting for review comments on this.

Thanks
Priyanka

-----Original Message-----
From: Jain Priyanka-B32167 
Sent: Thursday, May 17, 2012 9:35 AM
To: linux-rt-users@vger.kernel.org
Cc: tglx@linutronix.de; rostedt@goodmis.orgn; Srivastava Rajan-B34330; Jain Priyanka-B32167
Subject: [PATCH][UPSTREAM]net,RT:Remove preemption disabling in netif_rx()

1)enqueue_to_backlog() (called from netif_rx) should be
  bind to a particluar CPU. This can be achieved by
  disabling migration. No need to disable preemption

2)Fixes crash "BUG: scheduling while atomic: ksoftirqd"
  in case of RT.
  If preemption is disabled, enqueue_to_backog() is called
  in atomic context. And if backlog exceeds its count,
  kfree_skb() is called. But in RT, kfree_skb() might
  gets scheduled out, so it expects non atomic context.

3)When CONFIG_PREEMPT_RT_FULL is not defined,  migrate_enable(), migrate_disable() maps to
 preempt_enable() and preempt_disable(), so no  change in functionality in case of non-RT.

-Replace preempt_enable(), preempt_disable() with  migrate_enable(), migrate_disable() respectively -Replace get_cpu(), put_cpu() with get_cpu_light(),
 put_cpu_light() respectively

Signed-off-by: Priyanka Jain <Priyanka.Jain@freescale.com>
Acked-by: Rajan Srivastava <Rajan.Srivastava@freescale.com>
---
 Testing: Tested successfully on p4080ds(8-core SMP system)  

 net/core/dev.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c index 452db70..4017820 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2940,7 +2940,7 @@ int netif_rx(struct sk_buff *skb)
 		struct rps_dev_flow voidflow, *rflow = &voidflow;
 		int cpu;
 
-		preempt_disable();
+		migrate_disable();
 		rcu_read_lock();
 
 		cpu = get_rps_cpu(skb->dev, skb, &rflow); @@ -2950,13 +2950,13 @@ int netif_rx(struct sk_buff *skb)
 		ret = enqueue_to_backlog(skb, cpu, &rflow->last_qtail);
 
 		rcu_read_unlock();
-		preempt_enable();
+		migrate_enable();
 	} else
 #endif
 	{
 		unsigned int qtail;
-		ret = enqueue_to_backlog(skb, get_cpu(), &qtail);
-		put_cpu();
+		ret = enqueue_to_backlog(skb, get_cpu_light(), &qtail);
+		put_cpu_light();
 	}
 	return ret;
 }
--
1.7.4.1

RE: [PATCH][UPSTREAM]net,RT:Remove preemption disabling in netif_rx()

[Steven Rostedt]

On Thu, 2012-05-24 at 04:28 +0000, Jain Priyanka-B32167 wrote:
> Waiting for review comments on this.
> 

> diff --git a/net/core/dev.c b/net/core/dev.c index 452db70..4017820 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2940,7 +2940,7 @@ int netif_rx(struct sk_buff *skb)
>  		struct rps_dev_flow voidflow, *rflow = &voidflow;
>  		int cpu;
>  
> -		preempt_disable();
> +		migrate_disable();

I really want to avoid placing open coded migrate_disable() around the
kernel. Perhaps we should use "get_cpu_light()" here too.

-- Steve

>  		rcu_read_lock();
>  
>  		cpu = get_rps_cpu(skb->dev, skb, &rflow); @@ -2950,13 +2950,13 @@ int netif_rx(struct sk_buff *skb)
>  		ret = enqueue_to_backlog(skb, cpu, &rflow->last_qtail);
>  
>  		rcu_read_unlock();
> -		preempt_enable();
> +		migrate_enable();
>  	} else
>  #endif
>  	{
>  		unsigned int qtail;
> -		ret = enqueue_to_backlog(skb, get_cpu(), &qtail);
> -		put_cpu();
> +		ret = enqueue_to_backlog(skb, get_cpu_light(), &qtail);
> +		put_cpu_light();
>  	}
>  	return ret;
>  }
> --
> 1.7.4.1
> 
> 

RE: [PATCH][UPSTREAM]net,RT:Remove preemption disabling in netif_rx()

[Thomas Gleixner]

On Thu, 24 May 2012, Steven Rostedt wrote:

> On Thu, 2012-05-24 at 04:28 +0000, Jain Priyanka-B32167 wrote:
> > Waiting for review comments on this.
> > 
> 
> > diff --git a/net/core/dev.c b/net/core/dev.c index 452db70..4017820 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -2940,7 +2940,7 @@ int netif_rx(struct sk_buff *skb)
> >  		struct rps_dev_flow voidflow, *rflow = &voidflow;
> >  		int cpu;
> >  
> > -		preempt_disable();
> > +		migrate_disable();
> 
> I really want to avoid placing open coded migrate_disable() around the
> kernel. Perhaps we should use "get_cpu_light()" here too.

No. get_cpu_light() and migrate_disable() are different.

Following your argument we would have to replace preempt_disable()
with get_cpu() all over the place.

Thanks,

	tglx

RE: [PATCH][UPSTREAM]net,RT:Remove preemption disabling in netif_rx()

[Steven Rostedt]

On Sat, 2012-05-26 at 00:31 +0200, Thomas Gleixner wrote:
> On Thu, 24 May 2012, Steven Rostedt wrote:
> 
> > On Thu, 2012-05-24 at 04:28 +0000, Jain Priyanka-B32167 wrote:
> > > Waiting for review comments on this.
> > > 
> > 
> > > diff --git a/net/core/dev.c b/net/core/dev.c index 452db70..4017820 100644
> > > --- a/net/core/dev.c
> > > +++ b/net/core/dev.c
> > > @@ -2940,7 +2940,7 @@ int netif_rx(struct sk_buff *skb)
> > >  		struct rps_dev_flow voidflow, *rflow = &voidflow;
> > >  		int cpu;
> > >  
> > > -		preempt_disable();
> > > +		migrate_disable();
> > 
> > I really want to avoid placing open coded migrate_disable() around the
> > kernel. Perhaps we should use "get_cpu_light()" here too.
> 
> No. get_cpu_light() and migrate_disable() are different.
> 
> Following your argument we would have to replace preempt_disable()
> with get_cpu() all over the place.
> 

I didn't like the get_cpu_light either, but I thought it was Peter that
was against a 'migrate-disable()' API leaking all over the kernel.

IIRC, he gave in when it was part of a locking internal infrastructure.
Now it's coming to what he was against. Maybe he changed his mind. I'll
let him speak for himself.

-- Steve

RE: [PATCH][UPSTREAM]net,RT:Remove preemption disabling in netif_rx()

[Peter Zijlstra]

On Fri, 2012-05-25 at 18:43 -0400, Steven Rostedt wrote:
> On Sat, 2012-05-26 at 00:31 +0200, Thomas Gleixner wrote:
> > On Thu, 24 May 2012, Steven Rostedt wrote:
> > 
> > > On Thu, 2012-05-24 at 04:28 +0000, Jain Priyanka-B32167 wrote:
> > > > Waiting for review comments on this.
> > > > 
> > > 
> > > > diff --git a/net/core/dev.c b/net/core/dev.c index 452db70..4017820 100644
> > > > --- a/net/core/dev.c
> > > > +++ b/net/core/dev.c
> > > > @@ -2940,7 +2940,7 @@ int netif_rx(struct sk_buff *skb)
> > > >  		struct rps_dev_flow voidflow, *rflow = &voidflow;
> > > >  		int cpu;
> > > >  
> > > > -		preempt_disable();
> > > > +		migrate_disable();
> > > 
> > > I really want to avoid placing open coded migrate_disable() around the
> > > kernel. Perhaps we should use "get_cpu_light()" here too.
> > 
> > No. get_cpu_light() and migrate_disable() are different.
> > 
> > Following your argument we would have to replace preempt_disable()
> > with get_cpu() all over the place.
> > 
> 
> I didn't like the get_cpu_light either, but I thought it was Peter that
> was against a 'migrate-disable()' API leaking all over the kernel.
> 
> IIRC, he gave in when it was part of a locking internal infrastructure.
> Now it's coming to what he was against. Maybe he changed his mind. I'll
> let him speak for himself.

Ah, right, so I've mapped migrate_disable() to preempt_disable() for !
PREEMPT_RT. This to make sure people don't do stupid things with it :-)

Then again, people do lots of stupid things with preempt_disable() too,
but at least we don't grow actual migrate_disable() abuse.

I think I've done one or two migrate_disable() site like the proposed in
-rt already.

The thing I really want to avoid is introducing a migrate_disable() that
can schedule for !rt.

RE: [PATCH][UPSTREAM]net,RT:Remove preemption disabling in netif_rx()

[Jain Priyanka-B32167]

-----Original Message-----
From: linux-rt-users-owner@vger.kernel.org [mailto:linux-rt-users-owner@vger.kernel.org] On Behalf Of Peter Zijlstra
Sent: Tuesday, May 29, 2012 1:29 PM
To: Steven Rostedt
Cc: Thomas Gleixner; Jain Priyanka-B32167; linux-rt-users@vger.kernel.org; Srivastava Rajan-B34330
Subject: RE: [PATCH][UPSTREAM]net,RT:Remove preemption disabling in netif_rx()

On Fri, 2012-05-25 at 18:43 -0400, Steven Rostedt wrote:
> On Sat, 2012-05-26 at 00:31 +0200, Thomas Gleixner wrote:
> > On Thu, 24 May 2012, Steven Rostedt wrote:
> > 
> > > On Thu, 2012-05-24 at 04:28 +0000, Jain Priyanka-B32167 wrote:
> > > > Waiting for review comments on this.
> > > > 
> > > 
> > > > diff --git a/net/core/dev.c b/net/core/dev.c index 
> > > > 452db70..4017820 100644
> > > > --- a/net/core/dev.c
> > > > +++ b/net/core/dev.c
> > > > @@ -2940,7 +2940,7 @@ int netif_rx(struct sk_buff *skb)
> > > >  		struct rps_dev_flow voidflow, *rflow = &voidflow;
> > > >  		int cpu;
> > > >  
> > > > -		preempt_disable();
> > > > +		migrate_disable();
> > > 
> > > I really want to avoid placing open coded migrate_disable() around 
> > > the kernel. Perhaps we should use "get_cpu_light()" here too.
> > 
> > No. get_cpu_light() and migrate_disable() are different.
> > 
> > Following your argument we would have to replace preempt_disable() 
> > with get_cpu() all over the place.
> > 
> 
> I didn't like the get_cpu_light either, but I thought it was Peter 
> that was against a 'migrate-disable()' API leaking all over the kernel.
> 
> IIRC, he gave in when it was part of a locking internal infrastructure.
> Now it's coming to what he was against. Maybe he changed his mind. 
> I'll let him speak for himself.

Ah, right, so I've mapped migrate_disable() to preempt_disable() for !
PREEMPT_RT. This to make sure people don't do stupid things with it :-)

Then again, people do lots of stupid things with preempt_disable() too, but at least we don't grow actual migrate_disable() abuse.

I think I've done one or two migrate_disable() site like the proposed in -rt already.

The thing I really want to avoid is introducing a migrate_disable() that can schedule for !rt.
[Priyanka]: In this case migrate_disable do get scheduled in case of !rt but as it mapped to preempt_disable, no change in functionality in case of !rt.

This patch is required to make sure that preemption is not disabled in case of RT to prevent the possible crash issue.
Please suggest some other better approach if not migrate_disable.
 
--
To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in the body of a message to majordomo@vger.kernel.org More majordomo info at  http://vger.kernel.org/majordomo-info.html