core: Factor out flow calculation from get_rps_cpu

From: Krishna Kumar <[email protected]>

Factor out flow calculation code from get_rps_cpu, since macvtap driver can use the same code.

Revisions:

v2 - Ben: Separate flow calcuation out and use in select queue v3 - Arnd: Don't re-implement MIN

Signed-off-by Krishna Kumar <[email protected]>

---
 include/linux/netdevice.h |    1
 net/core/dev.c            |   94 ++++++++++++++++++++++--------------

 2 files changed, 59 insertions(+), 36 deletions(-)

diff -ruNp org/include/linux/netdevice.h new/include/linux/netdevice.h

include/linux/netdevice.h       2010-08-03 08:19:57.000000000 +0530 +1 -0

@@ -2253,6 +2253,7 @@ static inline const char *netdev_name(co
        return dev->name;
  }
 
+extern int skb_calculate_flow(struct net_device *dev, struct sk_buff *skb);
  extern int netdev_printk(const char *level, const struct net_device *dev,
                         const char *format, ...)
        __attribute__ ((format (printf, 3, 4)));

diff -ruNp org/net/core/dev.c new/net/core/dev.c

net/core/dev.c  2010-08-03 08:19:57.000000000 +0530 +6 -33

@@ -2263,51 +2263,24 @@ static inline void ____napi_schedule(str
        __raise_softirq_irqoff(NET_RX_SOFTIRQ);
  }
 
-#ifdef CONFIG_RPS
-
-/* One global table that all flow-based protocols share. */
-struct rps_sock_flow_table *rps_sock_flow_table __read_mostly;
-EXPORT_SYMBOL(rps_sock_flow_table);
-
  /*
- * get_rps_cpu is called from netif_receive_skb and returns the target
- * CPU from the RPS map of the receiving queue for a given skb.
- * rcu_read_lock must be held on entry.
+ * skb_calculate_flow: calculate a flow hash based on src/dst addresses
+ * and src/dst port numbers. On success, returns a hash number (> 0),
+ * otherwise -1.
  */
