[PATCH] net : To avoid execution of extra instructions in NET RX path when rps_map is not set but rps_needed is true.

[PATCH] net : To avoid execution of extra instructions in NET RX path when rps_map is not set but rps_needed is true.

[Rahul Jain]

From: Ashutosh Kaushik <k.ashutosh@samsung.com>

The patch fixes the issues with check of global flag "rps_needed" in RX Path (which process packets in TCP/IP stack like netif_rx and netif_receive_skb functions)
These functions have flag CONFIG RPS which is enabled default in kernel and to enter in RPS mode, it depends on variable rps_needed.
This variable is updated whenever value in /sys/class/net/<device>/queues/rx-0/rps_cpus is being changed.
There are 2 scenarios where it is executing extra piece of code even when results would be same every time:-

1) Suppose in system more than one networking devices are connected i.e. wired (eth0) and wireless (wlan0). 
   If I enable RPS using above method for wlan0, then it sets atomic variable in rps_needed flag which is global.
   Now,whenever traffic uses wired network, then it will execute that extra piece of code in RX path because of only dependency on global rps_needed variable as below:
  #ifdef CONFIG_RPS
        if (static_key_false(&rps_needed)) {
                struct rps_dev_flow voidflow, *rflow = &voidflow;
                int cpu = get_rps_cpu(skb->dev, skb, &rflow);

                if (cpu >= 0) {
                        ret = enqueue_to_backlog(skb, cpu, &rflow->last_qtail);
                        rcu_read_unlock();
                        return ret;
                }
        }
  #endif
 
	while every time, value returned from get_rps_cpu will be < 0 as rps_map value is not set for this network device using /sys/class/net/device/queues/rx-0/rps_cpus.
	And it will every time execute get_rps_cpu function with fail case in IF condition which will be an extra overhead for packet processing.
	
2) Another scenario is as: Suppose that we have enable RPS for wireless device say wlan0 using above specified method which will set value of rps_needed. 
   After doing test with set value of rps_cpus in sysfs, we do rmmod driver of wireless device.
   Next time again when we do insmod without rebooting system, it always hit below code with fail case:
   #ifdef CONFIG_RPS
        if (static_key_false(&rps_needed)) {
                struct rps_dev_flow voidflow, *rflow = &voidflow;
                int cpu;

                preempt_disable();
                rcu_read_lock();

                cpu = get_rps_cpu(skb->dev, skb, &rflow);
                if (cpu < 0)
                        cpu = smp_processor_id();

                ret = enqueue_to_backlog(skb, cpu, &rflow->last_qtail);

                rcu_read_unlock();
                preempt_enable();
        } else
   #endif
   The reason behind this overhead of hitting this code with false case is same because before doing rmmod, we enabled RPS which set rps_needed flag.
   Next time when we do insmod, it will just create entry for network device in sysfs with default value which is 0 for rps_cpus. It implies that RPS is disable for that device.
   But due to unchanged value of rps_needed variable, it goes into IF condition every time (which is failed in get_rps_cpu function) even rps_cpus is 0.
   Because if we do not enable RPS for that network device, rps_map is not set.
   
The patch adds a check to these two RX functions which will check RPS availability locally for device specific.
 
