[RFC PATCH net-next v1 0/2] Threaded NAPI configurability

[RFC PATCH net-next v1 0/2] Threaded NAPI configurability

[Yannick Vignon]

From: Yannick Vignon <yannick.vignon@nxp.com>

The purpose of these 2 patches is to be able to configure the scheduling
properties (e.g. affinity, priority...) of the NAPI threads more easily
at run-time, based on the hardware queues each thread is handling.
The main goal is really to expose which thread does what, as the current
naming doesn't exactly make that clear.

Posting this as an RFC in case people have different opinions on how to
do that.

Yannick Vignon (2):
  net: add name field to napi struct
  net: stmmac: use specific name for each NAPI instance

 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 21 ++++++----
 include/linux/netdevice.h                     | 42 ++++++++++++++++++-
 net/core/dev.c                                | 20 +++++++--
 3 files changed, 69 insertions(+), 14 deletions(-)

-- 
2.17.1

[RFC PATCH net-next v1 1/2] net: add name field to napi struct

[Yannick Vignon]

From: Yannick Vignon <yannick.vignon@nxp.com>

An interesting possibility offered by the new thread NAPI code is to
fine-tune the affinities and priorities of different NAPI instances. In a
real-time networking context, this makes it possible to ensure packets
received in a high-priority queue are always processed, and with low
latency.

However, the way the NAPI threads are named does not really expose which
one is responsible for a given queue. Assigning a more explicit name to
NAPI instances can make that determination much easier.

