Re: [dpdk-dev] [PATCH] ethdev: warn only once for badly behaving applications

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

On Tue, Oct 26, 2021 at 5:57 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 26/10/2021 16:58, David Marchand:
> > Warning continuously is a pain when developping or if a unit test
> > is/gets broken.
> >
> > It could also be a problem if application behaves badly only in some
> > corner cases and a DoS results of those logs being continuously displayed.
> >
> > Let's warn once per port and per rx/tx.
> >
> > Getting such a log is scary, but let's make it more eye catching by
> > dumping a backtrace with it.
> [...]
> > Fixes: c87d435a4d79 ("ethdev: copy fast-path API into separate structure")
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> [...]
> > +static struct dummy_queue *dummy_queues_ref[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
> > +static struct dummy_queue dummy_queues[RTE_MAX_ETHPORTS];
>
> I feel we could better name those arrays, maybe adding a comment.
> First one is really queues array while the second one is to share
> the same value with all queues of a port. Right?

Yes, look fwd to v2 for better names.


>
> > +RTE_INIT(dummy_queue_init)
> > +{
> > +     uint16_t port_id;
> > +
> > +     for (port_id = 0; port_id < RTE_DIM(dummy_queues); port_id++) {
> > +             unsigned int i;
>
> q would be a better name than i

Ok, and I'll rename other variable q for actual queue objects later in
the patch.


> >  eth_dev_fp_ops_reset(struct rte_eth_fp_ops *fpo)
> >  {
> >       static void *dummy_data[RTE_MAX_QUEUES_PER_PORT];
> > -     static const struct rte_eth_fp_ops dummy_ops = {
> > +     uint16_t port_id = fpo - rte_eth_fp_ops;
> > +
> > +     dummy_queues[port_id].rx_warn_once = false;
> > +     dummy_queues[port_id].tx_warn_once = false;
> > +     *fpo = (struct rte_eth_fp_ops) {
> >               .rx_pkt_burst = dummy_eth_rx_burst,
> >               .tx_pkt_burst = dummy_eth_tx_burst,
> > -             .rxq = {.data = dummy_data, .clbk = dummy_data,},
> > -             .txq = {.data = dummy_data, .clbk = dummy_data,},
> > +             .rxq = (struct rte_ethdev_qdata) {
>
> Why this cast? rte_eth_fp_ops.rxq is of type rte_ethdev_qdata.

Funny how the compiler complains about:

../lib/ethdev/ethdev_private.c: In function ‘eth_dev_fp_ops_reset’:
../lib/ethdev/ethdev_private.c:243:9: error: expected expression
before ‘{’ token
  *fpo = {
         ^
if we don't explicitely tell this anonymous struct is of type struct
rte_eth_fp_ops (note that *fpo is of type struct rte_eth_fp_ops).
But otoh, compiler silently understands that, in .rxq case, the
anonymous struct is of type rte_ethdev_qdata.

So indeed, it works without the cast on .rxq and .txq.
I applied the cast on all anonymous struct in my patch once I hit the
first compiler complaint.

Do you have the explanation or can you point me at some standard
explaining the difference in treatment?


>
> > +                     .data = (void **)&dummy_queues_ref[port_id],
> > +                     .clbk = dummy_data,
> > +             },
> > +             .txq = (struct rte_ethdev_qdata) {
> > +                     .data = (void **)&dummy_queues_ref[port_id],
> > +                     .clbk = dummy_data,
> > +             },
> >       };
> > -
> > -     *fpo = dummy_ops;
> >  }


-- 
David Marchand