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