> -----Original Message-----
> From: Jerin Jacob <jerinjac...@gmail.com>
> Sent: Tuesday, October 1, 2019 6:40 PM
> To: Nipun Gupta <nipun.gu...@nxp.com>
> Cc: dpdk-dev <dev@dpdk.org>; Jerin Jacob <jer...@marvell.com>; Pavan
> Nikhilesh <pbhagavat...@marvell.com>; Sunil Kumar Kori
> <sk...@marvell.com>; Hemant Agrawal <hemant.agra...@nxp.com>;
> Richardson, Bruce <bruce.richard...@intel.com>; Marko Kovacevic
> <marko.kovace...@intel.com>; Ori Kam <or...@mellanox.com>; Radu
> Nicolau <radu.nico...@intel.com>; Tomasz Kantecki
> <tomasz.kante...@intel.com>; Van Haaren, Harry
> <harry.van.haa...@intel.com>; nikhil....@intel.com
> Subject: Re: [dpdk-dev] [PATCH] eventdev: flag to identify same destined
> packets enqueue
>
> On Tue, Oct 1, 2019 at 5:11 PM Nipun Gupta <nipun.gu...@nxp.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Jerin Jacob <jerinjac...@gmail.com>
> > > Sent: Tuesday, October 1, 2019 1:14 PM
> > > To: Nipun Gupta <nipun.gu...@nxp.com>
> > > Cc: dpdk-dev <dev@dpdk.org>; Jerin Jacob <jer...@marvell.com>; Pavan
> > > Nikhilesh <pbhagavat...@marvell.com>; Sunil Kumar Kori
> <sk...@marvell.com>;
> > > Hemant Agrawal <hemant.agra...@nxp.com>; Richardson, Bruce
> > > <bruce.richard...@intel.com>; Marko Kovacevic
> > > <marko.kovace...@intel.com>; Ori Kam <or...@mellanox.com>; Radu
> Nicolau
> > > <radu.nico...@intel.com>; Tomasz Kantecki
> <tomasz.kante...@intel.com>; Van
> > > Haaren, Harry <harry.van.haa...@intel.com>; nikhil....@intel.com
> > > Subject: Re: [dpdk-dev] [PATCH] eventdev: flag to identify same
> destined
> > > packets enqueue
> > >
> > > On Tue, Oct 1, 2019 at 12:32 PM Nipun Gupta <nipun.gu...@nxp.com>
> wrote:
> > > >
> > > > This patch introduces a `flag` in the Eth TX adapter enqueue API.
> > > > Some drivers may support burst functionality only with the packets
> > > > having same destination device and queue.
> > > >
> > > > The flag `RTE_EVENT_ETH_TX_ADAPTER_ENQUEUE_SAME_DEST` can
> be used
> > > > to indicate this so the underlying driver, for drivers to utilize
> > > > burst functionality appropriately.
> > >
> > > I understand the cost of aggregating packets based on port and queue
> > > to make it burst.
> > > But, Could you share the use case where how do you want to use this
> > > flag? I see two possibilities in eventdev context.
> > > (Where dequeue can be from any ethdev port)
> > > a) The application does the aggregation. If so, what would the
> > > difference be in doing driver or application?
> > > b) We may use this flag when the system has only one port and one
> queue.
> > >
> > > Could you share how do you want to use this flag?
> >
> > I would say both the cases you mentioned. I prefer this to depend on the
> smartness of
> > the application, as it is aware of the number of eth devices/queues it is
> using.
> > This is the reason to avoid in the driver.
> >
> > A user can also use separate event ports for separate or a group of
> Ethernet
> > devices/queues, to create easy segregation.
>
> If it is specific to _very_ static configuration, you can assume all
> the events comes
> from a specific eventdev port, comes only from a specific ethdev port.
Hi Jerin,
If I understand correctly this assumption would be made in the driver?
But then applications like l2fwd-event will break.
>
> >
> > Also the drawback of implementing in the driver, can be that when the
> events to
> > Separate devices and queues are so mixed up, that segregation does not
> make
> > sense, it shall be overhead for the driver to scan all the events.
>
> In the worst case, both, applications need to scan and driver need to
> iterate over
> all the events.
Agree, this is what we want to avoid.
>
>
> In generic l2fwd-eventdev applications etc, q0 will be connected p0 and p1
> etc.
> This flag will be zero.
Okay.
>
> But, If you think, there is a specific use case for it we can add this flag.
>
>
> >
> > A flag would help in providing the decision flexibility to the applications.
> >
> > >
> > > And another point is, tx adapter is NOT experimental now, We need
> > > depreciation notice for ABI change.
> > > If you share the exact use case, then we could think of adding a new
> > > symbol instead of breaking ABI and
> > > add it for next release.
> >
> > I have heard the discussion that we may get some exceptions to
> deprecation
> > process for 19.11 as the APIs will freeze of 1 year post it.
> > Anyway, if you have a better way with symbol, please suggest.
>
>
> One option could be (not as bad as changing the enqueue prototype) to
> add new field in struct rte_event_eth_tx_adapter_conf.
>
> Since this scheme can be used ONLY on the static configuration, adding
> a few fields
> for Tx adapter configuration would help.
> If that field is set you can choose dev->txa_enqueue light weight
> enqueue function
> if not, the driver can aggregate the buffers and send them.
Thanks for suggesting this, we also thought of it, but would prefer having this
as a
runtime option rather than static option.
We would not like to have one time configuration restriction.
>
>
>
> >
> > >
> > >
> > >
> > >
> > > > Signed-off-by: Nipun Gupta <nipun.gu...@nxp.com>
> > > > ---
> > > > app/test-eventdev/test_pipeline_common.h | 6 +++---
> > > > .../prog_guide/event_ethernet_tx_adapter.rst | 3 ++-
> > > > drivers/event/octeontx/ssovf_evdev.h | 2 +-
> > > > drivers/event/octeontx/ssovf_worker.c | 3 ++-
> > > > drivers/event/octeontx2/otx2_evdev.h | 12 ++++++++----
> > > > drivers/event/octeontx2/otx2_worker.c | 8 ++++++--
> > > > drivers/event/octeontx2/otx2_worker_dual.c | 8 ++++++--
> > > > lib/librte_eventdev/rte_event_eth_tx_adapter.h | 15
> +++++++++++++--
> > > > lib/librte_eventdev/rte_eventdev.c | 3 ++-
> > > > lib/librte_eventdev/rte_eventdev.h | 2 +-
> > > > 10 files changed, 44 insertions(+), 18 deletions(-)
> > > >
> > > > diff --git a/app/test-eventdev/test_pipeline_common.h b/app/test-
> > > eventdev/test_pipeline_common.h
> > > > index 0440b9e29..6e73c6ab2 100644
> > > > --- a/app/test-eventdev/test_pipeline_common.h
> > > > +++ b/app/test-eventdev/test_pipeline_common.h
> > > > @@ -106,7 +106,7 @@ pipeline_event_tx(const uint8_t dev, const
> uint8_t
> > > port,
> > > > struct rte_event * const ev)
> > > > {
> > > > rte_event_eth_tx_adapter_txq_set(ev->mbuf, 0);
> > > > - while (!rte_event_eth_tx_adapter_enqueue(dev, port, ev, 1))
> > > > + while (!rte_event_eth_tx_adapter_enqueue(dev, port, ev, 1, 0))
> > > > rte_pause();
> > > > }
> > > >
> > > > @@ -116,10 +116,10 @@ pipeline_event_tx_burst(const uint8_t dev,
> const
> > > uint8_t port,
> > > > {
> > > > uint16_t enq;
> > > >
> > > > - enq = rte_event_eth_tx_adapter_enqueue(dev, port, ev, nb_rx);
> > > > + enq = rte_event_eth_tx_adapter_enqueue(dev, port, ev, nb_rx,
> 0);
> > > > while (enq < nb_rx) {
> > > > enq += rte_event_eth_tx_adapter_enqueue(dev, port,
> > > > - ev + enq, nb_rx - enq);
> > > > + ev + enq, nb_rx - enq, 0);
> > > > }
> > > > }
> > > >
> > > > diff --git a/doc/guides/prog_guide/event_ethernet_tx_adapter.rst
> > > b/doc/guides/prog_guide/event_ethernet_tx_adapter.rst
> > > > index 192f9e1cf..a8c13e136 100644
> > > > --- a/doc/guides/prog_guide/event_ethernet_tx_adapter.rst
> > > > +++ b/doc/guides/prog_guide/event_ethernet_tx_adapter.rst
> > > > @@ -137,11 +137,12 @@ should use the
> ``rte_event_enqueue_burst()``
> > > function.
> > > > if (cap & RTE_EVENT_ETH_TX_ADAPTER_CAP_INTERNAL_PORT) {
> > > >
> > > > event.mbuf = m;
> > > > + eq_flags = 0;
> > > >
> > > > m->port = tx_port;
> > > > rte_event_eth_tx_adapter_txq_set(m, tx_queue_id);
> > > >
> > > > - rte_event_eth_tx_adapter_enqueue(dev_id, ev_port,
> > > > &event,
> 1);
> > > > + rte_event_eth_tx_adapter_enqueue(dev_id, ev_port,
> &event, 1,
> > > eq_flags);
> > > > } else {
> > > >
> > > > event.queue_id = qid; /* event queue linked to adapter
> > > > port */
> > > > diff --git a/drivers/event/octeontx/ssovf_evdev.h
> > > b/drivers/event/octeontx/ssovf_evdev.h
> > > > index 0e622152c..1b156edab 100644
> > > > --- a/drivers/event/octeontx/ssovf_evdev.h
> > > > +++ b/drivers/event/octeontx/ssovf_evdev.h
> > > > @@ -181,7 +181,7 @@ void ssows_flush_events(struct ssows *ws,
> uint8_t
> > > queue_id,
> > > > ssows_handle_event_t fn, void *arg);
> > > > void ssows_reset(struct ssows *ws);
> > > > uint16_t sso_event_tx_adapter_enqueue(void *port,
> > > > - struct rte_event ev[], uint16_t nb_events);
> > > > + struct rte_event ev[], uint16_t nb_events, uint8_t
> > > > eq_flags);
> > > > int ssovf_info(struct ssovf_info *info);
> > > > void *ssovf_bar(enum ssovf_type, uint8_t id, uint8_t bar);
> > > > int test_eventdev_octeontx(void);
> > > > diff --git a/drivers/event/octeontx/ssovf_worker.c
> > > b/drivers/event/octeontx/ssovf_worker.c
> > > > index d940b5dd6..1d0467af3 100644
> > > > --- a/drivers/event/octeontx/ssovf_worker.c
> > > > +++ b/drivers/event/octeontx/ssovf_worker.c
> > > > @@ -264,7 +264,7 @@ ssows_reset(struct ssows *ws)
> > > >
> > > > uint16_t
> > > > sso_event_tx_adapter_enqueue(void *port,
> > > > - struct rte_event ev[], uint16_t nb_events)
> > > > + struct rte_event ev[], uint16_t nb_events, uint8_t
> > > > eq_flags)
> > > > {
> > > > uint16_t port_id;
> > > > uint16_t queue_id;
> > > > @@ -275,6 +275,7 @@ sso_event_tx_adapter_enqueue(void *port,
> > > > octeontx_dq_t *dq;
> > > >
> > > > RTE_SET_USED(nb_events);
> > > > + RTE_SET_USED(eq_flags);
> > > > switch (ev->sched_type) {
> > > > case SSO_SYNC_ORDERED:
> > > > ssows_swtag_norm(ws, ev->event, SSO_SYNC_ATOMIC);
> > > > diff --git a/drivers/event/octeontx2/otx2_evdev.h
> > > b/drivers/event/octeontx2/otx2_evdev.h
> > > > index 5cd80e3b2..74b749a15 100644
> > > > --- a/drivers/event/octeontx2/otx2_evdev.h
> > > > +++ b/drivers/event/octeontx2/otx2_evdev.h
> > > > @@ -333,16 +333,20 @@ SSO_RX_ADPTR_ENQ_FASTPATH_FUNC
> > > >
> > > > #define T(name, f4, f3, f2, f1, f0, sz, flags)
> > > > \
> > > > uint16_t otx2_ssogws_tx_adptr_enq_ ## name(void *port, struct
> rte_event
> > > ev[],\
> > > > - uint16_t nb_events);
> > > > \
> > > > + uint16_t nb_events,
> > > > \
> > > > + uint8_t eq_flags);
> > > > \
> > > > uint16_t otx2_ssogws_tx_adptr_enq_seg_ ## name(void *port,
> \
> > > > struct rte_event ev[],
> > > > \
> > > > - uint16_t nb_events);
> > > > \
> > > > + uint16_t nb_events,
> > > > \
> > > > + uint8_t eq_flags);
> > > > \
> > > > uint16_t otx2_ssogws_dual_tx_adptr_enq_ ## name(void *port,
> \
> > > > struct rte_event ev[],
> > > > \
> > > > - uint16_t nb_events);
> > > > \
> > > > + uint16_t nb_events,
> > > > \
> > > > + uint8_t eq_flags);
> > > > \
> > > > uint16_t otx2_ssogws_dual_tx_adptr_enq_seg_ ## name(void *port,
> > > \
> > > > struct rte_event
> > > > ev[], \
> > > > - uint16_t
> > > > nb_events); \
> > > > + uint16_t nb_events,
> > > > \
> > > > + uint8_t eq_flags);
> > > > \
> > > >
> > > > SSO_TX_ADPTR_ENQ_FASTPATH_FUNC
> > > > #undef T
> > > > diff --git a/drivers/event/octeontx2/otx2_worker.c
> > > b/drivers/event/octeontx2/otx2_worker.c
> > > > index cd14cd3d2..100e21669 100644
> > > > --- a/drivers/event/octeontx2/otx2_worker.c
> > > > +++ b/drivers/event/octeontx2/otx2_worker.c
> > > > @@ -270,12 +270,14 @@ otx2_ssogws_enq_fwd_burst(void *port,
> const
> > > struct rte_event ev[],
> > > > #define T(name, f4, f3, f2, f1, f0, sz, flags)
> > > > \
> > > > uint16_t __hot
> > > > \
> > > > otx2_ssogws_tx_adptr_enq_ ## name(void *port, struct rte_event
> ev[], \
> > > > - uint16_t nb_events)
> > > > \
> > > > + uint16_t nb_events,
> > > > \
> > > > + uint8_t eq_flags)
> > > > \
> > > > {
> > > > \
> > > > struct otx2_ssogws *ws = port;
> > > > \
> > > > uint64_t cmd[sz];
> > > > \
> > > >
> > > > \
> > > > RTE_SET_USED(nb_events);
> > > > \
> > > > + RTE_SET_USED(eq_flags);
> > > > \
> > > > return otx2_ssogws_event_tx(ws, ev, cmd, flags);
> > > > \
> > > > }
> > > > SSO_TX_ADPTR_ENQ_FASTPATH_FUNC
> > > > @@ -284,12 +286,14 @@ SSO_TX_ADPTR_ENQ_FASTPATH_FUNC
> > > > #define T(name, f4, f3, f2, f1, f0, sz, flags)
> > > > \
> > > > uint16_t __hot
> > > > \
> > > > otx2_ssogws_tx_adptr_enq_seg_ ## name(void *port, struct
> rte_event ev[],\
> > > > - uint16_t nb_events)
> > > > \
> > > > + uint16_t nb_events,
> > > > \
> > > > + uint8_t eq_flags)
> > > > \
> > > > {
> > > > \
> > > > struct otx2_ssogws *ws = port;
> > > > \
> > > > uint64_t cmd[(sz) + NIX_TX_MSEG_SG_DWORDS - 2];
> > > > \
> > > >
> > > > \
> > > > RTE_SET_USED(nb_events);
> > > > \
> > > > + RTE_SET_USED(eq_flags);
> > > > \
> > > > return otx2_ssogws_event_tx(ws, ev, cmd, (flags) |
> > > > \
> > > > NIX_TX_MULTI_SEG_F);
> > > > \
> > > > }
> > > > diff --git a/drivers/event/octeontx2/otx2_worker_dual.c
> > > b/drivers/event/octeontx2/otx2_worker_dual.c
> > > > index 37c274a54..c3e48da42 100644
> > > > --- a/drivers/event/octeontx2/otx2_worker_dual.c
> > > > +++ b/drivers/event/octeontx2/otx2_worker_dual.c
> > > > @@ -310,7 +310,8 @@ SSO_RX_ADPTR_ENQ_FASTPATH_FUNC
> > > > uint16_t __hot
> > > > \
> > > > otx2_ssogws_dual_tx_adptr_enq_ ## name(void *port,
> > > > \
> > > > struct rte_event ev[],
> > > > \
> > > > - uint16_t nb_events)
> > > > \
> > > > + uint16_t nb_events,
> > > > \
> > > > + uint8_t eq_flags)
> > > > \
> > > > {
> > > > \
> > > > struct otx2_ssogws_dual *ws = port;
> > > > \
> > > > struct otx2_ssogws *vws =
> > > > \
> > > > @@ -318,6 +319,7 @@ otx2_ssogws_dual_tx_adptr_enq_ ##
> name(void *port,
> > > \
> > > > uint64_t cmd[sz];
> > > > \
> > > >
> > > > \
> > > > RTE_SET_USED(nb_events);
> > > > \
> > > > + RTE_SET_USED(eq_flags);
> > > > \
> > > > return otx2_ssogws_event_tx(vws, ev, cmd, flags);
> > > > \
> > > > }
> > > > SSO_TX_ADPTR_ENQ_FASTPATH_FUNC
> > > > @@ -327,7 +329,8 @@ SSO_TX_ADPTR_ENQ_FASTPATH_FUNC
> > > > uint16_t __hot
> > > > \
> > > > otx2_ssogws_dual_tx_adptr_enq_seg_ ## name(void *port,
> > > > \
> > > > struct rte_event ev[],
> > > > \
> > > > - uint16_t nb_events)
> > > > \
> > > > + uint16_t nb_events,
> > > > \
> > > > + uint8_t eq_flags)
> > > > \
> > > > {
> > > > \
> > > > struct otx2_ssogws_dual *ws = port;
> > > > \
> > > > struct otx2_ssogws *vws =
> > > > \
> > > > @@ -335,6 +338,7 @@ otx2_ssogws_dual_tx_adptr_enq_seg_ ##
> name(void
> > > *port, \
> > > > uint64_t cmd[(sz) + NIX_TX_MSEG_SG_DWORDS - 2];
> > > > \
> > > >
> > > > \
> > > > RTE_SET_USED(nb_events);
> > > > \
> > > > + RTE_SET_USED(eq_flags);
> > > > \
> > > > return otx2_ssogws_event_tx(vws, ev, cmd, (flags) |
> > > > \
> > > > NIX_TX_MULTI_SEG_F);
> > > > \
> > > > }
> > > > diff --git a/lib/librte_eventdev/rte_event_eth_tx_adapter.h
> > > b/lib/librte_eventdev/rte_event_eth_tx_adapter.h
> > > > index c848261c4..98be77568 100644
> > > > --- a/lib/librte_eventdev/rte_event_eth_tx_adapter.h
> > > > +++ b/lib/librte_eventdev/rte_event_eth_tx_adapter.h
> > > > @@ -300,6 +300,11 @@ rte_event_eth_tx_adapter_txq_get(struct
> rte_mbuf
> > > *pkt)
> > > > int
> > > > rte_event_eth_tx_adapter_event_port_get(uint8_t id, uint8_t
> > > *event_port_id);
> > > >
> > > > +#define RTE_EVENT_ETH_TX_ADAPTER_ENQUEUE_SAME_DEST 0x1
> > > > +/**< This flag is used when all the packets enqueued in the tx adapter
> are
> > > > + * destined for the same Ethernet device, queue pair.
> > > > + */
> > > > +
> > > > /**
> > > > * Enqueue a burst of events objects or an event object supplied in
> > > *rte_event*
> > > > * structure on an event device designated by its *dev_id* through
> the event
> > > > @@ -324,6 +329,10 @@
> rte_event_eth_tx_adapter_event_port_get(uint8_t
> > > id, uint8_t *event_port_id);
> > > > * The number of event objects to enqueue, typically number of
> > > > * rte_event_port_attr_get(...RTE_EVENT_PORT_ATTR_ENQ_DEPTH...)
> > > > * available for this port.
> > > > + * @param flags
> > > > + * See RTE_EVENT_ETH_TX_ADAPTER_ENQUEUE_ flags.
> > > > + * #RTE_EVENT_ETH_TX_ADAPTER_ENQUEUE_SAME_DEST signifies
> that all
> > > the packets
> > > > + * which are enqueued are destined for the same Ethernet device,
> queue pair.
> > > > *
> > > > * @return
> > > > * The number of event objects actually enqueued on the event
> device. The
> > > > @@ -343,7 +352,8 @@ static inline uint16_t
> > > > rte_event_eth_tx_adapter_enqueue(uint8_t dev_id,
> > > > uint8_t port_id,
> > > > struct rte_event ev[],
> > > > - uint16_t nb_events)
> > > > + uint16_t nb_events,
> > > > + uint8_t flags)
> > > > {
> > > > const struct rte_eventdev *dev = &rte_eventdevs[dev_id];
> > > >
> > > > @@ -359,7 +369,8 @@ rte_event_eth_tx_adapter_enqueue(uint8_t
> dev_id,
> > > > return 0;
> > > > }
> > > > #endif
> > > > - return dev->txa_enqueue(dev->data->ports[port_id], ev,
> nb_events);
> > > > + return dev->txa_enqueue(dev->data->ports[port_id], ev,
> > > > + nb_events, flags);
> > > > }
> > > >
> > > > /**
> > > > diff --git a/lib/librte_eventdev/rte_eventdev.c
> > > b/lib/librte_eventdev/rte_eventdev.c
> > > > index f44c869cb..3bf9d7115 100644
> > > > --- a/lib/librte_eventdev/rte_eventdev.c
> > > > +++ b/lib/librte_eventdev/rte_eventdev.c
> > > > @@ -1324,7 +1324,8 @@ rte_eventdev_find_free_device_index(void)
> > > > static uint16_t
> > > > rte_event_tx_adapter_enqueue(__rte_unused void *port,
> > > > __rte_unused struct rte_event ev[],
> > > > - __rte_unused uint16_t nb_events)
> > > > + __rte_unused uint16_t nb_events,
> > > > + __rte_unused uint8_t flags)
> > > > {
> > > > rte_errno = ENOTSUP;
> > > > return 0;
> > > > diff --git a/lib/librte_eventdev/rte_eventdev.h
> > > b/lib/librte_eventdev/rte_eventdev.h
> > > > index 5044a13d0..2a5643da3 100644
> > > > --- a/lib/librte_eventdev/rte_eventdev.h
> > > > +++ b/lib/librte_eventdev/rte_eventdev.h
> > > > @@ -1227,7 +1227,7 @@ typedef uint16_t
> (*event_dequeue_burst_t)(void
> > > *port, struct rte_event ev[],
> > > > /**< @internal Dequeue burst of events from port of a device */
> > > >
> > > > typedef uint16_t (*event_tx_adapter_enqueue)(void *port,
> > > > - struct rte_event ev[], uint16_t
> > > > nb_events);
> > > > + struct rte_event ev[], uint16_t nb_events, uint8_t
> > > > flags);
> > > > /**< @internal Enqueue burst of events on port of a device */
> > > >
> > > > #define RTE_EVENTDEV_NAME_MAX_LEN (64)
> > > > --
> > > > 2.17.1
> > > >