-static int get_rps_cpu(struct net_device *dev, struct sk_buff *skb,
-                      struct rps_dev_flow **rflowp)
+int skb_calculate_flow(struct net_device *dev, struct sk_buff *skb)
  {
+       int hash = skb->rxhash;
        struct ipv6hdr *ip6;
        struct iphdr *ip;
-       struct netdev_rx_queue *rxqueue;
-       struct rps_map *map;
-       struct rps_dev_flow_table *flow_table;
-       struct rps_sock_flow_table *sock_flow_table;
-       int cpu = -1;
        u8 ip_proto;
-       u16 tcpu;
        u32 addr1, addr2, ihl;
        union {
                u32 v32;
                u16 v16[2];
        } ports;
 
-       if (skb_rx_queue_recorded(skb)) {
-               u16 index = skb_get_rx_queue(skb);
-               if (unlikely(index >= dev->num_rx_queues)) {
-                       WARN_ONCE(dev->num_rx_queues > 1, "%s received packet "
-                               "on queue %u, but number of RX queues is %u\n",
-                               dev->name, index, dev->num_rx_queues);
-                       goto done;
-               }
-               rxqueue = dev->_rx + index;
-       } else
-               rxqueue = dev->_rx;
-
-       if (!rxqueue->rps_map && !rxqueue->rps_flow_table)
-               goto done;
-
-       if (skb->rxhash)
+       if (hash)

               goto got_hash; /* Skip hash computation on packet header */

       switch (skb->protocol) {

net/core/dev.c  2010-08-03 08:19:57.000000000 +0530 +0 -0

@@ -2334,6 +2307,7 @@ static int get_rps_cpu(struct net_device

       default:
               goto done;
       }
+
       switch (ip_proto) {
       case IPPROTO_TCP:
       case IPPROTO_UDP:

net/core/dev.c  2010-08-03 08:19:57.000000000 +0530 +4 -3

@@ -2356,11 +2330,59 @@ static int get_rps_cpu(struct net_device
        /* get a consistent hash (same value on both flow directions) */
        if (addr2 < addr1)
                swap(addr1, addr2);
-       skb->rxhash = jhash_3words(addr1, addr2, ports.v32, hashrnd);
-       if (!skb->rxhash)
-               skb->rxhash = 1;
+
+       hash = jhash_3words(addr1, addr2, ports.v32, hashrnd);
+       if (!hash)
+               hash = 1;
 

 got_hash:
+       return hash;

net/core/dev.c  2010-08-03 08:19:57.000000000 +0530 +41 -0

+
+#ifdef CONFIG_RPS
+
+/* One global table that all flow-based protocols share. */
+struct rps_sock_flow_table *rps_sock_flow_table __read_mostly;
+EXPORT_SYMBOL(rps_sock_flow_table);
+
+/*
+ * get_rps_cpu is called from netif_receive_skb and returns the target
+ * CPU from the RPS map of the receiving queue for a given skb.
+ * rcu_read_lock must be held on entry.
+ */
+static int get_rps_cpu(struct net_device *dev, struct sk_buff *skb,
+                      struct rps_dev_flow **rflowp)
+{
+       struct netdev_rx_queue *rxqueue;
+       struct rps_map *map;
+       struct rps_dev_flow_table *flow_table;
+       struct rps_sock_flow_table *sock_flow_table;
+       int cpu = -1;
+       u16 tcpu;
+
+       if (skb_rx_queue_recorded(skb)) {
+               u16 index = skb_get_rx_queue(skb);
+               if (unlikely(index >= dev->num_rx_queues)) {
+                       WARN_ONCE(dev->num_rx_queues > 1, "%s received packet "
+                               "on queue %u, but number of RX queues is %u\n",
+                               dev->name, index, dev->num_rx_queues);
+                       goto done;
+               }
+               rxqueue = dev->_rx + index;
+       } else
+               rxqueue = dev->_rx;
+
+       if (!rxqueue->rps_map && !rxqueue->rps_flow_table)
+               goto done;
+
+       skb->rxhash = skb_calculate_flow(dev, skb);
+       if (skb->rxhash < 0)
+               goto done;
+

       flow_table = rcu_dereference(rxqueue->rps_flow_table); sock_flow_table = rcu_dereference(rps_sock_flow_table); if (flow_table && sock_flow_table) {
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Changli Gao <[email protected]>
08/03/2010 09:35 AM

To

Krishna Kumar2/India/IBM@IBMIN

cc

[email protected], [email protected], [email protected], [email protected], [email protected], [email protected]

Subject

Re: [PATCH v3 1/2] core: Factor out flow calculation from get_rps_cpu

On Tue, Aug 3, 2010 at 11:02 AM, Krishna Kumar <[email protected]>

Quoted 10 lines

From: Krishna Kumar <[email protected]>

Factor out flow calculation code from get_rps_cpu, since macvtap driver can use the same code.

Revisions:

v2 - Ben: Separate flow calcuation out and use in select queue v3 - Arnd: Don't re-implement MIN

Quoted 8 lines

Signed-off-by Krishna Kumar <[email protected]>

---
 include/linux/netdevice.h |    1
 net/core/dev.c            |   94 ++++++++++++++++++++++--------------

 2 files changed, 59 insertions(+), 36 deletions(-)

diff -ruNp org/include/linux/netdevice.h new/include/linux/netdevice.h --- org/include/linux/netdevice.h       2010-08-03 08:19:57.000000000

Quoted 1 line

+++ new/include/linux/netdevice.h       2010-08-03 08:19:57.000000000

include/linux/netdevice.h static inline const char *netdev_name(co · line 2256

Quoted 5 lines

include/linux/netdevice.h +1 -0

@@ -2253,6 +2253,7 @@ static inline const char *netdev_name(co
        return dev->name;
  }
 
+extern int skb_calculate_flow(struct net_device *dev, struct sk_buff

Quoted 1 line

 extern int netdev_printk(const char *level, const struct net_device

Quoted 9 lines

                        const char *format, ...)
       __attribute__ ((format (printf, 3, 4)));
diff -ruNp org/net/core/dev.c new/net/core/dev.c

net/core/dev.c  2010-08-03 08:19:57.000000000 +0530 +0 -0

@@ -2263,51 +2263,24 @@ static inline void ____napi_schedule(str
        __raise_softirq_irqoff(NET_RX_SOFTIRQ);
  }
 

net/core/dev.c  2010-08-03 08:19:57.000000000 +0530 static inline void ____napi_schedule(str · line 2267

Quoted 11 lines

net/core/dev.c  2010-08-03 08:19:57.000000000 +0530 +1 -9

-#ifdef CONFIG_RPS
-
-/* One global table that all flow-based protocols share. */
-struct rps_sock_flow_table *rps_sock_flow_table __read_mostly;
-EXPORT_SYMBOL(rps_sock_flow_table);
-
  /*
- * get_rps_cpu is called from netif_receive_skb and returns the target
- * CPU from the RPS map of the receiving queue for a given skb.
- * rcu_read_lock must be held on entry.
+ * skb_calculate_flow: calculate a flow hash based on src/dst

Quoted 17 lines

net/core/dev.c  2010-08-03 08:19:57.000000000 +0530 +4 -8

+ * and src/dst port numbers. On success, returns a hash number (> 0),
+ * otherwise -1.
  */
-static int get_rps_cpu(struct net_device *dev, struct sk_buff *skb,
-                      struct rps_dev_flow **rflowp)
+int skb_calculate_flow(struct net_device *dev, struct sk_buff *skb)
  {
+       int hash = skb->rxhash;
        struct ipv6hdr *ip6;
        struct iphdr *ip;
-       struct netdev_rx_queue *rxqueue;
-       struct rps_map *map;
-       struct rps_dev_flow_table *flow_table;
-       struct rps_sock_flow_table *sock_flow_table;
-       int cpu = -1;
        u8 ip_proto;
-       u16 tcpu;

net/core/dev.c  2010-08-03 08:19:57.000000000 +0530 static inline void ____napi_schedule(str · line 2298

Quoted 10 lines

net/core/dev.c  2010-08-03 08:19:57.000000000 +0530 +0 -4

        u32 addr1, addr2, ihl;
        union {
                u32 v32;
                u16 v16[2];
        } ports;
 
-       if (skb_rx_queue_recorded(skb)) {
-               u16 index = skb_get_rx_queue(skb);
-               if (unlikely(index >= dev->num_rx_queues)) {
-                       WARN_ONCE(dev->num_rx_queues > 1, "%s

received packet "

net/core/dev.c  2010-08-03 08:19:57.000000000 +0530 static inline void ____napi_schedule(str · line 2299

Quoted 1 line

net/core/dev.c  2010-08-03 08:19:57.000000000 +0530 +0 -1

-                               "on queue %u, but number of RX

queues is %u\n",

Quoted 12 lines

net/core/dev.c  2010-08-03 08:19:57.000000000 +0530 +1 -11

-                               dev->name, index, dev->num_rx_queues);
-                       goto done;
-               }
-               rxqueue = dev->_rx + index;
-       } else
-               rxqueue = dev->_rx;
-
-       if (!rxqueue->rps_map && !rxqueue->rps_flow_table)
-               goto done;
-
-       if (skb->rxhash)
+       if (hash)

Quoted 1 line

               goto got_hash; /* Skip hash computation on packet header

Quoted 24 lines

       switch (skb->protocol) {

net/core/dev.c  2010-08-03 08:19:57.000000000 +0530 +0 -0

@@ -2334,6 +2307,7 @@ static int get_rps_cpu(struct net_device

       default:
               goto done;
       }
+
       switch (ip_proto) {
       case IPPROTO_TCP:
       case IPPROTO_UDP:

net/core/dev.c  2010-08-03 08:19:57.000000000 +0530 +4 -3

@@ -2356,11 +2330,59 @@ static int get_rps_cpu(struct net_device
        /* get a consistent hash (same value on both flow directions) */
        if (addr2 < addr1)
                swap(addr1, addr2);
-       skb->rxhash = jhash_3words(addr1, addr2, ports.v32, hashrnd);
-       if (!skb->rxhash)
-               skb->rxhash = 1;
+
+       hash = jhash_3words(addr1, addr2, ports.v32, hashrnd);
+       if (!hash)
+               hash = 1;
 

 got_hash:
+       return hash;

net/core/dev.c  2010-08-03 08:19:57.000000000 +0530

Quoted 5 lines

net/core/dev.c  2010-08-03 08:19:57.000000000 +0530 +5 -0

+
+done:
+       return -1;
+}
+EXPORT_SYMBOL(skb_calculate_flow);

I have noticed that you use skb_calculate_flow() in macvtap_get_queue() where skb->data doesn't point to the network header but the ethernet header. However, skb_calculate_flow() assume skb->data points to the network header. There are two choices:
* update skb_calculate_flow to support called in ethernet layer.
* pull skb before skb_calculate_flow, and push skb after
skb_calculate_flow() in macvtap_get_queue().

I prefer the former way.

BTW: the function name skb_calculate_flow isn't good. How about skb_get_rxhash(). Maybe we can implement two versions: fast path and slow path. And implement the fast path version as a inline function in skbuff.h.

static inline u32 skb_get_rxhash(struct sk_buff *skb)

{
        u32 rxhash;

        rxhash = skb->rxhash;
        if (!rxhash)
                return __skb_get_rxhash(skb);
        return rxhash;
}

Quoted 16 lines

net/core/dev.c  2010-08-03 08:19:57.000000000 +0530 +16 -0

+
+#ifdef CONFIG_RPS
+
+/* One global table that all flow-based protocols share. */
+struct rps_sock_flow_table *rps_sock_flow_table __read_mostly;
+EXPORT_SYMBOL(rps_sock_flow_table);
+
+/*
+ * get_rps_cpu is called from netif_receive_skb and returns the target
+ * CPU from the RPS map of the receiving queue for a given skb.
+ * rcu_read_lock must be held on entry.
+ */
+static int get_rps_cpu(struct net_device *dev, struct sk_buff *skb,
+                      struct rps_dev_flow **rflowp)
+{
+       struct netdev_rx_queue *rxqueue;

net/core/dev.c  2010-08-03 08:19:57.000000000 +0530

Quoted 10 lines

net/core/dev.c  2010-08-03 08:19:57.000000000 +0530 +10 -0

+       struct rps_map *map;
+       struct rps_dev_flow_table *flow_table;
+       struct rps_sock_flow_table *sock_flow_table;
+       int cpu = -1;
+       u16 tcpu;
+
+       if (skb_rx_queue_recorded(skb)) {
+               u16 index = skb_get_rx_queue(skb);
+               if (unlikely(index >= dev->num_rx_queues)) {
+                       WARN_ONCE(dev->num_rx_queues > 1, "%s

received packet "

Quoted 1 line

+                               "on queue %u, but number of RX

queues is %u\n",

Quoted 14 lines

+                               dev->name, index, dev->num_rx_queues);
+                       goto done;
+               }
+               rxqueue = dev->_rx + index;
+       } else
+               rxqueue = dev->_rx;

net/core/dev.c  2010-08-03 08:19:57.000000000 +0530 +8 -0

+
+       if (!rxqueue->rps_map && !rxqueue->rps_flow_table)
+               goto done;
+
+       skb->rxhash = skb_calculate_flow(dev, skb);
+       if (skb->rxhash < 0)
+               goto done;
+

Quoted 8 lines

       flow_table = rcu_dereference(rxqueue->rps_flow_table); sock_flow_table = rcu_dereference(rps_sock_flow_table); if (flow_table && sock_flow_table) {
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
Regards,
Changli Gao([email protected])

Hi Changli,

Good catch.

Instead of adding support for ethernet header or pull/push, I could defer the skb_push(ETH_HLEN), something like:

static int macvtap_forward(struct net_device *dev, struct sk_buff *skb) {
       struct macvtap_queue *q = macvtap_get_queue(dev, skb); if (!q)
               goto drop;

       if (skb_queue_len(&q->sk.sk_receive_queue) >= dev->tx_queue_len) goto drop;

+       skb_push(skb, ETH_HLEN);
       ...
}

and remove the same in macvtap_receive. Will this be better?

Quoted 13 lines · On Tue, Aug 3, 2010 at 1:57 PM, Krishna Kumar2 <[email protected]> wrote:

Hi Changli,

Good catch.

Instead of adding support for ethernet header or pull/push, I could defer the skb_push(ETH_HLEN), something like:

static int macvtap_forward(struct net_device *dev, struct sk_buff *skb) {
       struct macvtap_queue *q = macvtap_get_queue(dev, skb); if (!q)
               goto drop;

Quoted 9 lines

       if (skb_queue_len(&q->sk.sk_receive_queue) >= dev->tx_queue_len) goto drop;

+       skb_push(skb, ETH_HLEN);
       ...
}

and remove the same in macvtap_receive. Will this be better?

I am confused by the call sites of macvlan_dev.receive and macvlan_dev.forward. They both are possible to be called in both RX(skb->data points to network header) and TX(skb->data points to ethernet) paths. The current code in macvtap shows that macvlan_dev.receive should be called in network layer, and macvlan_dev.forward should be called in dev layer. Am I correct?

Quoted 7 lines

I am confused by the call sites of macvlan_dev.receive and macvlan_dev.forward. They both are possible to be called in both RX(skb->data points to network header) and TX(skb->data points to ethernet) paths. The current code in macvtap shows that macvlan_dev.receive should be called in network layer, and macvlan_dev.forward should be called in dev layer. Am I correct?

After checking the code carefully, I find macvlan_dev.receive is called in network layer(RX path), and macvlan_dev.forward is called in dev layer(TX path). The current code hasn't any issue. Your solution won't work, as macvtap_forward is called in dev layer, when skb->data points to ethernet header already.

Quoted 8 lines · Changli Gao

Quoted 7 lines

I am confused by the call sites of macvlan_dev.receive and macvlan_dev.forward. They both are possible to be called in both RX(skb->data points to network header) and TX(skb->data points to ethernet) paths. The current code in macvtap shows that macvlan_dev.receive should be called in network layer, and macvlan_dev.forward should be called in dev layer. Am I correct?

Quoted 5 lines

After checking the code carefully, I find macvlan_dev.receive is called in network layer(RX path), and macvlan_dev.forward is called in dev layer(TX path). The current code hasn't any issue. Your solution won't work, as macvtap_forward is called in dev layer, when skb->data points to ethernet header already.

Yes, that is correct. Forward is used for "bridge" mode, where we can send data between two macvlan/macvtap endpoints directly, as opposed to the default "vepa" mode, where all data is always sent out to the lowerdev and may be returned by an adjacent switch in hairpin configuration.