Re: [PATCH v2 1/3] eventdev/eth_rx: add params set/get APIs

[Jerin Jacob <jerinjacobk@gmail.com>]

On Thu, Feb 2, 2023 at 9:42 PM Naga Harish K, S V
<s.v.naga.harish.k@intel.com> wrote:
>
> Hi Jerin,
>
> > -----Original Message-----
> > From: Jerin Jacob <jerinjacobk@gmail.com>
> > Sent: Monday, January 30, 2023 8:13 PM
> > To: Naga Harish K, S V <s.v.naga.harish.k@intel.com>
> > Cc: jerinj@marvell.com; Carrillo, Erik G <erik.g.carrillo@intel.com>; Gujjar,
> > Abhinandan S <abhinandan.gujjar@intel.com>; dev@dpdk.org;
> > Jayatheerthan, Jay <jay.jayatheerthan@intel.com>
> > Subject: Re: [PATCH v2 1/3] eventdev/eth_rx: add params set/get APIs
> >
> > On Mon, Jan 30, 2023 at 3:26 PM Naga Harish K, S V
> > <s.v.naga.harish.k@intel.com> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Jerin Jacob <jerinjacobk@gmail.com>
> > > > Sent: Saturday, January 28, 2023 4:24 PM
> > > > To: Naga Harish K, S V <s.v.naga.harish.k@intel.com>
> > > > Cc: jerinj@marvell.com; Carrillo, Erik G
> > > > <erik.g.carrillo@intel.com>; Gujjar, Abhinandan S
> > > > <abhinandan.gujjar@intel.com>; dev@dpdk.org; Jayatheerthan, Jay
> > > > <jay.jayatheerthan@intel.com>
> > > > Subject: Re: [PATCH v2 1/3] eventdev/eth_rx: add params set/get APIs
> > > >
> > > > On Wed, Jan 25, 2023 at 10:02 PM Naga Harish K, S V
> > > > <s.v.naga.harish.k@intel.com> wrote:
> > > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Jerin Jacob <jerinjacobk@gmail.com>
> > > >
> > > > > > > > >
> > > > > > > > > > > +        */
> > > > > > > > > > > +       uint32_t rsvd[15];
> > > > > > > > > > > +       /**< Reserved fields for future use */
> > > > > > > > > >
> > > > > > > > > > Introduce rte_event_eth_rx_adapter_runtime_params_init()
> > > > > > > > > > to
> > > > > > make
> > > > > > > > > > sure rsvd is zero.
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > The reserved fields are not used by the adapter or application.
> > > > > > > > > Not sure Is it necessary to Introduce a new API to clear
> > > > > > > > > reserved
> > > > fields.
> > > > > > > >
> > > > > > > > When adapter starts using new fileds(when we add new fieds
> > > > > > > > in future), the old applicaiton which is not using
> > > > > > > > rte_event_eth_rx_adapter_runtime_params_init() may have
> > junk
> > > > > > > > value and then adapter implementation will behave bad.
> > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > > > does it mean, the application doesn't re-compile for the new DPDK?
> > > > > >
> > > > > > Yes. No need recompile if ABI not breaking.
> > > > > >
> > > > > > > When some of the reserved fields are used in the future, the
> > > > > > > application
> > > > > > also may need to be recompiled along with DPDK right?
> > > > > > > As the application also may need to use the newly consumed
> > > > > > > reserved
> > > > > > fields?
> > > > > >
> > > > > > The problematic case is:
> > > > > >
> > > > > > Adapter implementation of 23.07(Assuming there is change params)
> > > > > > field needs to work with application of 23.03.
> > > > > > rte_event_eth_rx_adapter_runtime_params_init() will sove that.
> > > > > >
> > > > >
> > > > > As rte_event_eth_rx_adapter_runtime_params_init() initializes only
> > > > reserved fields to zero,  it may not solve the issue in this case.
> > > >
> > > > rte_event_eth_rx_adapter_runtime_params_init() needs to zero all
> > > > fields, not just reserved field.
> > > > The application calling sequence  is
> > > >
> > > > struct my_config c;
> > > > rte_event_eth_rx_adapter_runtime_params_init(&c)
> > > > c.interseted_filed_to_be_updated = val;
> > > >
> > > Can it be done like
> > >         struct my_config c = {0};
> > >         c.interseted_filed_to_be_updated = val; and update Doxygen
> > > comments to recommend above usage to reset all fields?
> > > This way,  rte_event_eth_rx_adapter_runtime_params_init() can be
> > avoided.
> >
> > Better to have a function for documentation clarity. Similar scheme already
> > there in DPDK. See rte_eth_cman_config_init()
> >
> >
>
>
> The reference function rte_eth_cman_config_init() is resetting the params struct and initializing the required params with default values in the pmd cb.

No need for PMD cb.

> The proposed rte_event_eth_rx_adapter_runtime_params_init () API just needs to reset the params struct. There are no pmd CBs involved.
> Having an API just to reset the struct seems overkill. What do you think?

It is slow path API. Keeping it as function is better. Also, it helps
the documentations of config parm in
rte_event_eth_rx_adapter_runtime_params_config()
like, This structure must be initialized with
rte_event_eth_rx_adapter_runtime_params_init() or so.



>
> > >
> > > > Let me share an example and you can tell where is the issue
> > > >
> > > > 1)Assume parameter structure is 64B and for 22.03 8B are used.
> > > > 2)rte_event_eth_rx_adapter_runtime_params_init() will clear all 64B.
> > > > 3)There is an application written based on 22.03 which using only 8B
> > > > after calling rte_event_eth_rx_adapter_runtime_params_init()
> > > > 4)Assume, in 22.07 another 8B added to structure.
> > > > 5)Now, the application (3) needs to run on 22.07. Since the
> > > > application is calling
> > > > rte_event_eth_rx_adapter_runtime_params_init()
> > > > and 9 to 15B are zero, the implementation will not go bad.
> > > >
> > > > > The old application only tries to set/get previous valid fields
> > > > > and the newly
> > > > used fields may still contain junk value.
> > > > > If the application wants to make use of any the newly used params,
> > > > > the
> > > > application changes are required anyway.
> > > >
> > > > Yes. If application wants to make use of newly added features. No
> > > > need to change if new features are not needed for old application.