Re: [PATCH v4 1/3] lib: introduce dispatcher library

[Jerin Jacob <jerinjacobk@gmail.com>]

On Wed, Sep 27, 2023 at 1:43 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Tue, Sep 26, 2023 at 11:58:37PM +0530, Jerin Jacob wrote:
> > On Mon, Sep 25, 2023 at 12:41 PM Mattias Rönnblom <hofors@lysator.liu.se> wrote:
> > >
> > > On 2023-09-22 09:38, Mattias Rönnblom wrote:
> > >
> > > <snip>
> > >
> > > > +int
> > > > +rte_dispatcher_create(uint8_t id, uint8_t event_dev_id)
> > > > +{
> > >
> > >
> > > There are two changes I'm considering:
> > >
> > > 1) Removing the "id" to identify the dispatcher, replacing it with an
> > > forward-declared rte_dispatcher struct pointer.
> > >
> > > struct rte_dispatcher;
> > >
> > > struct rte_dispatcher *
> > > rte_dispatcher_create(uint8_t event_dev_id);
> > >
> > >
> > > The original reason for using an integer id to identify a dispatcher is
> > > to make it look like everything else in Eventdev. I find this pattern a
> > > little awkward to use - in particular the fact the id is
> > > application-allocated (and thus require coordination between different
> > > part of the application in case multiple instances are used).
> > >
> > > 2) Adding a flags field to the create function "for future use". But
> > > since the API is experimental, there may not be that much need to
> > > attempt to be future-proof?
> > >
> > > Any thoughts are appreciated.
> >
> > IMO, better to have rte_dispatcher_create(struct
> > rte_dispatch_create_params *params)
> > for better future proofing with specific
> > rte_dispatch_crearte_params_init() API(No need to add reserved fields
> > in rte_dispatch_create_params  now, may need only for before removing
> > experimental status)
> >
> > Just 2c.
> >
>
> I don't like using structs in those cases, I'd much rather have a flags
> parameter, as flags can be checked for explicit zeros for future proofing,
> while a struct cannot be checked for extra space on the end for future
> fields added.

For lib/dispatcher library, I have don't have specific preference. So
anything is fine for me.
However, I thought of understanding your rationale for arguments vs
structure(Looks like more of vi vs emac discussion) for _my
understanding_.

In my view,
# Use flags for setting up to express specific behavior, not as
inputting a lot of input parameters.
#  Do we need to check extra space if struct have reserved fields and
having init() functions for filling default

>
> Furthermore, if we need to add new parameters to the create function, I
> actually believe it is better to add them as explicit parameters rather
> than new fields to the struct. Struct fields can be missed by a user just
> recompiling, while new function parameters will be flagged by the compiler

I would see this as on the positive side, when

- Same code base needs to support multiple DPDK versions.
- A lot of times, API consumer may need only  _default_ values. Like
local_cache value in mempool_create API. So struct with _init() get
required values in easy way.

My views are based mostly used existing rte_mempool_create() APIs. For
some reason, I don't like this scheme.
struct rte_mempool *
rte_mempool_create(const char *name, unsigned n, unsigned elt_size,
                   unsigned cache_size, unsigned private_data_size,
                   rte_mempool_ctor_t *mp_init, void *mp_init_arg,
                   rte_mempool_obj_cb_t *obj_init, void *obj_init_arg,
                   int socket_id, unsigned flags);


> to make the user aware of the change. [There would be no change for ABI
> compatibility as function versioning would be usable in both cases]

Yes. But need to too much template code via VERSION_SYMBOL where
structure scheme does not need.




>
> /Bruce