On Wed, Feb 16, 2022 8:03 Andrew Rybchenko <andrew.rybche...@oktetlabs.ru>:
> On 2/11/22 21:47, Alexander Kozyrev wrote:
> > On Friday, February 11, 2022 5:17 Andrew Rybchenko
> <andrew.rybche...@oktetlabs.ru> wrote:
> >> Sent: Friday, February 11, 2022 5:17
> >> To: Alexander Kozyrev <akozy...@nvidia.com>; dev@dpdk.org
> >> Cc: Ori Kam <or...@nvidia.com>; NBU-Contact-Thomas Monjalon
> (EXTERNAL)
> >> <tho...@monjalon.net>; ivan.ma...@oktetlabs.ru;
> ferruh.yi...@intel.com;
> >> mohammad.abdul.a...@intel.com; qi.z.zh...@intel.com;
> jer...@marvell.com;
> >> ajit.khapa...@broadcom.com; bruce.richard...@intel.com
> >> Subject: Re: [PATCH v5 01/10] ethdev: introduce flow pre-configuration
> hints
> >>
> >> On 2/11/22 05:26, Alexander Kozyrev wrote:
> >>> The flow rules creation/destruction at a large scale incurs a performance
> >>> penalty and may negatively impact the packet processing when used
> >>> as part of the datapath logic. This is mainly because software/hardware
> >>> resources are allocated and prepared during the flow rule creation.
> >>>
> >>> In order to optimize the insertion rate, PMD may use some hints
> provided
> >>> by the application at the initialization phase. The rte_flow_configure()
> >>> function allows to pre-allocate all the needed resources beforehand.
> >>> These resources can be used at a later stage without costly allocations.
> >>> Every PMD may use only the subset of hints and ignore unused ones or
> >>> fail in case the requested configuration is not supported.
> >>>
> >>> The rte_flow_info_get() is available to retrieve the information about
> >>> supported pre-configurable resources. Both these functions must be
> called
> >>> before any other usage of the flow API engine.
> >>>
> >>> Signed-off-by: Alexander Kozyrev <akozy...@nvidia.com>
> >>> Acked-by: Ori Kam <or...@nvidia.com>
>
> [snip]
>
> >>> diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c
> >>> index a93f68abbc..66614ae29b 100644
> >>> --- a/lib/ethdev/rte_flow.c
> >>> +++ b/lib/ethdev/rte_flow.c
> >>> @@ -1391,3 +1391,43 @@ rte_flow_flex_item_release(uint16_t port_id,
> >>> ret = ops->flex_item_release(dev, handle, error);
> >>> return flow_err(port_id, ret, error);
> >>> }
> >>> +
> >>> +int
> >>> +rte_flow_info_get(uint16_t port_id,
> >>> + struct rte_flow_port_info *port_info,
> >>> + struct rte_flow_error *error)
> >>> +{
> >>> + struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> >>> + const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error);
> >>> +
> >>> + if (unlikely(!ops))
> >>> + return -rte_errno;
> >>> + if (likely(!!ops->info_get)) {
> >>
> >> expected ethdev state must be validated. Just configured?
> >>
> >>> + return flow_err(port_id,
> >>> + ops->info_get(dev, port_info, error),
> >>
> >> port_info must be checked vs NULL
> >
> > We don’t have any NULL checks for parameters in the whole ret flow API
> library.
> > See rte_flow_create() for example. attributes, pattern and actions are
> passed to PMD unchecked.
>
> IMHO it is hardly a good reason to have no such check here.
> The API is pure control path. So, it must validate all input
> arguments and it is better to do it in a generic place.
Agree, I have no objections to introduce these validation checks on control
path.
My only concern is the data-path performance, so I'm reluctant to add them to
rte_flow_q_create/destroy functions. But let's add NULL checks to configuration
routines, ok?
> >>> + error);
> >>> + }
> >>> + return rte_flow_error_set(error, ENOTSUP,
> >>> + RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> >>> + NULL, rte_strerror(ENOTSUP));
> >>> +}
> >>> +
> >>> +int
> >>> +rte_flow_configure(uint16_t port_id,
> >>> + const struct rte_flow_port_attr *port_attr,
> >>> + struct rte_flow_error *error)
> >>> +{
> >>> + struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> >>> + const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error);
> >>> +
> >>> + if (unlikely(!ops))
> >>> + return -rte_errno;
> >>> + if (likely(!!ops->configure)) {
> >>
> >> The API must validate ethdev state. configured and not started?
> > Again, we have no such validation for any rte flow API today.
>
> Same here. If documentation defines in which state the API
> should be called, generic code must guarantee it.
Ok, as long as it stays in the configuration phase only.
> >>
> >>> + return flow_err(port_id,
> >>> + ops->configure(dev, port_attr, error),
> >>
> >> port_attr must be checked vs NULL
> > Same.
> >
> >>> + error);
> >>> + }
> >>> + return rte_flow_error_set(error, ENOTSUP,
> >>> + RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> >>> + NULL, rte_strerror(ENOTSUP));
> >>> +}
> >>> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
> >>> index 1031fb246b..92be2a9a89 100644
> >>> --- a/lib/ethdev/rte_flow.h
> >>> +++ b/lib/ethdev/rte_flow.h
> >>> @@ -4853,6 +4853,114 @@ rte_flow_flex_item_release(uint16_t
> port_id,
> >>> const struct rte_flow_item_flex_handle
> >>> *handle,
> >>> struct rte_flow_error *error);
> >>>
> >>> +/**
> >>> + * @warning
> >>> + * @b EXPERIMENTAL: this API may change without prior notice.
> >>> + *
> >>> + * Information about available pre-configurable resources.
> >>> + * The zero value means a resource cannot be pre-allocated.
> >>> + *
> >>> + */
> >>> +struct rte_flow_port_info {
> >>> + /**
> >>> + * Number of pre-configurable counter actions.
> >>> + * @see RTE_FLOW_ACTION_TYPE_COUNT
> >>> + */
> >>> + uint32_t nb_counters;
> >>
> >> Name says that it is a number of counters, but description
> >> says that it is about actions.
> >> Also I don't understand what does "pre-configurable" mean.
> >> Isn't it a maximum number of available counters?
> >> If no, how can I find a maximum?
> > It is number of pre-allocated and pre-configured actions.
> > How are they pr-configured is up to PDM driver.
> > But let's change to "pre-configured" everywhere.
> > Configuration includes some memory allocation anyway.
>
> Sorry, but I still don't understand. I guess HW has
> a hard limit on a number of counters. How can I get
> the information?
Sorry for not being clear. These are resources/objects limitation.
It may be the hard HW limit on number of counter objects, for example.
Or the system has a little of memory and NIC is constrained in memory
in its attempt to create these counter objects as another example.
In any case, the info_get() API should return the limit to a user.
> >>
> >>> + /**
> >>> + * Number of pre-configurable aging flows actions.
> >>> + * @see RTE_FLOW_ACTION_TYPE_AGE
> >>> + */
> >>> + uint32_t nb_aging_flows;
> >>
> >> Same
> > Ditto.
> >
> >>> + /**
> >>> + * Number of pre-configurable traffic metering actions.
> >>> + * @see RTE_FLOW_ACTION_TYPE_METER
> >>> + */
> >>> + uint32_t nb_meters;
> >>
> >> Same
> > Ditto.
> >
> >>> +};
> >>> +
> >>> +/**
> >>> + * @warning
> >>> + * @b EXPERIMENTAL: this API may change without prior notice.
> >>> + *
> >>> + * Retrieve configuration attributes supported by the port.
> >>
> >> Description should be a bit more flow API aware.
> >> Right now it sounds too generic.
> > Ok, how about
> > "Get information about flow engine pre-configurable resources."
> >
> >>> + *
> >>> + * @param port_id
> >>> + * Port identifier of Ethernet device.
> >>> + * @param[out] port_info
> >>> + * A pointer to a structure of type *rte_flow_port_info*
> >>> + * to be filled with the contextual information of the port.
> >>> + * @param[out] error
> >>> + * Perform verbose error reporting if not NULL.
> >>> + * PMDs initialize this structure in case of error only.
> >>> + *
> >>> + * @return
> >>> + * 0 on success, a negative errno value otherwise and rte_errno is set.
> >>> + */
> >>> +__rte_experimental
> >>> +int
> >>> +rte_flow_info_get(uint16_t port_id,
> >>> + struct rte_flow_port_info *port_info,
> >>> + struct rte_flow_error *error);
> >>> +
> >>> +/**
> >>> + * @warning
> >>> + * @b EXPERIMENTAL: this API may change without prior notice.
> >>> + *
> >>> + * Resource pre-allocation and pre-configuration settings.
> >>
> >> What is the difference between pre-allocation and pre-configuration?
> >> Why are both mentioned above, but just pre-configured actions are
> >> mentioned below?
> > Please see answer to this question above.
> >
> >>> + * The zero value means on demand resource allocations only.
> >>> + *
> >>> + */
> >>> +struct rte_flow_port_attr {
> >>> + /**
> >>> + * Number of counter actions pre-configured.
> >>> + * @see RTE_FLOW_ACTION_TYPE_COUNT
> >>> + */
> >>> + uint32_t nb_counters;
> >>> + /**
> >>> + * Number of aging flows actions pre-configured.
> >>> + * @see RTE_FLOW_ACTION_TYPE_AGE
> >>> + */
> >>> + uint32_t nb_aging_flows;
> >>> + /**
> >>> + * Number of traffic metering actions pre-configured.
> >>> + * @see RTE_FLOW_ACTION_TYPE_METER
> >>> + */
> >>> + uint32_t nb_meters;
> >>> +};
> >>> +
> >>> +/**
> >>> + * @warning
> >>> + * @b EXPERIMENTAL: this API may change without prior notice.
> >>> + *
> >>> + * Configure the port's flow API engine.
> >>> + *
> >>> + * This API can only be invoked before the application
> >>> + * starts using the rest of the flow library functions.
> >>> + *
> >>> + * The API can be invoked multiple times to change the
> >>> + * settings. The port, however, may reject the changes.
> >>> + *
> >>> + * Parameters in configuration attributes must not exceed
> >>> + * numbers of resources returned by the rte_flow_info_get API.
> >>> + *
> >>> + * @param port_id
> >>> + * Port identifier of Ethernet device.
> >>> + * @param[in] port_attr
> >>> + * Port configuration attributes.
> >>> + * @param[out] error
> >>> + * Perform verbose error reporting if not NULL.
> >>> + * PMDs initialize this structure in case of error only.
> >>> + *
> >>> + * @return
> >>> + * 0 on success, a negative errno value otherwise and rte_errno is set.
> >>> + */
> >>> +__rte_experimental
> >>> +int
> >>> +rte_flow_configure(uint16_t port_id,
> >>> + const struct rte_flow_port_attr *port_attr,
> >>> + struct rte_flow_error *error);
> >>> +
> >>> #ifdef __cplusplus
> >>> }
> >>> #endif
>
> [snip]
