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

[Ajit Khaparde <ajit.khaparde@broadcom.com>]

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 ?
>
>
> > +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.
>
> > + *
> > + * @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

>
>
> > +                  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