> From: David Wei <dw@xxxxxxxxxxx>
> Sent: Friday, April 5, 2024 12:23 AM
>
> On 2024-04-04 11:50, Parav Pandit wrote:
> >
> >
> >> 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?
>
> Yeah of course, thanks. I'm making some changes in netdevsim and can do a
> drive-by cleanup there too.
>
Ok. yes, that will be helpful. Thanks.
> >
> >>>
> >>>>> + 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.
