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

[Olivier Matz <olivier.matz@6wind.com>]

Hi,

On Wed, Oct 27, 2021 at 09:20:52AM +0200, David Marchand wrote:
> 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?

Let me try an explanation, hope it is the correct one.

In the first case, this is an assignment as described in 6.5.16 of
the standard [1]:

  *fpo = (struct rte_eth_fp_ops) { .rx_pkt_burst = dummy_eth_rx_burst, ... };

The compiler expects the right side to be an expression. The expression
is a "compound literal", as described in 6.5.2.5:

 1. The type name shall specify a complete object type or an array of
    unknown size, but not avariable length array type.
 2. All the constraints for initializer lists in 6.7.9 also apply to
    compound literals

The second cast { ..., .rxq = (struct rte_ethdev_qdata) { ... } } is
inside a construction that behaves like an initialization (according to
the second point above). The compiler already knows the type of the
struct (and therefore the types of the fields), so the cast is not
required.

[1] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1548.pdf

 
> >
> > > +                     .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
>