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

["Mattias Rönnblom" <mattias.ronnblom@ericsson.com>]

On 2021-10-29 17:17, Jerin Jacob wrote:
> 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.


Applications that are using some other event device today, and don't 
care about DSW or potential future event devices 
requiringRTE_EVENT_DEV_CAP_REQUIRES_MAINT, won't be affected at all, 
except the ops struct will be 8 bytes larger.


A rte_event_maintain() call on a device which doesn't need maintenance 
is just an inlined NULL compare on the ops struct field, which is 
frequently used and should be in a cache close to the core. In my 
benchmarks, I've been unable to measure any additional cost at all.


I reviewed the test and example applications last time I attempted to 
upstream this patch set, and from what I remember there was nothing to 
update. Things might have changed and I might misremember, so I'll have 
a look again.


What's important to keep in mind is that applications (DPDK tests, 
examples, user applications etc.) that have producer-only ports or 
otherwise potentially leave eventdev ports "unattended" don't work with 
DSW today, unless the take the measures described in the DSW 
documentation (which for example the eventdev adapters do not). So 
rte_event_maintain() will not break anything that's not already broken.


> 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.


rte_event_maintain() is called when no interaction with the event device 
has been done, during that service function call. That's the overall 
intention.

In the RX adapter, zero flushed events can also mean that the RX adapter 
had buffered events it wanted to flush, but the event device didn't 
accept new events (i.e, back pressure). In that case, the 
rte_event_maintain() call is redundant, but harmless (both because it's 
very low overhead on DSW, and near-zero overhead on any other current 
event device). Plus, if you are back-pressured by the pipeline, RX is 
not the bottleneck so a tiny bit of extra overhead is not an issue.


> In such a case, if the port is configured as producer and then
> internally it can call maintain.


To be able to perform maintenance (flushing, migration etc.), it needs 
cycles from the thread that "owns" the port. If the thread neither does 
enqueue (because it doesn't have anything to enqueue), nor dequeue, the 
driver will never get the chance to run. If DPDK had a delayed work 
mechanism that somehow could be tied to the "owning" port, then you 
could use that. But it doesn't.


> 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
>>>>