> From: David Wei <dw@xxxxxxxxxxx>
> Sent: Friday, April 5, 2024 12:10 AM
>
> On 2024-04-04 10:58, Parav Pandit wrote:
> > Hi David,
> >
> >> From: David Wei <dw@xxxxxxxxxxx>
> >> Sent: Thursday, April 4, 2024 10:29 PM
> >>
> >> On 2024-04-03 10:41, Parav Pandit wrote:
> >>> Many devices send event notifications for the IO queues, such as tx
> >>> and rx queues, through event queues.
> >>>
> >>> Enable a privileged owner, such as a hypervisor PF, to set the
> >>> number of IO event queues for the VF and SF during the provisioning
> stage.
> >>>
> >>> example:
> >>> Get maximum IO event queues of the VF device::
> >>>
> >>> $ devlink port show pci/0000:06:00.0/2
> >>> pci/0000:06:00.0/2: type eth netdev enp6s0pf0vf1 flavour pcivf
> >>> pfnum 0
> >> vfnum 1
> >>> function:
> >>> hw_addr 00:00:00:00:00:00 ipsec_packet disabled max_io_eqs
> >>> 10
> >>>
> >>> Set maximum IO event queues of the VF device::
> >>>
> >>> $ devlink port function set pci/0000:06:00.0/2 max_io_eqs 32
> >>>
> >>> $ devlink port show pci/0000:06:00.0/2
> >>> pci/0000:06:00.0/2: type eth netdev enp6s0pf0vf1 flavour pcivf
> >>> pfnum 0
> >> vfnum 1
> >>> function:
> >>> hw_addr 00:00:00:00:00:00 ipsec_packet disabled max_io_eqs
> >>> 32
> >>>
> >>> Reviewed-by: Jiri Pirko <jiri@xxxxxxxxxx>
> >>> Reviewed-by: Shay Drory <shayd@xxxxxxxxxx>
> >>> Signed-off-by: Parav Pandit <parav@xxxxxxxxxx>
> >>> ---
> >>> changelog:
> >>> v2->v3:
> >>> - limited 80 chars per line
> >>> v1->v2:
> >>> - limited comment to 80 chars per line in header file
> >>> ---
> >>> .../networking/devlink/devlink-port.rst | 25 +++++++++
> >>> include/net/devlink.h | 14 +++++
> >>> include/uapi/linux/devlink.h | 1 +
> >>> net/devlink/port.c | 53 +++++++++++++++++++
> >>> 4 files changed, 93 insertions(+)
> >>>
> >>> diff --git a/Documentation/networking/devlink/devlink-port.rst
> >>> b/Documentation/networking/devlink/devlink-port.rst
> >>> index 562f46b41274..451f57393f11 100644
> >>> --- a/Documentation/networking/devlink/devlink-port.rst
> >>> +++ b/Documentation/networking/devlink/devlink-port.rst
> >>> @@ -134,6 +134,9 @@ Users may also set the IPsec crypto capability
> >>> of the function using Users may also set the IPsec packet
> >>> capability of the function using `devlink port function set ipsec_packet`
> command.
> >>>
> >>> +Users may also set the maximum IO event queues of the function
> >>> +using `devlink port function set max_io_eqs` command.
> >>> +
> >>> Function attributes
> >>> ===================
> >>>
> >>> @@ -295,6 +298,28 @@ policy is processed in software by the kernel.
> >>> function:
> >>> hw_addr 00:00:00:00:00:00 ipsec_packet enabled
> >>>
> >>> +Maximum IO events queues setup
> >>> +------------------------------
> >>> +When user sets maximum number of IO event queues for a SF or a VF,
> >>> +such function driver is limited to consume only enforced number of
> >>> +IO event queues.
> >>> +
> >>> +- Get maximum IO event queues of the VF device::
> >>> +
> >>> + $ devlink port show pci/0000:06:00.0/2
> >>> + pci/0000:06:00.0/2: type eth netdev enp6s0pf0vf1 flavour pcivf
> >>> + pfnum
> >> 0 vfnum 1
> >>> + function:
> >>> + hw_addr 00:00:00:00:00:00 ipsec_packet disabled
> >>> + max_io_eqs 10
> >>> +
> >>> +- Set maximum IO event queues of the VF device::
> >>> +
> >>> + $ devlink port function set pci/0000:06:00.0/2 max_io_eqs 32
> >>> +
> >>> + $ devlink port show pci/0000:06:00.0/2
> >>> + pci/0000:06:00.0/2: type eth netdev enp6s0pf0vf1 flavour pcivf
> >>> + pfnum
> >> 0 vfnum 1
> >>> + function:
> >>> + hw_addr 00:00:00:00:00:00 ipsec_packet disabled
> >>> + max_io_eqs 32
> >>> +
> >>> Subfunction
> >>> ============
> >>>
> >>> diff --git a/include/net/devlink.h b/include/net/devlink.h index
> >>> 9ac394bdfbe4..bb1af599d101 100644
> >>> --- a/include/net/devlink.h
> >>> +++ b/include/net/devlink.h
> >>> @@ -1602,6 +1602,14 @@ void devlink_free(struct devlink *devlink);
> >>> * capability. Should be used by device drivers to
> >>> * enable/disable ipsec_packet capability of a
> >>> * function managed by the devlink port.
> >>> + * @port_fn_max_io_eqs_get: Callback used to get port function's
> >> maximum number
> >>> + * of event queues. Should be used by device drivers
> >> to
> >>> + * report the maximum event queues of a function
> >>> + * managed by the devlink port.
> >>> + * @port_fn_max_io_eqs_set: Callback used to set port function's
> >> maximum number
> >>> + * of event queues. Should be used by device drivers
> >> to
> >>> + * configure maximum number of event queues
> >>> + * of a function managed by the devlink port.
> >>> *
> >>> * Note: Driver should return -EOPNOTSUPP if it doesn't support
> >>> * port function (@port_fn_*) handling for a particular port.
> >>> @@ -1651,6 +1659,12 @@ struct devlink_port_ops {
> >>> int (*port_fn_ipsec_packet_set)(struct devlink_port *devlink_port,
> >>> bool enable,
> >>> struct netlink_ext_ack *extack);
> >>> + int (*port_fn_max_io_eqs_get)(struct devlink_port *devlink_port,
> >>> + u32 *max_eqs,
> >>> + struct netlink_ext_ack *extack);
> >>> + int (*port_fn_max_io_eqs_set)(struct devlink_port *devlink_port,
> >>> + u32 max_eqs,
> >>> + struct netlink_ext_ack *extack);
> >>> };
> >>>
> >>> void devlink_port_init(struct devlink *devlink, diff --git
> >>> a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h index
> >>> 2da0c7eb6710..9401aa343673 100644
> >>> --- a/include/uapi/linux/devlink.h
> >>> +++ b/include/uapi/linux/devlink.h
> >>> @@ -686,6 +686,7 @@ enum devlink_port_function_attr {
> >>> DEVLINK_PORT_FN_ATTR_OPSTATE, /* u8 */
> >>> DEVLINK_PORT_FN_ATTR_CAPS, /* bitfield32 */
> >>> DEVLINK_PORT_FN_ATTR_DEVLINK, /* nested */
> >>> + DEVLINK_PORT_FN_ATTR_MAX_IO_EQS, /* u32 */
> >>>
> >>> __DEVLINK_PORT_FUNCTION_ATTR_MAX,
> >>> DEVLINK_PORT_FUNCTION_ATTR_MAX =
> >> __DEVLINK_PORT_FUNCTION_ATTR_MAX -
> >>> 1 diff --git a/net/devlink/port.c b/net/devlink/port.c index
> >>> 118d130d2afd..be9158b4453c 100644
> >>> --- a/net/devlink/port.c
> >>> +++ b/net/devlink/port.c
> >>> @@ -16,6 +16,7 @@ static const struct nla_policy
> >> devlink_function_nl_policy[DEVLINK_PORT_FUNCTION_
> >>> DEVLINK_PORT_FN_STATE_ACTIVE),
> >>> [DEVLINK_PORT_FN_ATTR_CAPS] =
> >>>
> >> NLA_POLICY_BITFIELD32(DEVLINK_PORT_FN_CAPS_VALID_MASK),
> >>> + [DEVLINK_PORT_FN_ATTR_MAX_IO_EQS] = { .type = NLA_U32 },
> >>> };
> >>>
> >>> #define ASSERT_DEVLINK_PORT_REGISTERED(devlink_port)
> >> \
> >>> @@ -182,6 +183,30 @@ static int devlink_port_fn_caps_fill(struct
> >> devlink_port *devlink_port,
> >>> return 0;
> >>> }
> >>>
> >>> +static int devlink_port_fn_max_io_eqs_fill(struct devlink_port *port,
> >>> + struct sk_buff *msg,
> >>> + struct netlink_ext_ack *extack,
> >>> + bool *msg_updated)
> >>> +{
> >>> + u32 max_io_eqs;
> >>> + int err;
> >>> +
> >>> + if (!port->ops->port_fn_max_io_eqs_get)
> >>> + return 0;
> >>> +
> >>> + err = port->ops->port_fn_max_io_eqs_get(port, &max_io_eqs,
> >> extack);
> >>> + if (err) {
> >>> + if (err == -EOPNOTSUPP)
> >>> + return 0;
> >>
> >> Docs above says:
> >> * Note: Driver should return -EOPNOTSUPP if it doesn't support
> >> * port function (@port_fn_*) handling for a particular port.
> >>
> >> But here you're returning 0 in both cases of no
> >> port_fn_max_io_eqs_get or
> >> port_fn_max_io_eqs_get() returns EOPNOTSUPP.
> >>
> > When the port does not support this op, the function pointer is null and, 0
> is returned as expected.
> >
> > When the port for some reason has the ops function pointer set for a port,
> but if the port does not support the ops, it will return ENOPNOTSUPP.
> > This may be possible when the driver has chosen to use same ops callback
> structure for multiple port flavors.
> >
> > This code pattern is likely left over code of relatively recent work that
> moved ops from devlink instance to per port ops.
> > I propose to keep the current check as done in this patch, and run a
> > full audit of all the drivers, if all drivers have moved to per port ops, then
> simplify the code to drop the check for EOPNOTSUPP in a new series that
> may touch more drivers.
> > Otherwise, we may end up failing the port show operation when it returns
> - ENOPNOTSUPP.
>
> Thanks for the explanation. So ideally each port flavour has its own unique
> set of struct ops, and if something is not supported then don't set the func
> ptr in the struct ops.
>
> Yes, I see that 0 has to be returned for devlink_port_fn_caps_fill() to succeed.
>
> Had a brief look and there's only a handful of drivers (mlx, nfp, ice) that use
> devlink_port_ops.
>
And netdevsim too. :)
Can we please the cleanup in a separate series?
Post this may be later in the month, I may have cycles to audit and simplify.
Would it be ok with you?
> >
> >>> + return err;
> >>> + }
> >>> + err = nla_put_u32(msg, DEVLINK_PORT_FN_ATTR_MAX_IO_EQS,
> >> max_io_eqs);
> >>> + if (err)
> >>> + return err;
> >>> + *msg_updated = true;
> >>> + return 0;
> >>> +}
> >>> +
> >>> int devlink_nl_port_handle_fill(struct sk_buff *msg, struct
> >>> devlink_port *devlink_port) {
> >>> if (devlink_nl_put_handle(msg, devlink_port->devlink)) @@ -409,6
> >>> +434,18 @@ static int devlink_port_fn_caps_set(struct devlink_port
> >> *devlink_port,
> >>> return 0;
> >>> }
> >>>
> >>> +static int
> >>> +devlink_port_fn_max_io_eqs_set(struct devlink_port *devlink_port,
> >>> + const struct nlattr *attr,
> >>> + struct netlink_ext_ack *extack) {
> >>> + u32 max_io_eqs;
> >>> +
> >>> + max_io_eqs = nla_get_u32(attr);
> >>> + return devlink_port->ops->port_fn_max_io_eqs_set(devlink_port,
> >>> + max_io_eqs, extack);
> >>> +}
> >>> +
> >>> static int
> >>> devlink_nl_port_function_attrs_put(struct sk_buff *msg, struct
> >> devlink_port *port,
> >>> struct netlink_ext_ack *extack) @@ -428,6
> >> +465,9 @@
> >>> devlink_nl_port_function_attrs_put(struct sk_buff *msg, struct
> >> devlink_port *por
> >>> if (err)
> >>> goto out;
> >>> err = devlink_port_fn_state_fill(port, msg, extack, &msg_updated);
> >>> + if (err)
> >>> + goto out;
> >>> + err = devlink_port_fn_max_io_eqs_fill(port, msg, extack,
> >>> +&msg_updated);
> >>> if (err)
> >>> goto out;
> >>> err = devlink_rel_devlink_handle_put(msg, port->devlink, @@ -726,6
> >>> +766,12 @@ static int devlink_port_function_validate(struct
> >>> +devlink_port
> >> *devlink_port,
> >>> }
> >>> }
> >>> }
> >>> + if (tb[DEVLINK_PORT_FN_ATTR_MAX_IO_EQS] &&
> >>> + !ops->port_fn_max_io_eqs_set) {
> >>> + NL_SET_ERR_MSG_ATTR(extack,
> >> tb[DEVLINK_PORT_FN_ATTR_MAX_IO_EQS],
> >>> + "Function does not support max_io_eqs
> >> setting");
> >>> + return -EOPNOTSUPP;
> >>> + }
> >>> return 0;
> >>> }
> >>>
> >>> @@ -761,6 +807,13 @@ static int devlink_port_function_set(struct
> >> devlink_port *port,
> >>> return err;
> >>> }
> >>>
> >>> + attr = tb[DEVLINK_PORT_FN_ATTR_MAX_IO_EQS];
> >>> + if (attr) {
> >>> + err = devlink_port_fn_max_io_eqs_set(port, attr, extack);
> >>> + if (err)
> >>> + return err;
> >>> + }
> >>> +
> >>> /* Keep this as the last function attribute set, so that when
> >>> * multiple port function attributes are set along with state,
> >>> * Those can be applied first before activating the state.
