diff mbox series

[net-next,1/2] packet: rework packet_pick_tx_queue() to use common code selection

Message ID 1963b3df33f0a9dddd84c950cbf15773ad7252f9.1552497037.git.pabeni@redhat.com
State Changes Requested
Delegated to: David Miller
Headers show
Series net: refactor ndo_select_queue() | expand

Commit Message

Paolo Abeni March 19, 2019, 1:25 p.m. UTC
Currently packet_pick_tx_queue() is the only caller of
ndo_select_queue() using a fallback argument other than
__netdev_pick_tx.

Leveraging rx queue, we can obtain a similar queue selection
behavior using core helpers. After this change, ndo_select_queue()
is always invoked with __netdev_pick_tx() as fallback.
We can change ndo_select_queue() signature in a followup patch,
dropping an indirect call per transmitted packet in some scenarios
(e.g. TCP syn and XDP generic xmit)

This changes slightly how af packet queue selection happens when
PACKET_QDISC_BYPASS is set. It's now more similar to plan dev_queue_xmit()
tacking in account both XPS and TC mapping.

 RFC -> v1:
  - initialize sender_cpu to the expected value

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
Note: the main goal here is getting rid of the fallback() indirect
call in the device drivers implementing ndo_select_queue(). We can
obtain the same result with the INDIRECT_CALL() harness. Both ways
we need to export __netdev_pick_tx() but here we avoid the need
for additional branching.
---
 include/linux/netdevice.h |  2 ++
 net/core/dev.c            |  5 +++--
 net/packet/af_packet.c    | 15 +++++++--------
 3 files changed, 12 insertions(+), 10 deletions(-)

Comments

Eric Dumazet March 19, 2019, 1:54 p.m. UTC | #1
On 03/19/2019 06:25 AM, Paolo Abeni wrote:

> +u16 __netdev_pick_tx(struct net_device *dev, struct sk_buff *skb,
> +		     struct net_device *sb_dev)
>  {
>  	struct sock *sk = skb->sk;
>  	int queue_index = sk_tx_queue_get(sk);
> @@ -3729,6 +3729,7 @@ static u16 __netdev_pick_tx(struct net_device *dev, struct sk_buff *skb,
>  
>  	return queue_index;
>  }
> +EXPORT_SYMBOL(__netdev_pick_tx);
>
I would rather rename this function, now it is going to be used from many drivers.

Maybe the internal core-networking should be renamed as core_netdev_pick_tx()
and the 'fallback' would be named netdev_pick_tx()
David Miller March 19, 2019, 9:06 p.m. UTC | #2
From: Paolo Abeni <pabeni@redhat.com>
Date: Tue, 19 Mar 2019 14:25:01 +0100

> Currently packet_pick_tx_queue() is the only caller of
> ndo_select_queue() using a fallback argument other than
> __netdev_pick_tx.
> 
> Leveraging rx queue, we can obtain a similar queue selection
> behavior using core helpers. After this change, ndo_select_queue()
> is always invoked with __netdev_pick_tx() as fallback.
> We can change ndo_select_queue() signature in a followup patch,
> dropping an indirect call per transmitted packet in some scenarios
> (e.g. TCP syn and XDP generic xmit)
> 
> This changes slightly how af packet queue selection happens when
> PACKET_QDISC_BYPASS is set. It's now more similar to plan dev_queue_xmit()
> tacking in account both XPS and TC mapping.
> 
>  RFC -> v1:
>   - initialize sender_cpu to the expected value
> 
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

To reply indirectly to Eric's feedback.  I agree with him that maybe these
should have different names.

But having a global symbol for networking be core_*() doesn't sit
right with me.

netdev_core_*() might be better.
diff mbox series

Patch

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 26f69cf763f4..c75a6216d0f9 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2152,6 +2152,8 @@  static inline void netdev_for_each_tx_queue(struct net_device *dev,
 				  &qdisc_xmit_lock_key);	\
 }
 
+u16 __netdev_pick_tx(struct net_device *dev, struct sk_buff *skb,
+		     struct net_device *sb_dev);
 struct netdev_queue *netdev_pick_tx(struct net_device *dev,
 				    struct sk_buff *skb,
 				    struct net_device *sb_dev);
diff --git a/net/core/dev.c b/net/core/dev.c
index 2b67f2aa59dd..004d1180671a 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3704,8 +3704,8 @@  u16 dev_pick_tx_cpu_id(struct net_device *dev, struct sk_buff *skb,
 }
 EXPORT_SYMBOL(dev_pick_tx_cpu_id);
 
-static u16 __netdev_pick_tx(struct net_device *dev, struct sk_buff *skb,
-			    struct net_device *sb_dev)
+u16 __netdev_pick_tx(struct net_device *dev, struct sk_buff *skb,
+		     struct net_device *sb_dev)
 {
 	struct sock *sk = skb->sk;
 	int queue_index = sk_tx_queue_get(sk);
@@ -3729,6 +3729,7 @@  static u16 __netdev_pick_tx(struct net_device *dev, struct sk_buff *skb,
 
 	return queue_index;
 }
+EXPORT_SYMBOL(__netdev_pick_tx);
 
 struct netdev_queue *netdev_pick_tx(struct net_device *dev,
 				    struct sk_buff *skb,
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 8376bc1c1508..633d47dba542 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -275,24 +275,23 @@  static bool packet_use_direct_xmit(const struct packet_sock *po)
 	return po->xmit == packet_direct_xmit;
 }
 
-static u16 __packet_pick_tx_queue(struct net_device *dev, struct sk_buff *skb,
-				  struct net_device *sb_dev)
-{
-	return dev_pick_tx_cpu_id(dev, skb, sb_dev, NULL);
-}
-
 static u16 packet_pick_tx_queue(struct sk_buff *skb)
 {
 	struct net_device *dev = skb->dev;
 	const struct net_device_ops *ops = dev->netdev_ops;
+	int cpu = raw_smp_processor_id();
 	u16 queue_index;
 
+#ifdef CONFIG_XPS
+	skb->sender_cpu = cpu + 1;
+#endif
+	skb_record_rx_queue(skb, cpu % dev->real_num_tx_queues);
 	if (ops->ndo_select_queue) {
 		queue_index = ops->ndo_select_queue(dev, skb, NULL,
-						    __packet_pick_tx_queue);
+						    __netdev_pick_tx);
 		queue_index = netdev_cap_txqueue(dev, queue_index);
 	} else {
-		queue_index = __packet_pick_tx_queue(dev, skb, NULL);
+		queue_index = __netdev_pick_tx(dev, skb, NULL);
 	}
 
 	return queue_index;