| Message ID | 20250203191714.155526-1-jdamato@fastly.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v2] netdev-genl: Elide napi_id when not present | expand |
On 2/3/2025 11:17 AM, Joe Damato wrote: > There are at least two cases where napi_id may not present and the > napi_id should be elided: > > 1. Queues could be created, but napi_enable may not have been called > yet. In this case, there may be a NAPI but it may not have an ID and > output of a napi_id should be elided. > > 2. TX-only NAPIs currently do not have NAPI IDs. If a TX queue happens > to be linked with a TX-only NAPI, elide the NAPI ID from the netlink > output as a NAPI ID of 0 is not useful for users. > > Signed-off-by: Joe Damato <jdamato@fastly.com> Reviewed-by: Sridhar Samudrala <sridhar.samudrala@intel.com> > --- > v2: > - Updated to elide NAPI IDs for RX queues which may have not called > napi_enable yet. > > rfc: https://lore.kernel.org/lkml/20250128163038.429864-1-jdamato@fastly.com/ > net/core/netdev-genl.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c > index 715f85c6b62e..a97d3b99f6cd 100644 > --- a/net/core/netdev-genl.c > +++ b/net/core/netdev-genl.c > @@ -385,9 +385,10 @@ netdev_nl_queue_fill_one(struct sk_buff *rsp, struct net_device *netdev, > switch (q_type) { > case NETDEV_QUEUE_TYPE_RX: > rxq = __netif_get_rx_queue(netdev, q_idx); > - if (rxq->napi && nla_put_u32(rsp, NETDEV_A_QUEUE_NAPI_ID, > - rxq->napi->napi_id)) > - goto nla_put_failure; > + if (rxq->napi && rxq->napi->napi_id >= MIN_NAPI_ID) > + if (nla_put_u32(rsp, NETDEV_A_QUEUE_NAPI_ID, > + rxq->napi->napi_id)) > + goto nla_put_failure; > > binding = rxq->mp_params.mp_priv; > if (binding && > @@ -397,9 +398,10 @@ netdev_nl_queue_fill_one(struct sk_buff *rsp, struct net_device *netdev, > break; > case NETDEV_QUEUE_TYPE_TX: > txq = netdev_get_tx_queue(netdev, q_idx); > - if (txq->napi && nla_put_u32(rsp, NETDEV_A_QUEUE_NAPI_ID, > - txq->napi->napi_id)) > - goto nla_put_failure; > + if (txq->napi && txq->napi->napi_id >= MIN_NAPI_ID) > + if (nla_put_u32(rsp, NETDEV_A_QUEUE_NAPI_ID, > + txq->napi->napi_id)) > + goto nla_put_failure; > } > > genlmsg_end(rsp, hdr); > > base-commit: c2933b2befe25309f4c5cfbea0ca80909735fd76
On Mon, Feb 3, 2025 at 8:17 PM Joe Damato <jdamato@fastly.com> wrote: > > There are at least two cases where napi_id may not present and the > napi_id should be elided: > > 1. Queues could be created, but napi_enable may not have been called > yet. In this case, there may be a NAPI but it may not have an ID and > output of a napi_id should be elided. > > 2. TX-only NAPIs currently do not have NAPI IDs. If a TX queue happens > to be linked with a TX-only NAPI, elide the NAPI ID from the netlink > output as a NAPI ID of 0 is not useful for users. > > Signed-off-by: Joe Damato <jdamato@fastly.com> > --- > v2: > - Updated to elide NAPI IDs for RX queues which may have not called > napi_enable yet. > > rfc: https://lore.kernel.org/lkml/20250128163038.429864-1-jdamato@fastly.com/ > net/core/netdev-genl.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c > index 715f85c6b62e..a97d3b99f6cd 100644 > --- a/net/core/netdev-genl.c > +++ b/net/core/netdev-genl.c > @@ -385,9 +385,10 @@ netdev_nl_queue_fill_one(struct sk_buff *rsp, struct net_device *netdev, > switch (q_type) { > case NETDEV_QUEUE_TYPE_RX: > rxq = __netif_get_rx_queue(netdev, q_idx); > - if (rxq->napi && nla_put_u32(rsp, NETDEV_A_QUEUE_NAPI_ID, > - rxq->napi->napi_id)) > - goto nla_put_failure; > + if (rxq->napi && rxq->napi->napi_id >= MIN_NAPI_ID) > + if (nla_put_u32(rsp, NETDEV_A_QUEUE_NAPI_ID, > + rxq->napi->napi_id)) > + goto nla_put_failure; > > binding = rxq->mp_params.mp_priv; > if (binding && > @@ -397,9 +398,10 @@ netdev_nl_queue_fill_one(struct sk_buff *rsp, struct net_device *netdev, > break; > case NETDEV_QUEUE_TYPE_TX: > txq = netdev_get_tx_queue(netdev, q_idx); > - if (txq->napi && nla_put_u32(rsp, NETDEV_A_QUEUE_NAPI_ID, > - txq->napi->napi_id)) > - goto nla_put_failure; > + if (txq->napi && txq->napi->napi_id >= MIN_NAPI_ID) > + if (nla_put_u32(rsp, NETDEV_A_QUEUE_NAPI_ID, > + txq->napi->napi_id)) > + goto nla_put_failure; > } Hi Joe This might be time to add helpers, we now have these checks about MIN_NAPI_ID all around the places. diff --git a/include/net/busy_poll.h b/include/net/busy_poll.h index c39a426ebf52038b493b145e65e2d3d7f0c74e4c..741fa77547001aa5d966763c81540ee9210c949e 100644 --- a/include/net/busy_poll.h +++ b/include/net/busy_poll.h @@ -24,6 +24,11 @@ */ #define MIN_NAPI_ID ((unsigned int)(NR_CPUS + 1)) +static inline bool napi_id_valid(unsigned int napi_id) +{ + return napi_id >= MIN_NAPI_ID; +} + #define BUSY_POLL_BUDGET 8 #ifdef CONFIG_NET_RX_BUSY_POLL diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c index 715f85c6b62eabcc61a10065f98cc914f5f0d975..b5c4e42351e67d73c0aa879156eea431d4baf7aa 100644 --- a/net/core/netdev-genl.c +++ b/net/core/netdev-genl.c @@ -364,6 +364,13 @@ int netdev_nl_napi_set_doit(struct sk_buff *skb, struct genl_info *info) return err; } +static int nla_put_napi_id(struct sk_buff *skb, const struct napi_struct *napi) +{ + if (napi && napi_id_valid(napi->napi_id)) + return nla_put_u32(skb, NETDEV_A_QUEUE_NAPI_ID, napi->napi_id); + return 0; +} + static int netdev_nl_queue_fill_one(struct sk_buff *rsp, struct net_device *netdev, u32 q_idx, u32 q_type, const struct genl_info *info) @@ -385,8 +392,7 @@ netdev_nl_queue_fill_one(struct sk_buff *rsp, struct net_device *netdev, switch (q_type) { case NETDEV_QUEUE_TYPE_RX: rxq = __netif_get_rx_queue(netdev, q_idx); - if (rxq->napi && nla_put_u32(rsp, NETDEV_A_QUEUE_NAPI_ID, - rxq->napi->napi_id)) + if (nla_put_napi_id(rsp, rxq->napi)) goto nla_put_failure; binding = rxq->mp_params.mp_priv; @@ -397,8 +403,7 @@ netdev_nl_queue_fill_one(struct sk_buff *rsp, struct net_device *netdev, break; case NETDEV_QUEUE_TYPE_TX: txq = netdev_get_tx_queue(netdev, q_idx); - if (txq->napi && nla_put_u32(rsp, NETDEV_A_QUEUE_NAPI_ID, - txq->napi->napi_id)) + if (nla_put_napi_id(rsp, txq->napi)) goto nla_put_failure; }
On 2/4/25 6:41 AM, Eric Dumazet wrote: > On Mon, Feb 3, 2025 at 8:17 PM Joe Damato <jdamato@fastly.com> wrote: >> >> There are at least two cases where napi_id may not present and the >> napi_id should be elided: >> >> 1. Queues could be created, but napi_enable may not have been called >> yet. In this case, there may be a NAPI but it may not have an ID and >> output of a napi_id should be elided. >> >> 2. TX-only NAPIs currently do not have NAPI IDs. If a TX queue happens >> to be linked with a TX-only NAPI, elide the NAPI ID from the netlink >> output as a NAPI ID of 0 is not useful for users. >> >> Signed-off-by: Joe Damato <jdamato@fastly.com> >> --- >> v2: >> - Updated to elide NAPI IDs for RX queues which may have not called >> napi_enable yet. >> >> rfc: https://lore.kernel.org/lkml/20250128163038.429864-1-jdamato@fastly.com/ >> net/core/netdev-genl.c | 14 ++++++++------ >> 1 file changed, 8 insertions(+), 6 deletions(-) >> >> diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c >> index 715f85c6b62e..a97d3b99f6cd 100644 >> --- a/net/core/netdev-genl.c >> +++ b/net/core/netdev-genl.c >> @@ -385,9 +385,10 @@ netdev_nl_queue_fill_one(struct sk_buff *rsp, struct net_device *netdev, >> switch (q_type) { >> case NETDEV_QUEUE_TYPE_RX: >> rxq = __netif_get_rx_queue(netdev, q_idx); >> - if (rxq->napi && nla_put_u32(rsp, NETDEV_A_QUEUE_NAPI_ID, >> - rxq->napi->napi_id)) >> - goto nla_put_failure; >> + if (rxq->napi && rxq->napi->napi_id >= MIN_NAPI_ID) >> + if (nla_put_u32(rsp, NETDEV_A_QUEUE_NAPI_ID, >> + rxq->napi->napi_id)) >> + goto nla_put_failure; >> >> binding = rxq->mp_params.mp_priv; >> if (binding && >> @@ -397,9 +398,10 @@ netdev_nl_queue_fill_one(struct sk_buff *rsp, struct net_device *netdev, >> break; >> case NETDEV_QUEUE_TYPE_TX: >> txq = netdev_get_tx_queue(netdev, q_idx); >> - if (txq->napi && nla_put_u32(rsp, NETDEV_A_QUEUE_NAPI_ID, >> - txq->napi->napi_id)) >> - goto nla_put_failure; >> + if (txq->napi && txq->napi->napi_id >= MIN_NAPI_ID) >> + if (nla_put_u32(rsp, NETDEV_A_QUEUE_NAPI_ID, >> + txq->napi->napi_id)) >> + goto nla_put_failure; >> } > > Hi Joe > > This might be time to add helpers, we now have these checks about > MIN_NAPI_ID all around the places. +1. IMHO a follow-up patch could additionally replace the existing open-coded tests with the new helper. /P
On Tue, Feb 04, 2025 at 06:41:34AM +0100, Eric Dumazet wrote: > On Mon, Feb 3, 2025 at 8:17 PM Joe Damato <jdamato@fastly.com> wrote: > > > > There are at least two cases where napi_id may not present and the > > napi_id should be elided: > > > > 1. Queues could be created, but napi_enable may not have been called > > yet. In this case, there may be a NAPI but it may not have an ID and > > output of a napi_id should be elided. > > > > 2. TX-only NAPIs currently do not have NAPI IDs. If a TX queue happens > > to be linked with a TX-only NAPI, elide the NAPI ID from the netlink > > output as a NAPI ID of 0 is not useful for users. > > > > Signed-off-by: Joe Damato <jdamato@fastly.com> > > --- > > v2: > > - Updated to elide NAPI IDs for RX queues which may have not called > > napi_enable yet. > > > > rfc: https://lore.kernel.org/lkml/20250128163038.429864-1-jdamato@fastly.com/ > > net/core/netdev-genl.c | 14 ++++++++------ > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c > > index 715f85c6b62e..a97d3b99f6cd 100644 > > --- a/net/core/netdev-genl.c > > +++ b/net/core/netdev-genl.c > > @@ -385,9 +385,10 @@ netdev_nl_queue_fill_one(struct sk_buff *rsp, struct net_device *netdev, > > switch (q_type) { > > case NETDEV_QUEUE_TYPE_RX: > > rxq = __netif_get_rx_queue(netdev, q_idx); > > - if (rxq->napi && nla_put_u32(rsp, NETDEV_A_QUEUE_NAPI_ID, > > - rxq->napi->napi_id)) > > - goto nla_put_failure; > > + if (rxq->napi && rxq->napi->napi_id >= MIN_NAPI_ID) > > + if (nla_put_u32(rsp, NETDEV_A_QUEUE_NAPI_ID, > > + rxq->napi->napi_id)) > > + goto nla_put_failure; > > > > binding = rxq->mp_params.mp_priv; > > if (binding && > > @@ -397,9 +398,10 @@ netdev_nl_queue_fill_one(struct sk_buff *rsp, struct net_device *netdev, > > break; > > case NETDEV_QUEUE_TYPE_TX: > > txq = netdev_get_tx_queue(netdev, q_idx); > > - if (txq->napi && nla_put_u32(rsp, NETDEV_A_QUEUE_NAPI_ID, > > - txq->napi->napi_id)) > > - goto nla_put_failure; > > + if (txq->napi && txq->napi->napi_id >= MIN_NAPI_ID) > > + if (nla_put_u32(rsp, NETDEV_A_QUEUE_NAPI_ID, > > + txq->napi->napi_id)) > > + goto nla_put_failure; > > } > > Hi Joe > > This might be time to add helpers, we now have these checks about > MIN_NAPI_ID all around the places. Thanks that's a good idea. I'll add that for the v3.
On Tue, Feb 04, 2025 at 06:41:34AM +0100, Eric Dumazet wrote: > On Mon, Feb 3, 2025 at 8:17 PM Joe Damato <jdamato@fastly.com> wrote: > > > > There are at least two cases where napi_id may not present and the > > napi_id should be elided: > > > > 1. Queues could be created, but napi_enable may not have been called > > yet. In this case, there may be a NAPI but it may not have an ID and > > output of a napi_id should be elided. > > > > 2. TX-only NAPIs currently do not have NAPI IDs. If a TX queue happens > > to be linked with a TX-only NAPI, elide the NAPI ID from the netlink > > output as a NAPI ID of 0 is not useful for users. > > > > Signed-off-by: Joe Damato <jdamato@fastly.com> > > --- > > v2: > > - Updated to elide NAPI IDs for RX queues which may have not called > > napi_enable yet. > > > > rfc: https://lore.kernel.org/lkml/20250128163038.429864-1-jdamato@fastly.com/ > > net/core/netdev-genl.c | 14 ++++++++------ > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c > > index 715f85c6b62e..a97d3b99f6cd 100644 > > --- a/net/core/netdev-genl.c > > +++ b/net/core/netdev-genl.c > > @@ -385,9 +385,10 @@ netdev_nl_queue_fill_one(struct sk_buff *rsp, struct net_device *netdev, > > switch (q_type) { > > case NETDEV_QUEUE_TYPE_RX: > > rxq = __netif_get_rx_queue(netdev, q_idx); > > - if (rxq->napi && nla_put_u32(rsp, NETDEV_A_QUEUE_NAPI_ID, > > - rxq->napi->napi_id)) > > - goto nla_put_failure; > > + if (rxq->napi && rxq->napi->napi_id >= MIN_NAPI_ID) > > + if (nla_put_u32(rsp, NETDEV_A_QUEUE_NAPI_ID, > > + rxq->napi->napi_id)) > > + goto nla_put_failure; > > > > binding = rxq->mp_params.mp_priv; > > if (binding && > > @@ -397,9 +398,10 @@ netdev_nl_queue_fill_one(struct sk_buff *rsp, struct net_device *netdev, > > break; > > case NETDEV_QUEUE_TYPE_TX: > > txq = netdev_get_tx_queue(netdev, q_idx); > > - if (txq->napi && nla_put_u32(rsp, NETDEV_A_QUEUE_NAPI_ID, > > - txq->napi->napi_id)) > > - goto nla_put_failure; > > + if (txq->napi && txq->napi->napi_id >= MIN_NAPI_ID) > > + if (nla_put_u32(rsp, NETDEV_A_QUEUE_NAPI_ID, > > + txq->napi->napi_id)) > > + goto nla_put_failure; > > } > > Hi Joe > > This might be time to add helpers, we now have these checks about > MIN_NAPI_ID all around the places. I'm not sure what the right etiquette is; I was thinking of just taking the patch you proposed below and submitting it with you as the author with my Reviewed-by. Is that OK and if so, are you OK with the commit message?
On Tue, Feb 4, 2025 at 7:28 PM Joe Damato <jdamato@fastly.com> wrote: > > On Tue, Feb 04, 2025 at 06:41:34AM +0100, Eric Dumazet wrote: > > On Mon, Feb 3, 2025 at 8:17 PM Joe Damato <jdamato@fastly.com> wrote: > > > > > > There are at least two cases where napi_id may not present and the > > > napi_id should be elided: > > > > > > 1. Queues could be created, but napi_enable may not have been called > > > yet. In this case, there may be a NAPI but it may not have an ID and > > > output of a napi_id should be elided. > > > > > > 2. TX-only NAPIs currently do not have NAPI IDs. If a TX queue happens > > > to be linked with a TX-only NAPI, elide the NAPI ID from the netlink > > > output as a NAPI ID of 0 is not useful for users. > > > > > > Signed-off-by: Joe Damato <jdamato@fastly.com> > > > --- > > > v2: > > > - Updated to elide NAPI IDs for RX queues which may have not called > > > napi_enable yet. > > > > > > rfc: https://lore.kernel.org/lkml/20250128163038.429864-1-jdamato@fastly.com/ > > > net/core/netdev-genl.c | 14 ++++++++------ > > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > > > diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c > > > index 715f85c6b62e..a97d3b99f6cd 100644 > > > --- a/net/core/netdev-genl.c > > > +++ b/net/core/netdev-genl.c > > > @@ -385,9 +385,10 @@ netdev_nl_queue_fill_one(struct sk_buff *rsp, struct net_device *netdev, > > > switch (q_type) { > > > case NETDEV_QUEUE_TYPE_RX: > > > rxq = __netif_get_rx_queue(netdev, q_idx); > > > - if (rxq->napi && nla_put_u32(rsp, NETDEV_A_QUEUE_NAPI_ID, > > > - rxq->napi->napi_id)) > > > - goto nla_put_failure; > > > + if (rxq->napi && rxq->napi->napi_id >= MIN_NAPI_ID) > > > + if (nla_put_u32(rsp, NETDEV_A_QUEUE_NAPI_ID, > > > + rxq->napi->napi_id)) > > > + goto nla_put_failure; > > > > > > binding = rxq->mp_params.mp_priv; > > > if (binding && > > > @@ -397,9 +398,10 @@ netdev_nl_queue_fill_one(struct sk_buff *rsp, struct net_device *netdev, > > > break; > > > case NETDEV_QUEUE_TYPE_TX: > > > txq = netdev_get_tx_queue(netdev, q_idx); > > > - if (txq->napi && nla_put_u32(rsp, NETDEV_A_QUEUE_NAPI_ID, > > > - txq->napi->napi_id)) > > > - goto nla_put_failure; > > > + if (txq->napi && txq->napi->napi_id >= MIN_NAPI_ID) > > > + if (nla_put_u32(rsp, NETDEV_A_QUEUE_NAPI_ID, > > > + txq->napi->napi_id)) > > > + goto nla_put_failure; > > > } > > > > Hi Joe > > > > This might be time to add helpers, we now have these checks about > > MIN_NAPI_ID all around the places. > > I'm not sure what the right etiquette is; I was thinking of just > taking the patch you proposed below and submitting it with you as > the author with my Reviewed-by. > > Is that OK and if so, are you OK with the commit message? Oh no worries, please grab the patch completely, I will add my usual 'Reviewed-by' tag. Thanks
On Tue, Feb 04, 2025 at 07:53:51PM +0100, Eric Dumazet wrote: > On Tue, Feb 4, 2025 at 7:28 PM Joe Damato <jdamato@fastly.com> wrote: > > > > On Tue, Feb 04, 2025 at 06:41:34AM +0100, Eric Dumazet wrote: > > > On Mon, Feb 3, 2025 at 8:17 PM Joe Damato <jdamato@fastly.com> wrote: [...] > > > > > > Hi Joe > > > > > > This might be time to add helpers, we now have these checks about > > > MIN_NAPI_ID all around the places. > > > > I'm not sure what the right etiquette is; I was thinking of just > > taking the patch you proposed below and submitting it with you as > > the author with my Reviewed-by. > > > > Is that OK and if so, are you OK with the commit message? > > Oh no worries, please grab the patch completely, I will add my usual > 'Reviewed-by' tag. OK, will do. Thanks for the guidance.
diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c index 715f85c6b62e..a97d3b99f6cd 100644 --- a/net/core/netdev-genl.c +++ b/net/core/netdev-genl.c @@ -385,9 +385,10 @@ netdev_nl_queue_fill_one(struct sk_buff *rsp, struct net_device *netdev, switch (q_type) { case NETDEV_QUEUE_TYPE_RX: rxq = __netif_get_rx_queue(netdev, q_idx); - if (rxq->napi && nla_put_u32(rsp, NETDEV_A_QUEUE_NAPI_ID, - rxq->napi->napi_id)) - goto nla_put_failure; + if (rxq->napi && rxq->napi->napi_id >= MIN_NAPI_ID) + if (nla_put_u32(rsp, NETDEV_A_QUEUE_NAPI_ID, + rxq->napi->napi_id)) + goto nla_put_failure; binding = rxq->mp_params.mp_priv; if (binding && @@ -397,9 +398,10 @@ netdev_nl_queue_fill_one(struct sk_buff *rsp, struct net_device *netdev, break; case NETDEV_QUEUE_TYPE_TX: txq = netdev_get_tx_queue(netdev, q_idx); - if (txq->napi && nla_put_u32(rsp, NETDEV_A_QUEUE_NAPI_ID, - txq->napi->napi_id)) - goto nla_put_failure; + if (txq->napi && txq->napi->napi_id >= MIN_NAPI_ID) + if (nla_put_u32(rsp, NETDEV_A_QUEUE_NAPI_ID, + txq->napi->napi_id)) + goto nla_put_failure; } genlmsg_end(rsp, hdr);
There are at least two cases where napi_id may not present and the napi_id should be elided: 1. Queues could be created, but napi_enable may not have been called yet. In this case, there may be a NAPI but it may not have an ID and output of a napi_id should be elided. 2. TX-only NAPIs currently do not have NAPI IDs. If a TX queue happens to be linked with a TX-only NAPI, elide the NAPI ID from the netlink output as a NAPI ID of 0 is not useful for users. Signed-off-by: Joe Damato <jdamato@fastly.com> --- v2: - Updated to elide NAPI IDs for RX queues which may have not called napi_enable yet. rfc: https://lore.kernel.org/lkml/20250128163038.429864-1-jdamato@fastly.com/ net/core/netdev-genl.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) base-commit: c2933b2befe25309f4c5cfbea0ca80909735fd76