Re: [dpdk-dev] [PATCH 7/8] event: switch sequence number to dynamic field

[David Marchand <david.marchand@redhat.com>]

On Wed, Oct 28, 2020 at 8:27 AM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > diff --git a/app/test-eventdev/test_order_common.h b/app/test-eventdev/test_order_common.h
> > index 9e3415e421..d4ad31da46 100644
> > --- a/app/test-eventdev/test_order_common.h
> > +++ b/app/test-eventdev/test_order_common.h
> > @@ -89,9 +89,10 @@ order_process_stage_1(struct test_order *const t,
> >  {
> >         const uint32_t flow = (uintptr_t)ev->mbuf % nb_flows;
> >         /* compare the seqn against expected value */
> > -       if (ev->mbuf->seqn != expected_flow_seq[flow]) {
> > +       if (*rte_event_test_seqn(ev->mbuf) != expected_flow_seq[flow]) {
> >                 evt_err("flow=%x seqn mismatch got=%x expected=%x",
> > -                       flow, ev->mbuf->seqn, expected_flow_seq[flow]);
> > +                       flow, *rte_event_test_seqn(ev->mbuf),
> > +                       expected_flow_seq[flow]);
> >                 t->err = true;
> >                 rte_smp_wmb();
> >         }
>
> # Since rte_event_test_seqn_dynfield_register() is the public API, I
> would like to limit the scope
> only to self test so that  rte_event_test_seqn_dynfield_register()
> need not be exposed.
> Could you have a separate application-specific dynamic field here?
> # Also this patch used in fastpath, better to have offset stored in
> fastpath cache line.
> see http://mails.dpdk.org/archives/dev/2020-October/189588.html

Ack for a test app dynamic field.
On the second comment, I'll wait for Thomas respin.


> > diff --git a/lib/librte_eventdev/version.map b/lib/librte_eventdev/version.map
> > index 8ae8420f9b..e49382ba99 100644
> > --- a/lib/librte_eventdev/version.map
> > +++ b/lib/librte_eventdev/version.map
> > @@ -138,4 +138,6 @@ EXPERIMENTAL {
> >         __rte_eventdev_trace_port_setup;
> >         # added in 20.11
> >         rte_event_pmd_pci_probe_named;
> > +       rte_event_test_seqn_dynfield_offset;
> > +       rte_event_test_seqn_dynfield_register;
>
> Could you change symbol name to rte_event_dev_selftest_seqn_dynfield_offset()
> to limit the scope only to self test. also, it is not required to expose
> rte_event_test_seqn_dynfield_register() in that case.

How about moving this to rte_eventdev_pmd.h and make it a pmd only API?
=> rte_event_pmd_ prefix

I would mark it rte_internal too.


-- 
David Marchand