Signed-off-by: Ashutosh Kaushik <k.ashutosh@samsung.com>
---
 net/core/dev.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 5df6cbc..1aa4402 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3531,12 +3531,14 @@ drop:
 static int netif_rx_internal(struct sk_buff *skb)
 {
 	int ret;
+	struct netdev_rx_queue *dev_rxqueue = skb->dev->_rx;
 
 	net_timestamp_check(netdev_tstamp_prequeue, skb);
 
 	trace_netif_rx(skb);
 #ifdef CONFIG_RPS
-	if (static_key_false(&rps_needed)) {
+	if (static_key_false(&rps_needed) &&
+	    dev_rxqueue->rps_map && dev_rxqueue->rps_map->len) {
 		struct rps_dev_flow voidflow, *rflow = &voidflow;
 		int cpu;
 
@@ -3986,6 +3988,7 @@ static int __netif_receive_skb(struct sk_buff *skb)
 static int netif_receive_skb_internal(struct sk_buff *skb)
 {
 	int ret;
+	struct netdev_rx_queue *dev_rxqueue = skb->dev->_rx;
 
 	net_timestamp_check(netdev_tstamp_prequeue, skb);
 
@@ -3995,7 +3998,8 @@ static int netif_receive_skb_internal(struct sk_buff *skb)
 	rcu_read_lock();
 
 #ifdef CONFIG_RPS
-	if (static_key_false(&rps_needed)) {
+	if (static_key_false(&rps_needed) &&
+	    dev_rxqueue->rps_map && dev_rxqueue->rps_map->len) {
 		struct rps_dev_flow voidflow, *rflow = &voidflow;
 		int cpu = get_rps_cpu(skb->dev, skb, &rflow);
 
-- 
1.9.1

Re: [PATCH] net : To avoid execution of extra instructions in NET RX path when rps_map is not set but rps_needed is true.

[Jiri Pirko]

Wed, Dec 09, 2015 at 11:35:21AM CET, rahul.jain@samsung.com wrote:
>From: Ashutosh Kaushik <k.ashutosh@samsung.com>
>
>The patch fixes the issues with check of global flag "rps_needed" in RX Path (which process packets in TCP/IP stack like netif_rx and netif_receive_skb functions)

First of all, if you want people to pay attention, please have your
patch description contained within 80 cols. Thanks.

Re: [PATCH] net : To avoid execution of extra instructions in NET RX path when rps_map is not set but rps_needed is true.

[Eric Dumazet]

On Wed, 2015-12-09 at 16:05 +0530, Rahul Jain wrote:
> From: Ashutosh Kaushik <k.ashutosh@samsung.com>
> 
> The patch fixes the issues with check of global flag "rps_needed" 

Hi Rahul

I have no issue here. What is the issue exactly ?

You provide no perf numbers, so it is hard to guess what 'problem' you
intend to fix.

> The patch adds a check to these two RX functions which will check RPS availability locally for device specific.
>  
> Signed-off-by: Ashutosh Kaushik <k.ashutosh@samsung.com>
> ---
>  net/core/dev.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 5df6cbc..1aa4402 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -3531,12 +3531,14 @@ drop:
>  static int netif_rx_internal(struct sk_buff *skb)
>  {
>  	int ret;
> +	struct netdev_rx_queue *dev_rxqueue = skb->dev->_rx;
>  
>  	net_timestamp_check(netdev_tstamp_prequeue, skb);
>  

But dev_rxqueue points at this point to first RX queue on the device.

This code breaks on multiqueue device.

It also adds unnecessary overhead on configs where CONFIG_RPS is
disabled.

>  	trace_netif_rx(skb);
>  #ifdef CONFIG_RPS
> -	if (static_key_false(&rps_needed)) {
> +	if (static_key_false(&rps_needed) &&
> +	    dev_rxqueue->rps_map && dev_rxqueue->rps_map->len) {
>  		struct rps_dev_flow voidflow, *rflow = &voidflow;
>  		int cpu;


Well, you violate basic RCU requirements here. Your patch adds a
critical bug.
If we accept such patch, it will be very easy to crash the kernel. 

Hint : rps_map can be set to NULL at anytime from another CPU.

Adding code duplicating what is done later is adding kernel bloat.
It might give you some performance improvement (how much ?), but will
add few cycles per packets to others.

Its like inlining a function : It might give you some gains, but also
increase icache occupancy and might hurt others.

If really you can demonstrate more than 2-3 % performance improvement, I
would advise you to send a patch against net/Kconfig allowing to make
CONFIG_RPS independent on SMP so that you can decide to build your
kernels without RPS.

But we need a real good analysis : Maybe the issue is caused by some
false sharing and reordering fields in a structure would help.

Re: [PATCH] net : To avoid execution of extra instructions in NET RX path when rps_map is not set but rps_needed is true.

[kbuild test robot]

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

Hi Ashutosh,

[auto build test ERROR on net-next/master]
[also build test ERROR on v4.4-rc4 next-20151209]

url:    https://github.com/0day-ci/linux/commits/Rahul-Jain/net-To-avoid-execution-of-extra-instructions-in-NET-RX-path-when-rps_map-is-not-set-but-rps_needed-is-true/20151209-183932
config: m68k-m5208evb_defconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=m68k 

All errors (new ones prefixed by >>):

   net/core/dev.c: In function 'netif_rx_internal':
>> net/core/dev.c:3535:48: error: 'struct net_device' has no member named '_rx'
     struct netdev_rx_queue *dev_rxqueue = skb->dev->_rx;
                                                   ^
   net/core/dev.c:3535:26: warning: unused variable 'dev_rxqueue' [-Wunused-variable]
     struct netdev_rx_queue *dev_rxqueue = skb->dev->_rx;
                             ^
   net/core/dev.c: In function 'netif_receive_skb_internal':
   net/core/dev.c:3992:48: error: 'struct net_device' has no member named '_rx'
     struct netdev_rx_queue *dev_rxqueue = skb->dev->_rx;
                                                   ^
   net/core/dev.c:3992:26: warning: unused variable 'dev_rxqueue' [-Wunused-variable]
     struct netdev_rx_queue *dev_rxqueue = skb->dev->_rx;
                             ^

vim +3535 net/core/dev.c

  3529		return NET_RX_DROP;
  3530	}
  3531	
  3532	static int netif_rx_internal(struct sk_buff *skb)
  3533	{
  3534		int ret;
> 3535		struct netdev_rx_queue *dev_rxqueue = skb->dev->_rx;
  3536	
  3537		net_timestamp_check(netdev_tstamp_prequeue, skb);
  3538	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 7435 bytes --]