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

[Jerin Jacob <jerinjacobk@gmail.com>]

On Thu, Sep 21, 2023 at 10:17 PM Mattias Rönnblom <hofors@lysator.liu.se> wrote:
>
> On 2023-09-19 12:58, Jerin Jacob wrote:
> > On Mon, Sep 4, 2023 at 6:39 PM Mattias Rönnblom
> > <mattias.ronnblom@ericsson.com> wrote:
> >>
> >> The purpose of the dispatcher library is to help reduce coupling in an
> >> Eventdev-based DPDK application.
> >>
> >> In addition, the dispatcher also provides a convenient and flexible
> >> way for the application to use service cores for application-level
> >> processing.
> >>
> >> Signed-off-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
> >> Tested-by: Peter Nilsson <peter.j.nilsson@ericsson.com>
> >
> > High level architecture comment
> > --------------------------------
> >
> > 1) I think, we don't need tie this library ONLY to event dev
> > application. It can be used with poll mode as well,
> > that way traditiona pipeline application with ethdev as source could
> > use this library dispatch the packets.
> >
>
> They could potentially use a library *like this*, but I'm not sure it
> should be this library, or at least I think it should be a different API
> (better type checking, plus no obvious benefit of being more generic).

The only reason why I thought of this, It is cheap to do as all the logic
for comparing match actions, packet/event aggregation and calling the
action function is _same_
and better type checking can be added by separate callback for each source.
and allow more user to use this library.

I don't have a strong opinion of API semantic on this library API
other than the placement.
Feel free to ignore.

> Another option for a traditional app calling rte_eth_rx_burst() directly
> is to start using eventdev. :)

Yes. Those who can afford extra SW cores to emulate eventdev or has evendev HW.

>
> > We dont need to implement that first version but API can make room for
> > such abstractions.
> >
> > Based on my understanding in fast-path it has means to
> > a)Pull out the events using rte_event_dequeue()
> > b)Compare with registered match functions and call process upon match.
> >
> > if we abstract (a) as rte_dispatcher_source, We could pull from ethdev
> > via rte_eth_rx_burst() or
> > from ring via dequeue_burst API or so based on rte_dispatcher_source
> > selected for dispatch configuration
> > and we can use different sevice function pointers to have different service core
> > implementation without effecting performance each sources.
> >
>
> It could be generalized, for sure. I don't think it should be, at this
> point at least.
>
> Non-event dev events could - and at this point I'm leaning toward
> *should* - be consumed as a different DPDK service, but potentially on
> the same lcore.
>
> If you would want to prepare for a future with several different event
> sources, one could consider reintroducing the word "event" somewhere in
> the dispatcher's name. So you would have
> rte_event_dispatcher.h
> rte_eth_dispatcher.h
>
> or
>
> rte_dispatcher_event.h
> rte_dispatcher_eth.h

Yes.

> > High level cosmetic comment
> > ----------------------------------------------------
> > 1)Missing doxygen connection- See doc/api/doxy-api-index.md
> >
>
> rte_dispatcher.h is listed under **classification**, but this change is
> in the programming guide patch. I'll move it to the patch containing the
> header file.
>
>
> > Process related comment
> > ------------------------------------
> > 1) Documentation does not need need separate patch. All recent library
> > changes documentation in same file.
> > You could have doc and API header file as first patch and
> > implementation as subsequent patches.
> >
> >
>
> I'm not sure how this is an improvement. Can you elaborate? For me, it
> just seems like a change.
>
> Are there some guidelines on how to split a larger change into a patch
> set? A section on this matter in the contribution guide would be great.

In general, more patches easy review and attract more reviewers.

Last library was added to dpdk is lib/mldev. You can see
git log lib/mldev/

There operations like _create/free() etc made as separate patches.

I leave it up to you and Thomas as this library will be merged though main tree.
No strong opinion.

>
> >
> >> diff --git a/lib/dispatcher/rte_dispatcher.h b/lib/dispatcher/rte_dispatcher.h
> >> new file mode 100644
> >> index 0000000000..6712687a08
> >> --- /dev/null
> >> +++ b/lib/dispatcher/rte_dispatcher.h
> >> @@ -0,0 +1,480 @@
> >> +/* SPDX-License-Identifier: BSD-3-Clause
> >> + * Copyright(c) 2023 Ericsson AB
> >> + */
> >> +
> >> +#ifndef __RTE_DISPATCHER_H__
> >> +#define __RTE_DISPATCHER_H__
> >> +
> >
> >
> > All new API should be experimental. See
> > https://elixir.bootlin.com/dpdk/latest/source/lib/graph/rte_graph.h#L12
> > example.
> >
>
> Noted.
>
> >
> >> +/**
> >> + * @file
> >> + *
> >> + * RTE Dispatcher
> >> + *
> >> + * The purpose of the dispatcher is to help decouple different parts
> >> + * of an application (e.g., modules), sharing the same underlying
> >> + * event device.
> >> +
> >> +/**
> >> + * Function prototype for match callbacks.
> >> + *
> >> + * Match callbacks are used by an application to decide how the
> >> + * dispatcher distributes events to different parts of the
> >> + * application.
> >> + *
> >> + * The application is not expected to process the event at the point
> >> + * of the match call. Such matters should be deferred to the process
> >> + * callback invocation.
> >> + *
> >> + * The match callback may be used as an opportunity to prefetch data.
> >> + *
> >> + * @param event
> >> + *  Pointer to event
> >> + *
> >> + * @param cb_data
> >> + *  The pointer supplied by the application in
> >> + *  rte_dispatcher_register().
> >> + *
> >> + * @return
> >> + *   Returns true in case this events should be delivered (via
> >> + *   the process callback), and false otherwise.
> >> + */
> >> +typedef bool
> >> +(*rte_dispatcher_match_t)(const struct rte_event *event, void *cb_data);
> >
> >
> > a) Can we use void* event, so that it can be used with mbuf or other
> > type by casting in the call back implementer.
> >
> > b) I was thinking, How we can avoid this function pointer and enable
> > more have better performance at architecture level.
> >
> > Both x86, ARM has vector instructions[1] to form a vector from various
> > offset from memory and compare N events
> > in one shot. That is, if express match data like offset = X has value
> > is Y and offset = X has value = A.
> > I know, it may not good existing application using this APIs. But I
> > believe, it will be more performance
> > effective. If make sense, you can adapt to this.(Something to think about)
> >
>
> There may be a future development where you try to shave off a few of
> the circa 10 clock cycles per event of overhead that the current

OK, as you wish.