Re: [dpdk-dev] [PATCH 1/3] eventdev: allow for event devices requiring maintenance

[Jerin Jacob <jerinjacobk@gmail.com>]

On Fri, Oct 29, 2021 at 8:33 PM Mattias Rönnblom
<mattias.ronnblom@ericsson.com> wrote:
>
> On 2021-10-29 16:38, Jerin Jacob wrote:
> > On Tue, Oct 26, 2021 at 11:02 PM Mattias Rönnblom
> > <mattias.ronnblom@ericsson.com> wrote:
> >> Extend Eventdev API to allow for event devices which require various
> >> forms of internal processing to happen, even when events are not
> >> enqueued to or dequeued from a port.
> >>
> >> PATCH v1:
> >>    - Adapt to the move of fastpath function pointers out of
> >>      rte_eventdev struct
> >>    - Attempt to clarify how often the application is expected to
> >>      call rte_event_maintain()
> >>    - Add trace point
> >> RFC v2:
> >>    - Change rte_event_maintain() return type to be consistent
> >>      with the documentation.
> >>    - Remove unused typedef from eventdev_pmd.h.
> >>
> >> Signed-off-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
> >> Tested-by: Richard Eklycke <richard.eklycke@ericsson.com>
> >> Tested-by: Liron Himi <lironh@marvell.com>
> >> ---
> >>
> >> +/**
> >> + * Maintain an event device.
> >> + *
> >> + * This function is only relevant for event devices which has the
> >> + * RTE_EVENT_DEV_CAP_REQUIRES_MAINT flag set. Such devices require the
> >> + * application to call rte_event_maintain() on a port during periods
> >> + * which it is neither enqueuing nor dequeuing events from that
> >> + * port.
> > # We need to add  "by the same core". Right? As other core such as
> > service core can not call rte_event_maintain()
>
>
> Do you mean by the same lcore thread that "owns" (dequeues and enqueues
> to) the port? Yes. I thought that was implicit, since eventdev port are
> not MT safe. I'll try to figure out some wording that makes that more clear.

OK.

>
>
> > # Also, Incase of Adapters enqueue() happens, right? If so, either
> > above text is not correct.
> > # @Erik Gabriel Carrillo  @Jayatheerthan, Jay @Gujjar, Abhinandan S
> > Please review 3/3 patch on adapter change.
> > Let me know you folks are OK with change or not or need more time to analyze.
> >
> > If it need only for the adapter subsystem then can we make it an
> > internal API between DSW and adapters?
>
>
> No, it's needed for any producer-only eventdev ports, including any such
> ports used by the application.


In that case, the code path in testeventdev, eventdev_pipeline, etc needs
to be updated. I am worried about the performance impact for the drivers they
don't have such limitations.

Why not have an additional config option in port_config which says
it is a producer-only port by an application and takes care of the driver.

In the current adapters code, you are calling maintain() when enqueue
returns zero.
In such a case, if the port is configured as producer and then
internally it can call maintain.

Thoughts from other eventdev maintainers?
Cc+ @Van Haaren, Harry  @Richardson, Bruce @Gujjar, Abhinandan S
@Jayatheerthan, Jay @Erik Gabriel Carrillo @McDaniel, Timothy @Pavan
Nikhilesh  @Hemant Agrawal @Liang Ma


>
>
> Should rte_event_maintain() be marked experimental? I don't know how
> that works for inline functions.
>
>
> >
> > +  rte_event_maintain() is a low-overhead function and should be
> >> + * called at a high rate (e.g., in the applications poll loop).
> >> + *
> >> + * No port may be left unmaintained.
> >> + *
> >> + * rte_event_maintain() may be called on event devices which haven't
> >> + * set RTE_EVENT_DEV_CAP_REQUIRES_MAINT flag, in which case it is a
> >> + * no-operation.
> >> + *
> >> + * @param dev_id
> >> + *   The identifier of the device.
> >> + * @param port_id
> >> + *   The identifier of the event port.
> >> + * @return
> >> + *  - 0 on success.
> >> + *  - -EINVAL if *dev_id* or *port_id* is invalid
> >> + *
> >> + * @see RTE_EVENT_DEV_CAP_REQUIRES_MAINT
> >> + */
> >> +static inline int
> >> +rte_event_maintain(uint8_t dev_id, uint8_t port_id)
> >> +{
> >> +       const struct rte_event_fp_ops *fp_ops;
> >> +       void *port;
> >> +
> >> +       fp_ops = &rte_event_fp_ops[dev_id];
> >> +       port = fp_ops->data[port_id];
> >> +#ifdef RTE_LIBRTE_EVENTDEV_DEBUG
> >> +       if (dev_id >= RTE_EVENT_MAX_DEVS ||
> >> +           port_id >= RTE_EVENT_MAX_PORTS_PER_DEV) {
> >> +               rte_errno = EINVAL;
> >> +               return 0;
> >> +       }
> >> +
> >> +       if (port == NULL) {
> >> +               rte_errno = EINVAL;
> >> +               return 0;
> >> +       }
> >> +#endif
> >> +       rte_eventdev_trace_maintain(dev_id, port_id);
> >> +
> >> +       if (fp_ops->maintain != NULL)
> >> +               fp_ops->maintain(port);
> >> +
> >> +       return 0;
> >> +}
> >> +
> >>   #ifdef __cplusplus
> >>   }
> >>   #endif
> >> diff --git a/lib/eventdev/rte_eventdev_core.h b/lib/eventdev/rte_eventdev_core.h
> >> index 61d5ebdc44..61fa65cab3 100644
> >> --- a/lib/eventdev/rte_eventdev_core.h
> >> +++ b/lib/eventdev/rte_eventdev_core.h
> >> @@ -29,6 +29,9 @@ typedef uint16_t (*event_dequeue_burst_t)(void *port, struct rte_event ev[],
> >>                                            uint64_t timeout_ticks);
> >>   /**< @internal Dequeue burst of events from port of a device */
> >>
> >> +typedef void (*event_maintain_t)(void *port);
> >> +/**< @internal Maintains a port */
> >> +
> >>   typedef uint16_t (*event_tx_adapter_enqueue_t)(void *port,
> >>                                                 struct rte_event ev[],
> >>                                                 uint16_t nb_events);
> >> @@ -54,6 +57,8 @@ struct rte_event_fp_ops {
> >>          /**< PMD dequeue function. */
> >>          event_dequeue_burst_t dequeue_burst;
> >>          /**< PMD dequeue burst function. */
> >> +       event_maintain_t maintain;
> >> +       /**< PMD port maintenance function. */
> >>          event_tx_adapter_enqueue_t txa_enqueue;
> >>          /**< PMD Tx adapter enqueue function. */
> >>          event_tx_adapter_enqueue_t txa_enqueue_same_dest;
> >> diff --git a/lib/eventdev/rte_eventdev_trace_fp.h b/lib/eventdev/rte_eventdev_trace_fp.h
> >> index 5639e0b83a..c5a79a14d8 100644
> >> --- a/lib/eventdev/rte_eventdev_trace_fp.h
> >> +++ b/lib/eventdev/rte_eventdev_trace_fp.h
> >> @@ -38,6 +38,13 @@ RTE_TRACE_POINT_FP(
> >>          rte_trace_point_emit_ptr(enq_mode_cb);
> >>   )
> >>
> >> +RTE_TRACE_POINT_FP(
> >> +       rte_eventdev_trace_maintain,
> >> +       RTE_TRACE_POINT_ARGS(uint8_t dev_id, uint8_t port_id),
> >> +       rte_trace_point_emit_u8(dev_id);
> >> +       rte_trace_point_emit_u8(port_id);
> >> +)
> >> +
> >>   RTE_TRACE_POINT_FP(
> >>          rte_eventdev_trace_eth_tx_adapter_enqueue,
> >>          RTE_TRACE_POINT_ARGS(uint8_t dev_id, uint8_t port_id, void *ev_table,
> >> --
> >> 2.25.1
> >>
>