| Message ID | 171234777883.5075.17163018772262453896.stgit@anambiarhost.jf.intel.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Configuring NAPI instance for a queue | expand |
On 2024-04-05 13:09, Amritha Nambiar wrote: > Implement the netdev netlink framework functions for associating > a queue with NAPI ID. > > Signed-off-by: Amritha Nambiar <amritha.nambiar@intel.com> > --- > include/linux/netdevice.h | 7 +++ > net/core/netdev-genl.c | 117 +++++++++++++++++++++++++++++++++++++++------ > 2 files changed, 108 insertions(+), 16 deletions(-) > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > index 0c198620ac93..70df1cec4a60 100644 > --- a/include/linux/netdevice.h > +++ b/include/linux/netdevice.h > @@ -1351,6 +1351,10 @@ struct netdev_net_notifier { > * struct kernel_hwtstamp_config *kernel_config, > * struct netlink_ext_ack *extack); > * Change the hardware timestamping parameters for NIC device. > + * > + * int (*ndo_queue_set_napi)(struct net_device *dev, u32 q_idx, u32 q_type, > + * struct napi_struct *napi); > + * Change the NAPI instance associated with the queue. > */ > struct net_device_ops { > int (*ndo_init)(struct net_device *dev); > @@ -1596,6 +1600,9 @@ struct net_device_ops { > int (*ndo_hwtstamp_set)(struct net_device *dev, > struct kernel_hwtstamp_config *kernel_config, > struct netlink_ext_ack *extack); > + int (*ndo_queue_set_napi)(struct net_device *dev, > + u32 q_idx, u32 q_type, > + struct napi_struct *napi); > }; > > /** > diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c > index d5b2e90e5709..6b3d3165d76e 100644 > --- a/net/core/netdev-genl.c > +++ b/net/core/netdev-genl.c > @@ -288,12 +288,29 @@ int netdev_nl_napi_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb) > return err; > } > > +/* must be called under rtnl_lock() */ > +static struct napi_struct * > +napi_get_by_queue(struct net_device *netdev, u32 q_idx, u32 q_type) > +{ > + struct netdev_rx_queue *rxq; > + struct netdev_queue *txq; > + > + switch (q_type) { > + case NETDEV_QUEUE_TYPE_RX: > + rxq = __netif_get_rx_queue(netdev, q_idx); > + return rxq->napi; > + case NETDEV_QUEUE_TYPE_TX: > + txq = netdev_get_tx_queue(netdev, q_idx); > + return txq->napi; > + } > + return NULL; > +} > + > 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) > { > - struct netdev_rx_queue *rxq; > - struct netdev_queue *txq; > + struct napi_struct *napi; > void *hdr; > > hdr = genlmsg_iput(rsp, info); > @@ -305,19 +322,9 @@ netdev_nl_queue_fill_one(struct sk_buff *rsp, struct net_device *netdev, > nla_put_u32(rsp, NETDEV_A_QUEUE_IFINDEX, netdev->ifindex)) > goto nla_put_failure; > > - 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; > - 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; > - } > + napi = napi_get_by_queue(netdev, q_idx, q_type); > + if (napi && nla_put_u32(rsp, NETDEV_A_QUEUE_NAPI_ID, napi->napi_id)) > + goto nla_put_failure; > > genlmsg_end(rsp, hdr); > > @@ -674,9 +681,87 @@ int netdev_nl_qstats_get_dumpit(struct sk_buff *skb, > return err; > } > > +static int > +netdev_nl_queue_set_napi(struct sk_buff *rsp, struct net_device *netdev, > + u32 q_idx, u32 q_type, u32 napi_id, > + const struct genl_info *info) > +{ > + struct napi_struct *napi, *old_napi; > + int err; > + > + if (!(netdev->flags & IFF_UP)) > + return 0; Should this be an error code? > + > + err = netdev_nl_queue_validate(netdev, q_idx, q_type); > + if (err) > + return err; > + > + old_napi = napi_get_by_queue(netdev, q_idx, q_type); > + if (old_napi && old_napi->napi_id == napi_id) > + return 0; Same as above, I think this should be an error. > + > + napi = napi_by_id(napi_id); > + if (!napi) > + return -EINVAL; > + > + err = netdev->netdev_ops->ndo_queue_set_napi(netdev, q_idx, q_type, napi); > + > + return err; nit: return ndo_queue_set_napi() would save two lines. > +} > + > int netdev_nl_queue_set_doit(struct sk_buff *skb, struct genl_info *info) > { > - return -EOPNOTSUPP; > + u32 q_id, q_type, ifindex; nit: q_idx for consistency? > + struct net_device *netdev; > + struct sk_buff *rsp; > + u32 napi_id = 0; > + int err = 0; > + > + if (GENL_REQ_ATTR_CHECK(info, NETDEV_A_QUEUE_ID) || > + GENL_REQ_ATTR_CHECK(info, NETDEV_A_QUEUE_TYPE) || > + GENL_REQ_ATTR_CHECK(info, NETDEV_A_QUEUE_IFINDEX)) > + return -EINVAL; > + > + q_id = nla_get_u32(info->attrs[NETDEV_A_QUEUE_ID]); > + q_type = nla_get_u32(info->attrs[NETDEV_A_QUEUE_TYPE]); > + ifindex = nla_get_u32(info->attrs[NETDEV_A_QUEUE_IFINDEX]); > + > + if (info->attrs[NETDEV_A_QUEUE_NAPI_ID]) { > + napi_id = nla_get_u32(info->attrs[NETDEV_A_QUEUE_NAPI_ID]); > + if (napi_id < MIN_NAPI_ID) > + return -EINVAL; > + } > + > + rsp = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL); > + if (!rsp) > + return -ENOMEM; > + > + rtnl_lock(); > + > + netdev = __dev_get_by_index(genl_info_net(info), ifindex); > + if (netdev) { > + if (!napi_id) > + GENL_SET_ERR_MSG(info, "No queue parameters changed\n"); Could this be checked earlier outside of rtnl_lock? I feel like not setting a napi_id here is EINVAL. > + else > + err = netdev_nl_queue_set_napi(rsp, netdev, q_id, > + q_type, napi_id, info); > + if (!err) > + err = netdev_nl_queue_fill_one(rsp, netdev, q_id, > + q_type, info); > + } else { > + err = -ENODEV; Could be less nesty (completely untested): if (!netdev) { err = -ENODEV; goto err_rtnl_unlock; } if (!napi_id) { GENL_SET_ERR_MSG(info, "No queue parameters changed\n"); goto err_nonapi; } err = netdev_nl_queue_set_napi(rsp, netdev, q_id, q_type, napi_id, info); if (err) goto err_rtnl_unlock; err_nonapi: err = netdev_nl_queue_fill_one(rsp, netdev, q_id, q_type, info); err_rtnl_unlock: rtnl_unlock(); if (!err) return genlmsg_reply(rsp, info); err_free_msg: nlmsg_free(rsp); return err; > + } > + > + rtnl_unlock(); > + > + if (err) > + goto err_free_msg; > + > + return genlmsg_reply(rsp, info); > + > +err_free_msg: > + nlmsg_free(rsp); > + return err; > } > > static int netdev_genl_netdevice_event(struct notifier_block *nb, >
On 4/5/2024 5:20 PM, David Wei wrote: > On 2024-04-05 13:09, Amritha Nambiar wrote: >> Implement the netdev netlink framework functions for associating >> a queue with NAPI ID. >> >> Signed-off-by: Amritha Nambiar <amritha.nambiar@intel.com> >> --- >> include/linux/netdevice.h | 7 +++ >> net/core/netdev-genl.c | 117 +++++++++++++++++++++++++++++++++++++++------ >> 2 files changed, 108 insertions(+), 16 deletions(-) >> >> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h >> index 0c198620ac93..70df1cec4a60 100644 >> --- a/include/linux/netdevice.h >> +++ b/include/linux/netdevice.h >> @@ -1351,6 +1351,10 @@ struct netdev_net_notifier { >> * struct kernel_hwtstamp_config *kernel_config, >> * struct netlink_ext_ack *extack); >> * Change the hardware timestamping parameters for NIC device. >> + * >> + * int (*ndo_queue_set_napi)(struct net_device *dev, u32 q_idx, u32 q_type, >> + * struct napi_struct *napi); >> + * Change the NAPI instance associated with the queue. >> */ >> struct net_device_ops { >> int (*ndo_init)(struct net_device *dev); >> @@ -1596,6 +1600,9 @@ struct net_device_ops { >> int (*ndo_hwtstamp_set)(struct net_device *dev, >> struct kernel_hwtstamp_config *kernel_config, >> struct netlink_ext_ack *extack); >> + int (*ndo_queue_set_napi)(struct net_device *dev, >> + u32 q_idx, u32 q_type, >> + struct napi_struct *napi); >> }; >> >> /** >> diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c >> index d5b2e90e5709..6b3d3165d76e 100644 >> --- a/net/core/netdev-genl.c >> +++ b/net/core/netdev-genl.c >> @@ -288,12 +288,29 @@ int netdev_nl_napi_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb) >> return err; >> } >> >> +/* must be called under rtnl_lock() */ >> +static struct napi_struct * >> +napi_get_by_queue(struct net_device *netdev, u32 q_idx, u32 q_type) >> +{ >> + struct netdev_rx_queue *rxq; >> + struct netdev_queue *txq; >> + >> + switch (q_type) { >> + case NETDEV_QUEUE_TYPE_RX: >> + rxq = __netif_get_rx_queue(netdev, q_idx); >> + return rxq->napi; >> + case NETDEV_QUEUE_TYPE_TX: >> + txq = netdev_get_tx_queue(netdev, q_idx); >> + return txq->napi; >> + } >> + return NULL; >> +} >> + >> 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) >> { >> - struct netdev_rx_queue *rxq; >> - struct netdev_queue *txq; >> + struct napi_struct *napi; >> void *hdr; >> >> hdr = genlmsg_iput(rsp, info); >> @@ -305,19 +322,9 @@ netdev_nl_queue_fill_one(struct sk_buff *rsp, struct net_device *netdev, >> nla_put_u32(rsp, NETDEV_A_QUEUE_IFINDEX, netdev->ifindex)) >> goto nla_put_failure; >> >> - 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; >> - 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; >> - } >> + napi = napi_get_by_queue(netdev, q_idx, q_type); >> + if (napi && nla_put_u32(rsp, NETDEV_A_QUEUE_NAPI_ID, napi->napi_id)) >> + goto nla_put_failure; >> >> genlmsg_end(rsp, hdr); >> >> @@ -674,9 +681,87 @@ int netdev_nl_qstats_get_dumpit(struct sk_buff *skb, >> return err; >> } >> >> +static int >> +netdev_nl_queue_set_napi(struct sk_buff *rsp, struct net_device *netdev, >> + u32 q_idx, u32 q_type, u32 napi_id, >> + const struct genl_info *info) >> +{ >> + struct napi_struct *napi, *old_napi; >> + int err; >> + >> + if (!(netdev->flags & IFF_UP)) >> + return 0; > > Should this be an error code? Thought I can return 0 like the _get_ functions as this is a noop and not really an error. > >> + >> + err = netdev_nl_queue_validate(netdev, q_idx, q_type); >> + if (err) >> + return err; >> + >> + old_napi = napi_get_by_queue(netdev, q_idx, q_type); >> + if (old_napi && old_napi->napi_id == napi_id) >> + return 0; > > Same as above, I think this should be an error. I was thinking I can follow the ethtool semantics here, when there's no update/parameter values are not modified, it is a noop and proceed to display the current values instead of throwing error. > >> + >> + napi = napi_by_id(napi_id); >> + if (!napi) >> + return -EINVAL; >> + >> + err = netdev->netdev_ops->ndo_queue_set_napi(netdev, q_idx, q_type, napi); >> + >> + return err; > > nit: return ndo_queue_set_napi() would save two lines. Sure, will fix in the next version. > >> +} >> + >> int netdev_nl_queue_set_doit(struct sk_buff *skb, struct genl_info *info) >> { >> - return -EOPNOTSUPP; >> + u32 q_id, q_type, ifindex; > > nit: q_idx for consistency? Sure, will fix. > >> + struct net_device *netdev; >> + struct sk_buff *rsp; >> + u32 napi_id = 0; >> + int err = 0; >> + >> + if (GENL_REQ_ATTR_CHECK(info, NETDEV_A_QUEUE_ID) || >> + GENL_REQ_ATTR_CHECK(info, NETDEV_A_QUEUE_TYPE) || >> + GENL_REQ_ATTR_CHECK(info, NETDEV_A_QUEUE_IFINDEX)) >> + return -EINVAL; >> + >> + q_id = nla_get_u32(info->attrs[NETDEV_A_QUEUE_ID]); >> + q_type = nla_get_u32(info->attrs[NETDEV_A_QUEUE_TYPE]); >> + ifindex = nla_get_u32(info->attrs[NETDEV_A_QUEUE_IFINDEX]); >> + >> + if (info->attrs[NETDEV_A_QUEUE_NAPI_ID]) { >> + napi_id = nla_get_u32(info->attrs[NETDEV_A_QUEUE_NAPI_ID]); >> + if (napi_id < MIN_NAPI_ID) >> + return -EINVAL; >> + } >> + >> + rsp = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL); >> + if (!rsp) >> + return -ENOMEM; >> + >> + rtnl_lock(); >> + >> + netdev = __dev_get_by_index(genl_info_net(info), ifindex); >> + if (netdev) { >> + if (!napi_id) >> + GENL_SET_ERR_MSG(info, "No queue parameters changed\n"); > > Could this be checked earlier outside of rtnl_lock? I feel like not > setting a napi_id here is EINVAL. > I agree this part is a little ambiguous as I was trying to handle a few things here that I was not really sure of. The idea was that netdev_nl_queue_set_doit() could be the one function to handle all set params for the queue object. Today, only napi-id is supported. In future, there may be additional configurable attributes for the queue object beyond napi-id. So, it should be possible to set those params as well from netdev_nl_queue_set_doit(), which implies that, not setting napi-id is not EINVAL, as some other param could be set. If none of the configurable params are set, display the message and the current settings for the queue. So the code today handles this; if the user has given a napi-id and it is valid, then proceed to the ndo callback. If the user has not specified a napi-id to update, it is not an error case, display the current settings, same as ethtool-set params does (although it's done in the userspace in case of ethtool). Ideally, this should be handled in the userspace for netdev-genl, maybe some tricks in the YAML can achieve this for the set command to check for the presence of atleast one configurable attribute in netdev-user.c. I think, the following in YAML would make napi-id mandatory: name: napi-id checks: min: 1 But, that's not exactly what we want for set any params, what I was aiming for was to have any one attribute required from among a group of attributes. >> + else >> + err = netdev_nl_queue_set_napi(rsp, netdev, q_id, >> + q_type, napi_id, info); >> + if (!err) >> + err = netdev_nl_queue_fill_one(rsp, netdev, q_id, >> + q_type, info); >> + } else { >> + err = -ENODEV; > > Could be less nesty (completely untested): > Sure, thanks. I'll make this less nesty. > if (!netdev) { > err = -ENODEV; > goto err_rtnl_unlock; > } > > if (!napi_id) { > GENL_SET_ERR_MSG(info, "No queue parameters changed\n"); > goto err_nonapi; > } > > err = netdev_nl_queue_set_napi(rsp, netdev, q_id, > q_type, napi_id, info); > if (err) > goto err_rtnl_unlock; > > err_nonapi: > err = netdev_nl_queue_fill_one(rsp, netdev, q_id, > q_type, info); > > err_rtnl_unlock: > rtnl_unlock(); > > if (!err) > return genlmsg_reply(rsp, info); > > err_free_msg: > nlmsg_free(rsp); > return err; > >> + } >> + >> + rtnl_unlock(); >> + >> + if (err) >> + goto err_free_msg; >> + >> + return genlmsg_reply(rsp, info); >> + >> +err_free_msg: >> + nlmsg_free(rsp); >> + return err; >> } >> >> static int netdev_genl_netdevice_event(struct notifier_block *nb, >>
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index 0c198620ac93..70df1cec4a60 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -1351,6 +1351,10 @@ struct netdev_net_notifier { * struct kernel_hwtstamp_config *kernel_config, * struct netlink_ext_ack *extack); * Change the hardware timestamping parameters for NIC device. + * + * int (*ndo_queue_set_napi)(struct net_device *dev, u32 q_idx, u32 q_type, + * struct napi_struct *napi); + * Change the NAPI instance associated with the queue. */ struct net_device_ops { int (*ndo_init)(struct net_device *dev); @@ -1596,6 +1600,9 @@ struct net_device_ops { int (*ndo_hwtstamp_set)(struct net_device *dev, struct kernel_hwtstamp_config *kernel_config, struct netlink_ext_ack *extack); + int (*ndo_queue_set_napi)(struct net_device *dev, + u32 q_idx, u32 q_type, + struct napi_struct *napi); }; /** diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c index d5b2e90e5709..6b3d3165d76e 100644 --- a/net/core/netdev-genl.c +++ b/net/core/netdev-genl.c @@ -288,12 +288,29 @@ int netdev_nl_napi_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb) return err; } +/* must be called under rtnl_lock() */ +static struct napi_struct * +napi_get_by_queue(struct net_device *netdev, u32 q_idx, u32 q_type) +{ + struct netdev_rx_queue *rxq; + struct netdev_queue *txq; + + switch (q_type) { + case NETDEV_QUEUE_TYPE_RX: + rxq = __netif_get_rx_queue(netdev, q_idx); + return rxq->napi; + case NETDEV_QUEUE_TYPE_TX: + txq = netdev_get_tx_queue(netdev, q_idx); + return txq->napi; + } + return NULL; +} + 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) { - struct netdev_rx_queue *rxq; - struct netdev_queue *txq; + struct napi_struct *napi; void *hdr; hdr = genlmsg_iput(rsp, info); @@ -305,19 +322,9 @@ netdev_nl_queue_fill_one(struct sk_buff *rsp, struct net_device *netdev, nla_put_u32(rsp, NETDEV_A_QUEUE_IFINDEX, netdev->ifindex)) goto nla_put_failure; - 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; - 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; - } + napi = napi_get_by_queue(netdev, q_idx, q_type); + if (napi && nla_put_u32(rsp, NETDEV_A_QUEUE_NAPI_ID, napi->napi_id)) + goto nla_put_failure; genlmsg_end(rsp, hdr); @@ -674,9 +681,87 @@ int netdev_nl_qstats_get_dumpit(struct sk_buff *skb, return err; } +static int +netdev_nl_queue_set_napi(struct sk_buff *rsp, struct net_device *netdev, + u32 q_idx, u32 q_type, u32 napi_id, + const struct genl_info *info) +{ + struct napi_struct *napi, *old_napi; + int err; + + if (!(netdev->flags & IFF_UP)) + return 0; + + err = netdev_nl_queue_validate(netdev, q_idx, q_type); + if (err) + return err; + + old_napi = napi_get_by_queue(netdev, q_idx, q_type); + if (old_napi && old_napi->napi_id == napi_id) + return 0; + + napi = napi_by_id(napi_id); + if (!napi) + return -EINVAL; + + err = netdev->netdev_ops->ndo_queue_set_napi(netdev, q_idx, q_type, napi); + + return err; +} + int netdev_nl_queue_set_doit(struct sk_buff *skb, struct genl_info *info) { - return -EOPNOTSUPP; + u32 q_id, q_type, ifindex; + struct net_device *netdev; + struct sk_buff *rsp; + u32 napi_id = 0; + int err = 0; + + if (GENL_REQ_ATTR_CHECK(info, NETDEV_A_QUEUE_ID) || + GENL_REQ_ATTR_CHECK(info, NETDEV_A_QUEUE_TYPE) || + GENL_REQ_ATTR_CHECK(info, NETDEV_A_QUEUE_IFINDEX)) + return -EINVAL; + + q_id = nla_get_u32(info->attrs[NETDEV_A_QUEUE_ID]); + q_type = nla_get_u32(info->attrs[NETDEV_A_QUEUE_TYPE]); + ifindex = nla_get_u32(info->attrs[NETDEV_A_QUEUE_IFINDEX]); + + if (info->attrs[NETDEV_A_QUEUE_NAPI_ID]) { + napi_id = nla_get_u32(info->attrs[NETDEV_A_QUEUE_NAPI_ID]); + if (napi_id < MIN_NAPI_ID) + return -EINVAL; + } + + rsp = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL); + if (!rsp) + return -ENOMEM; + + rtnl_lock(); + + netdev = __dev_get_by_index(genl_info_net(info), ifindex); + if (netdev) { + if (!napi_id) + GENL_SET_ERR_MSG(info, "No queue parameters changed\n"); + else + err = netdev_nl_queue_set_napi(rsp, netdev, q_id, + q_type, napi_id, info); + if (!err) + err = netdev_nl_queue_fill_one(rsp, netdev, q_id, + q_type, info); + } else { + err = -ENODEV; + } + + rtnl_unlock(); + + if (err) + goto err_free_msg; + + return genlmsg_reply(rsp, info); + +err_free_msg: + nlmsg_free(rsp); + return err; } static int netdev_genl_netdevice_event(struct notifier_block *nb,
Implement the netdev netlink framework functions for associating a queue with NAPI ID. Signed-off-by: Amritha Nambiar <amritha.nambiar@intel.com> --- include/linux/netdevice.h | 7 +++ net/core/netdev-genl.c | 117 +++++++++++++++++++++++++++++++++++++++------ 2 files changed, 108 insertions(+), 16 deletions(-)