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

[Mattias Rönnblom]

On 2021-10-29 17:17, Jerin Jacob wrote:
> On Fri, Oct 29, 2021 at 8:33 PM Mattias Rönnblom
> <mattias.ronnblom at 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 at 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 at ericsson.com>
>>>> Tested-by: Richard Eklycke <richard.eklycke at ericsson.com>
>>>> Tested-by: Liron Himi <lironh at 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
>

One more thing to consider: should we add a "int op" parameter to 
rte_event_maintain()? It would also solve hack #2 in DSW eventdev API 
integration: forcing an output buffer flush. This is today done with a 
zero-sized rte_event_enqueue() call.


You could have something like:

#define RTE_EVENT_DEV_MAINT_FLUSH (1)

int

rte_event_maintain(int op);


It would also allow future extensions of "maintain", without ABI breakage.


Explicit flush is rare in real applications, in my experience, but 
useful for test cases. I suspect for DSW to work with the DPDK eventdev 
test suite, flushing buffered events (either zero-sized enqueue, 
repeated rte_event_maintain() calls, or a single of the 
rte_event_maintain(RTE_EVENT_DEV_MAINT_FLUSH) call [assuming the above 
API]) is required in the test code.


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