[1/3] eventdev: allow for event devices requiring maintenance
Checks
Commit Message
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>
---
lib/eventdev/eventdev_pmd.h | 2 +
lib/eventdev/eventdev_private.c | 9 ++++
lib/eventdev/eventdev_trace_points.c | 3 ++
lib/eventdev/rte_eventdev.h | 63 ++++++++++++++++++++++++++++
lib/eventdev/rte_eventdev_core.h | 5 +++
lib/eventdev/rte_eventdev_trace_fp.h | 7 ++++
6 files changed, 89 insertions(+)
Comments
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()
# 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?
+ 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
>
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.
> # 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.
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
>>
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
> >>
>
> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Friday, October 29, 2021 4:18 PM
> To: mattias.ronnblom <mattias.ronnblom@ericsson.com>; Van Haaren, Harry
> <harry.van.haaren@intel.com>; McDaniel, Timothy
> <timothy.mcdaniel@intel.com>; Pavan Nikhilesh <pbhagavatula@marvell.com>;
> Hemant Agrawal <hemant.agrawal@nxp.com>; Liang Ma
> <liangma@liangbit.com>
> Cc: Richardson, Bruce <bruce.richardson@intel.com>; Jerin Jacob
> <jerinj@marvell.com>; Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>;
> Carrillo, Erik G <erik.g.carrillo@intel.com>; Jayatheerthan, Jay
> <jay.jayatheerthan@intel.com>; dpdk-dev <dev@dpdk.org>
> Subject: Re: [dpdk-dev] [PATCH 1/3] eventdev: allow for event devices requiring
> maintenance
>
> 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 can identify if this "maintain()" call is required by
the RTE_EVENT_DEV_CAP_REQUIRES_MAINT flag correct?
So the call through the Eventdev function pointer could be branched over at application level if required.
Or if the PMD doesn't actually set the ".maintain" function pointer to a valid value, then it will just return?
Is it easy/possible to test/benchmark this with test-eventdev to measure perf-impact?
> 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.
The "port hints" that was recently added could be used to communicate such a concept.
http://patches.dpdk.org/project/dpdk/patch/20211014145141.679372-1-harry.van.haaren@intel.com/
Note that today the "port hints" are *hints*, and must not be *relied on* to do anything.
This may be useful as (one of) the tools/concepts we could use to try abstract the "maintain" requirement.
> 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
Above, thanks for the +CC.
<snip remaining patch>
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
>>>>
On 2021-10-29 18:02, Van Haaren, Harry wrote:
>> -----Original Message-----
>> From: Jerin Jacob <jerinjacobk@gmail.com>
>> Sent: Friday, October 29, 2021 4:18 PM
>> To: mattias.ronnblom <mattias.ronnblom@ericsson.com>; Van Haaren, Harry
>> <harry.van.haaren@intel.com>; McDaniel, Timothy
>> <timothy.mcdaniel@intel.com>; Pavan Nikhilesh <pbhagavatula@marvell.com>;
>> Hemant Agrawal <hemant.agrawal@nxp.com>; Liang Ma
>> <liangma@liangbit.com>
>> Cc: Richardson, Bruce <bruce.richardson@intel.com>; Jerin Jacob
>> <jerinj@marvell.com>; Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>;
>> Carrillo, Erik G <erik.g.carrillo@intel.com>; Jayatheerthan, Jay
>> <jay.jayatheerthan@intel.com>; dpdk-dev <dev@dpdk.org>
>> Subject: Re: [dpdk-dev] [PATCH 1/3] eventdev: allow for event devices requiring
>> maintenance
>>
>> 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 can identify if this "maintain()" call is required by
> the RTE_EVENT_DEV_CAP_REQUIRES_MAINT flag correct?
Yes.
> So the call through the Eventdev function pointer could be branched over at application level if required.
Yes, you could use the constant propagation trick to eliminate all
overhead, if so would be preferred.
> Or if the PMD doesn't actually set the ".maintain" function pointer to a valid value, then it will just return?
That's another alternative, but a slightly more costly one. I fail to
see the benefit of taking that approach.
> Is it easy/possible to test/benchmark this with test-eventdev to measure perf-impact?
>
I've benchmarked it in a different test bed, and as I've mentioned, for
event devices which doesn't set RTE_EVENT_DEV_CAP_REQUIRES_MAINT and
thus have NULL pointer as the maintain function in the ops struct, I was
unable to measure any increase at all in overhead, even if they called
rte_event_maintain() for every iteration in their
"dequeue+process+enqueue" loop. Obviously, there are a couple of more
instructions in the code path, so surely there is some cost, but ISP
managed to hide everything on the system I was using.
>> 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.
> The "port hints" that was recently added could be used to communicate such a concept.
> https://protect2.fireeye.com/v1/url?k=131d62df-4c865bdd-131d2244-869a14f4b08c-e995dd419beebf7c&q=1&e=75c16b2e-15e8-4a1a-ab9f-6f80ec7138e6&u=http%3A%2F%2Fpatches.dpdk.org%2Fproject%2Fdpdk%2Fpatch%2F20211014145141.679372-1-harry.van.haaren%40intel.com%2F
>
> Note that today the "port hints" are *hints*, and must not be *relied on* to do anything.
> This may be useful as (one of) the tools/concepts we could use to try abstract the "maintain" requirement.
>
>
>> 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
> Above, thanks for the +CC.
>
> <snip remaining patch>
Sorry, I should have included all from the original (RFC) discussion.
On Sat, Oct 30, 2021 at 10:49 PM Mattias Rönnblom
<mattias.ronnblom@ericsson.com> wrote:
>
> 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.
Ack. That's not an issue.
>
>
> 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.
OK.
>
>
> 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.
OK.
>
>
> > 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.
OK.
Looks good to me. Since DSW has better performance than other SW
drivers, I think, it is OK
to take some limitations to the application.
Please send the next version with the suggested documentation change.
If there is no objection, I will merge it on Wednesday. (3rd Nov)
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.
>
> 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
>>>>
On Sun, Oct 31, 2021 at 6:47 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>
> On Sat, Oct 30, 2021 at 10:49 PM Mattias Rönnblom
> <mattias.ronnblom@ericsson.com> wrote:
> >
> > 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.
>
> Ack. That's not an issue.
>
> >
> >
> > 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.
>
>
> OK.
>
> >
> >
> > 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.
>
>
> OK.
>
> >
> >
> > > 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.
>
> OK.
>
> Looks good to me. Since DSW has better performance than other SW
> drivers, I think, it is OK
> to take some limitations to the application.
>
> Please send the next version with the suggested documentation change.
>
> If there is no objection, I will merge it on Wednesday. (3rd Nov)
Applied v3 version to dpdk-next-net-eventdev/for-main. Thanks
@@ -164,6 +164,8 @@ struct rte_eventdev {
/**< Pointer to PMD dequeue function. */
event_dequeue_burst_t dequeue_burst;
/**< Pointer to PMD dequeue burst function. */
+ event_maintain_t maintain;
+ /**< Pointer to PMD port maintenance function. */
event_tx_adapter_enqueue_t txa_enqueue_same_dest;
/**< Pointer to PMD eth Tx adapter burst enqueue function with
* events destined to same Eth port & Tx queue.
@@ -44,6 +44,13 @@ dummy_event_dequeue_burst(__rte_unused void *port,
return 0;
}
+static void
+dummy_event_maintain(__rte_unused void *port)
+{
+ RTE_EDEV_LOG_ERR(
+ "maintenance requested for unconfigured event device");
+}
+
static uint16_t
dummy_event_tx_adapter_enqueue(__rte_unused void *port,
__rte_unused struct rte_event ev[],
@@ -85,6 +92,7 @@ event_dev_fp_ops_reset(struct rte_event_fp_ops *fp_op)
.enqueue_forward_burst = dummy_event_enqueue_burst,
.dequeue = dummy_event_dequeue,
.dequeue_burst = dummy_event_dequeue_burst,
+ .maintain = dummy_event_maintain,
.txa_enqueue = dummy_event_tx_adapter_enqueue,
.txa_enqueue_same_dest =
dummy_event_tx_adapter_enqueue_same_dest,
@@ -105,6 +113,7 @@ event_dev_fp_ops_set(struct rte_event_fp_ops *fp_op,
fp_op->enqueue_forward_burst = dev->enqueue_forward_burst;
fp_op->dequeue = dev->dequeue;
fp_op->dequeue_burst = dev->dequeue_burst;
+ fp_op->maintain = dev->maintain;
fp_op->txa_enqueue = dev->txa_enqueue;
fp_op->txa_enqueue_same_dest = dev->txa_enqueue_same_dest;
fp_op->ca_enqueue = dev->ca_enqueue;
@@ -37,6 +37,9 @@ RTE_TRACE_POINT_REGISTER(rte_eventdev_trace_enq_burst,
RTE_TRACE_POINT_REGISTER(rte_eventdev_trace_deq_burst,
lib.eventdev.deq.burst)
+RTE_TRACE_POINT_REGISTER(rte_eventdev_trace_maintain,
+ lib.eventdev.maintain)
+
/* Eventdev Rx adapter trace points */
RTE_TRACE_POINT_REGISTER(rte_eventdev_trace_eth_rx_adapter_create,
lib.eventdev.rx.adapter.create)
@@ -299,6 +299,15 @@ struct rte_event;
* the content of this field is implementation dependent.
*/
+#define RTE_EVENT_DEV_CAP_REQUIRES_MAINT (1ULL << 10)
+/**< Event device requires calls to rte_event_maintain() during
+ * periods when neither rte_event_dequeue_burst() nor
+ * rte_event_enqueue_burst() are called on a port. This will allow the
+ * event device to perform internal processing, such as flushing
+ * buffered events, return credits to a global pool, or process
+ * signaling related to load balancing.
+ */
+
/* Event device priority levels */
#define RTE_EVENT_DEV_PRIORITY_HIGHEST 0
/**< Highest priority expressed across eventdev subsystem
@@ -2063,6 +2072,60 @@ rte_event_dequeue_burst(uint8_t dev_id, uint8_t port_id, struct rte_event ev[],
timeout_ticks);
}
+/**
+ * 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. 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
@@ -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;
@@ -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,