RE: [PATCH v2 01/10] ethdev: introduce flow pre-configuration hints

[Alexander Kozyrev <akozyrev@nvidia.com>]

On Monday, January 24, 2022 12:41 Ajit Khaparde <ajit.khaparde@broadcom.com> wrote:
> On Mon, Jan 24, 2022 at 6:37 AM Jerin Jacob <jerinjacobk@gmail.com>
> wrote:
> >
> > On Tue, Jan 18, 2022 at 9:01 PM Alexander Kozyrev
> <akozyrev@nvidia.com> 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.
> > >
> > > Signed-off-by: Alexander Kozyrev <akozyrev@nvidia.com>
> > > ---
> >
> > >
> > > +/**
> > > + * @warning
> > > + * @b EXPERIMENTAL: this API may change without prior notice.
> > > + *
> > > + * Flow engine port configuration attributes.
> > > + */
> > > +__extension__
> >
> > Is this __extension__ required ?
No, it is not longer required as I removed bitfield from this structure. Thanks for catching.

> >
> > > +struct rte_flow_port_attr {
> > > +       /**
> > > +        * Version of the struct layout, should be 0.
> > > +        */
> > > +       uint32_t version;
> >
> > Why version number? Across DPDK, we are using dynamic function
> > versioning, I think, that would
> >  be sufficient for ABI versioning
> >
> > > +       /**
> > > +        * Number of counter actions pre-configured.
> > > +        * If set to 0, PMD will allocate counters dynamically.
> > > +        * @see RTE_FLOW_ACTION_TYPE_COUNT
> > > +        */
> > > +       uint32_t nb_counters;
> > > +       /**
> > > +        * Number of aging actions pre-configured.
> > > +        * If set to 0, PMD will allocate aging dynamically.
> > > +        * @see RTE_FLOW_ACTION_TYPE_AGE
> > > +        */
> > > +       uint32_t nb_aging;
> > > +       /**
> > > +        * Number of traffic metering actions pre-configured.
> > > +        * If set to 0, PMD will allocate meters dynamically.
> > > +        * @see RTE_FLOW_ACTION_TYPE_METER
> > > +        */
> > > +       uint32_t nb_meters;
> > > +};
> > > +
> > > +/**
> > > + * @warning
> > > + * @b EXPERIMENTAL: this API may change without prior notice.
> > > + *
> > > + * Configure flow rules module.
> > > + * To pre-allocate resources as per the flow port attributes
> > > + * this configuration function must be called before any flow rule is
> created.
> > > + * Must be called only after Ethernet device is configured, but may be
> called
> > > + * before or after the device is started as long as there are no flow rules.
> > > + * No other rte_flow function should be called while this function is
> invoked.
> > > + * This function can be called again to change the configuration.
> > > + * Some PMDs may not support re-configuration at all,
> > > + * or may only allow increasing the number of resources allocated.
> >
> > Following comment from Ivan looks good to me
> >
> > * Pre-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.
Ok, I'll adopt this wording in the v3.

> > > + *
> > > + * @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,
> >
> > Should we couple, setting resource limit hint to configure function as
> > if we add future items in
> > configuration, we may pain to manage all state. Instead how about,
> > rte_flow_resource_reserve_hint_set()?
> +1
Port attributes are the hints, PMD can safely ignore anything that is not supported/deemed unreasonable.
Having several functions to call instead of one configuration function seems like a burden to me.

> 
> >
> >
> > > +                  const struct rte_flow_port_attr *port_attr,
> > > +                  struct rte_flow_error *error);
> >
> > I think, we should have _get function to get those limit numbers otherwise,
> > we can not write portable applications as the return value is  kind of
> > boolean now if
> > don't define exact values for rte_errno for reasons.
> +1
We had this discussion in RFC. The limits will vary from NIC to NIC and from system to
system, depending on hardware capabilities and amount of free memory for example.
It is easier to reject a configuration with a clear error description as we do for flow creation.