On Thu, Jan 27, 2022 at 3:32 AM Alexander Kozyrev <akozy...@nvidia.com> wrote: > > On Tuesday, January 25, 2022 13:44 Jerin Jacob <jerinjac...@gmail.com> wrote: > > On Tue, Jan 25, 2022 at 6:58 AM Alexander Kozyrev <akozy...@nvidia.com> > > wrote: > > > > > > On Monday, January 24, 2022 12:41 Ajit Khaparde > > <ajit.khapa...@broadcom.com> wrote: > > > > On Mon, Jan 24, 2022 at 6:37 AM Jerin Jacob <jerinjac...@gmail.com> > > > > wrote: > > > > > > > > > > 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. > > > > If we add a lot of features which has different state it will be > > difficult to manage. > > Since it is the slow path and OPTIONAL API. IMO, it should be fine to > > have a separate API for a specific purpose > > to have a clean interface. > > This approach contradicts to the DPDK way of configuring devices. > It you look at the rte_eth_dev_configure or rte_eth_rx_queue_setup API > you will see that the configuration is propagated via config structures. > I would like to conform to this approach with my new API as well.
There is a subtle difference, those are mandatory APIs. i,e application must call those API to use the subsequent APIs. I am OK with introducing rte_flow_configure() for such use cases. Probably, we can add these parameters in rte_flow_configure() for the new features. And make it mandatory API for the next ABI to avoid application breakage. Also, please change git commit to the description for adding the configure state for rte_flow API. BTW: Your Queue patch[3/3] probably needs to add the nb_queue parameter to configure. So the driver knows, the number queue needed upfront like the ethdev API scheme. > > Another question is how to deal with interdependencies with separate hints? > There could be some resources that requires other resources to be present. > Or one resource shares the hardware registers with another one and needs to > be accounted for. That is not easy to do with separate function calls. I got the use case now. > > > > > > > > > > > > > > > > > > > > > > > > + 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. > > > > In that case, we can return a "defined" return value or "defined" > > errno to capture this case so that > > the application can make forward progress to differentiate between API > > failed vs dont having enough resources > > and move on. > > I think you are right and it will be useful to provide some hardware > capabilities. > I'll add something like rte_flow_info_get() to obtain available flow rule > resources. Ack.