Signed-off-by: Yannick Vignon <yannick.vignon@nxp.com>
---
 include/linux/netdevice.h | 42 +++++++++++++++++++++++++++++++++++++--
 net/core/dev.c            | 20 +++++++++++++++----
 2 files changed, 56 insertions(+), 6 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 5cbc950b34df..01fa9d342383 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -318,6 +318,8 @@ struct gro_list {
  */
 #define GRO_HASH_BUCKETS	8
 
+
+#define NAPINAMSIZ		8
 /*
  * Structure for NAPI scheduling similar to tasklet but with weighting
  */
@@ -348,6 +350,7 @@ struct napi_struct {
 	struct hlist_node	napi_hash_node;
 	unsigned int		napi_id;
 	struct task_struct	*thread;
+	char			name[NAPINAMSIZ];
 };
 
 enum {
@@ -2445,6 +2448,21 @@ static inline void *netdev_priv(const struct net_device *dev)
  */
 #define NAPI_POLL_WEIGHT 64
 
+/**
+ *	netif_napi_add_named - initialize a NAPI context
+ *	@dev:  network device
+ *	@napi: NAPI context
+ *	@poll: polling function
+ *	@weight: default weight
+ *	@name: napi instance name
+ *
+ * netif_napi_add_named() must be used to initialize a NAPI context prior to calling
+ * *any* of the other NAPI-related functions.
+ */
+void netif_napi_add_named(struct net_device *dev, struct napi_struct *napi,
+		    int (*poll)(struct napi_struct *, int), int weight,
+		    const char *name);
+
 /**
  *	netif_napi_add - initialize a NAPI context
  *	@dev:  network device
@@ -2458,6 +2476,27 @@ static inline void *netdev_priv(const struct net_device *dev)
 void netif_napi_add(struct net_device *dev, struct napi_struct *napi,
 		    int (*poll)(struct napi_struct *, int), int weight);
 
+/**
+ *	netif_tx_napi_add_named - initialize a NAPI context
+ *	@dev:  network device
+ *	@napi: NAPI context
+ *	@poll: polling function
+ *	@weight: default weight
+ *	@name: napi instance name
+ *
+ * This variant of netif_napi_add_named() should be used from drivers using NAPI
+ * to exclusively poll a TX queue.
+ * This will avoid we add it into napi_hash[], thus polluting this hash table.
+ */
+static inline void netif_tx_napi_add_named(struct net_device *dev,
+				     struct napi_struct *napi,
+				     int (*poll)(struct napi_struct *, int),
+				     int weight, const char *name)
+{
+	set_bit(NAPI_STATE_NO_BUSY_POLL, &napi->state);
+	netif_napi_add_named(dev, napi, poll, weight, name);
+}
+
 /**
  *	netif_tx_napi_add - initialize a NAPI context
  *	@dev:  network device
@@ -2474,8 +2513,7 @@ static inline void netif_tx_napi_add(struct net_device *dev,
 				     int (*poll)(struct napi_struct *, int),
 				     int weight)
 {
-	set_bit(NAPI_STATE_NO_BUSY_POLL, &napi->state);
-	netif_napi_add(dev, napi, poll, weight);
+	netif_tx_napi_add_named(dev, napi, poll, weight, NULL);
 }
 
 /**
diff --git a/net/core/dev.c b/net/core/dev.c
index 222b1d322c96..bc70f545cc5a 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1563,8 +1563,8 @@ static int napi_kthread_create(struct napi_struct *n)
 	 * TASK_INTERRUPTIBLE mode to avoid the blocked task
 	 * warning and work with loadavg.
 	 */
-	n->thread = kthread_run(napi_threaded_poll, n, "napi/%s-%d",
-				n->dev->name, n->napi_id);
+	n->thread = kthread_run(napi_threaded_poll, n, "napi/%s-%s",
+				n->dev->name, n->name);
 	if (IS_ERR(n->thread)) {
 		err = PTR_ERR(n->thread);
 		pr_err("kthread_run failed with err %d\n", err);
@@ -6844,8 +6844,9 @@ int dev_set_threaded(struct net_device *dev, bool threaded)
 }
 EXPORT_SYMBOL(dev_set_threaded);
 
-void netif_napi_add(struct net_device *dev, struct napi_struct *napi,
-		    int (*poll)(struct napi_struct *, int), int weight)
+void netif_napi_add_named(struct net_device *dev, struct napi_struct *napi,
+			  int (*poll)(struct napi_struct *, int), int weight,
+			  const char *name)
 {
 	if (WARN_ON(test_and_set_bit(NAPI_STATE_LISTED, &napi->state)))
 		return;
@@ -6871,6 +6872,10 @@ void netif_napi_add(struct net_device *dev, struct napi_struct *napi,
 	set_bit(NAPI_STATE_NPSVC, &napi->state);
 	list_add_rcu(&napi->dev_list, &dev->napi_list);
 	napi_hash_add(napi);
+	if (name)
+		strncpy(napi->name, name, NAPINAMSIZ);
+	else
+		snprintf(napi->name, NAPINAMSIZ, "%d", napi->napi_id);
 	/* Create kthread for this napi if dev->threaded is set.
 	 * Clear dev->threaded if kthread creation failed so that
 	 * threaded mode will not be enabled in napi_enable().
@@ -6878,6 +6883,13 @@ void netif_napi_add(struct net_device *dev, struct napi_struct *napi,
 	if (dev->threaded && napi_kthread_create(napi))
 		dev->threaded = 0;
 }
+EXPORT_SYMBOL(netif_napi_add_named);
+
+void netif_napi_add(struct net_device *dev, struct napi_struct *napi,
+		    int (*poll)(struct napi_struct *, int), int weight)
+{
+	netif_napi_add_named(dev, napi, poll, weight, NULL);
+}
 EXPORT_SYMBOL(netif_napi_add);
 
 void napi_disable(struct napi_struct *n)
-- 
2.17.1

Re: [RFC PATCH net-next v1 1/2] net: add name field to napi struct

[Eric Dumazet]

On Thu, May 6, 2021 at 7:20 PM Yannick Vignon
<yannick.vignon@oss.nxp.com> wrote:
>
> From: Yannick Vignon <yannick.vignon@nxp.com>
>
> An interesting possibility offered by the new thread NAPI code is to
> fine-tune the affinities and priorities of different NAPI instances. In a
> real-time networking context, this makes it possible to ensure packets
> received in a high-priority queue are always processed, and with low
> latency.
>
> However, the way the NAPI threads are named does not really expose which
> one is responsible for a given queue. Assigning a more explicit name to
> NAPI instances can make that determination much easier.
>
> Signed-off-by: Yannick Vignon <yannick.vignon@nxp.com>
> -

Having to change drivers seems a lot of work

How about exposing thread id (and napi_id eventually) in
/sys/class/net/eth0/queues/*/kthread_pid  ?

Re: [RFC PATCH net-next v1 1/2] net: add name field to napi struct

[Yannick Vignon]

On 5/6/2021 7:35 PM, Eric Dumazet wrote:
> On Thu, May 6, 2021 at 7:20 PM Yannick Vignon
> <yannick.vignon@oss.nxp.com> wrote:
>>
>> From: Yannick Vignon <yannick.vignon@nxp.com>
>>
>> An interesting possibility offered by the new thread NAPI code is to
>> fine-tune the affinities and priorities of different NAPI instances. In a
>> real-time networking context, this makes it possible to ensure packets
>> received in a high-priority queue are always processed, and with low
>> latency.
>>
>> However, the way the NAPI threads are named does not really expose which
>> one is responsible for a given queue. Assigning a more explicit name to
>> NAPI instances can make that determination much easier.
>>
>> Signed-off-by: Yannick Vignon <yannick.vignon@nxp.com>
>> -
> 
> Having to change drivers seems a lot of work
> 
> How about exposing thread id (and napi_id eventually) in
> /sys/class/net/eth0/queues/*/kthread_pid  ?
> 

This seemed like a good idea, but after looking into how to actually 
implement it, I can't find a way to "map" rx queues to napi instances. 
Am I missing something?

In the end, I'm afraid that the NAPI<->RX queue mapping is only known 
within the drivers, so we'll have no choice but to modify them
to extract that information somehow.

[RFC PATCH net-next v1 2/2] net: stmmac: use specific name for each NAPI instance

[Yannick Vignon]

From: Yannick Vignon <yannick.vignon@nxp.com>

Use the newly introduced name field of the NAPI struct to identify which
queue each NAPI instance works on.

Signed-off-by: Yannick Vignon <yannick.vignon@nxp.com>
---
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 21 ++++++++++++-------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index e0b7eebcb512..d7fc3eddb7b7 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -6615,6 +6615,7 @@ static void stmmac_napi_add(struct net_device *dev)
 {
 	struct stmmac_priv *priv = netdev_priv(dev);
 	u32 queue, maxq;
+	char name[NAPINAMSIZ];
 
 	maxq = max(priv->plat->rx_queues_to_use, priv->plat->tx_queues_to_use);
 
@@ -6626,19 +6627,23 @@ static void stmmac_napi_add(struct net_device *dev)
 		spin_lock_init(&ch->lock);
 
 		if (queue < priv->plat->rx_queues_to_use) {
-			netif_napi_add(dev, &ch->rx_napi, stmmac_napi_poll_rx,
-				       NAPI_POLL_WEIGHT);
+			snprintf(name, NAPINAMSIZ, "rx-%d", queue);
+			netif_napi_add_named(dev, &ch->rx_napi,
+					     stmmac_napi_poll_rx,
+					     NAPI_POLL_WEIGHT, name);
 		}
 		if (queue < priv->plat->tx_queues_to_use) {
-			netif_tx_napi_add(dev, &ch->tx_napi,
-					  stmmac_napi_poll_tx,
-					  NAPI_POLL_WEIGHT);
+			snprintf(name, NAPINAMSIZ, "tx-%d", queue);
+			netif_tx_napi_add_named(dev, &ch->tx_napi,
+						stmmac_napi_poll_tx,
+						NAPI_POLL_WEIGHT, name);
 		}
 		if (queue < priv->plat->rx_queues_to_use &&
 		    queue < priv->plat->tx_queues_to_use) {
-			netif_napi_add(dev, &ch->rxtx_napi,
-				       stmmac_napi_poll_rxtx,
-				       NAPI_POLL_WEIGHT);
+			snprintf(name, NAPINAMSIZ, "rxtx-%d", queue);
+			netif_napi_add_named(dev, &ch->rxtx_napi,
+					     stmmac_napi_poll_rxtx,
+					     NAPI_POLL_WEIGHT, name);
 		}
 	}
 }
-- 
2.17.1

Re: [RFC PATCH net-next v1 0/2] Threaded NAPI configurability

[Jakub Kicinski]

On Thu,  6 May 2021 19:20:19 +0200 Yannick Vignon wrote:
> The purpose of these 2 patches is to be able to configure the scheduling
> properties (e.g. affinity, priority...) of the NAPI threads more easily
> at run-time, based on the hardware queues each thread is handling.
> The main goal is really to expose which thread does what, as the current
> naming doesn't exactly make that clear.
> 
> Posting this as an RFC in case people have different opinions on how to
> do that.

WQ <-> CQ <-> irq <-> napi mapping needs an exhaustive netlink
interface. We've been saying this for a while. Neither hard coded
naming schemes nor one-off sysfs files are a great idea IMHO.

Re: [RFC PATCH net-next v1 0/2] Threaded NAPI configurability

[Yannick Vignon]

On 5/7/2021 12:18 AM, Jakub Kicinski wrote:
> On Thu,  6 May 2021 19:20:19 +0200 Yannick Vignon wrote:
>> The purpose of these 2 patches is to be able to configure the scheduling
>> properties (e.g. affinity, priority...) of the NAPI threads more easily
>> at run-time, based on the hardware queues each thread is handling.
>> The main goal is really to expose which thread does what, as the current
>> naming doesn't exactly make that clear.
>>
>> Posting this as an RFC in case people have different opinions on how to
>> do that.
> 
> WQ <-> CQ <-> irq <-> napi mapping needs an exhaustive netlink
> interface. We've been saying this for a while. Neither hard coded
> naming schemes nor one-off sysfs files are a great idea IMHO.
> 

Could you elaborate on the kind of netlink interface you are thinking about?
We already have standard ways of configuring process priorities and 
affinities, what we need is rather to expose which queue(s) each NAPI 
thread/instance is responsible for (and as I just said, I fear this will 
involve driver changes).
Now, one place were a netlink API could be of use is for statistics: we 
currently do not have any per-queue counters, and that would be useful 
when working on multi-queue setups.

Re: [RFC PATCH net-next v1 0/2] Threaded NAPI configurability

[Jakub Kicinski]

On Tue, 11 May 2021 18:46:16 +0200 Yannick Vignon wrote:
> On 5/7/2021 12:18 AM, Jakub Kicinski wrote:
> > On Thu,  6 May 2021 19:20:19 +0200 Yannick Vignon wrote:  
> >> The purpose of these 2 patches is to be able to configure the scheduling
> >> properties (e.g. affinity, priority...) of the NAPI threads more easily
> >> at run-time, based on the hardware queues each thread is handling.
> >> The main goal is really to expose which thread does what, as the current
> >> naming doesn't exactly make that clear.
> >>
> >> Posting this as an RFC in case people have different opinions on how to
> >> do that.  
> > 
> > WQ <-> CQ <-> irq <-> napi mapping needs an exhaustive netlink
> > interface. We've been saying this for a while. Neither hard coded
> > naming schemes nor one-off sysfs files are a great idea IMHO.
> 
> Could you elaborate on the kind of netlink interface you are thinking about?
> We already have standard ways of configuring process priorities and 
> affinities, what we need is rather to expose which queue(s) each NAPI 
> thread/instance is responsible for (and as I just said, I fear this will 
> involve driver changes).

An interface to carry information about the queues, interrupts and NAPI
instances, and relationship between them. 

As you noted in your reply to Eric such API would require driver
changes, but one driver using it is enough to add the API.

> Now, one place were a netlink API could be of use is for statistics: we 
> currently do not have any per-queue counters, and that would be useful 
> when working on multi-queue setups.

Yup, such API is exactly where we should add standard per queue
statistics.