RE: [PATCH 2/2] drivers/net: support single queue per port

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday, 6 November 2024 17.07
> 
> 06/11/2024 13:19, Morten Brørup:
> > > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > > Sent: Wednesday, 6 November 2024 12.52
> > >
> > > On Fri, Oct 25, 2024 at 11:52:23AM +0000, Morten Brørup wrote:
> > > > When configuring DPDK for one queue per port
> > > > (#define RTE_MAX_QUEUES_PER_PORT 1), compilation of some network
> > > drivers
> > > > fails with e.g.:
> > > >
> > > > ../drivers/net/bnxt/bnxt_rxq.c: In function 'bnxt_rx_queue_stop':
> > > > ../drivers/net/bnxt/bnxt_rxq.c:587:34: error: array subscript 1
> is
> > > above array bounds of 'uint8_t[1]' {aka 'unsigned char[1]'} [-
> > > Werror=array-bounds=]
> > > >   587 |         dev->data->rx_queue_state[q_id] =
> > > RTE_ETH_QUEUE_STATE_STOPPED;
> > > >       |         ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~
> > > > In file included from ../drivers/net/bnxt/bnxt.h:16,
> > > >                  from ../drivers/net/bnxt/bnxt_rxq.c:10:
> > > > ../lib/ethdev/ethdev_driver.h:168:17: note: while referencing
> > > 'rx_queue_state'
> > > >   168 |         uint8_t rx_queue_state[RTE_MAX_QUEUES_PER_PORT];
> > > >       |                 ^~~~~~~~~~~~~~
> > > >
> > > > To fix this, a hint is added to the network drivers where a
> compiler
> > > in
> > > > the CI has been seen to emit the above error when DPDK is
> configured
> > > for
> > > > one queue per port, but we know that the error cannot occur.
> [...]
> > > >  	for (i = 0; i < dev->data->nb_rx_queues; i++) {
> > > > +		__rte_assume(i < RTE_MAX_QUEUES_PER_PORT);
> > > >  		rxq = dev->data->rx_queues[i];
> [...]
> > > BTW: is this the only/best way to put in the assumption? If it were
> me,
> > > I'd
> > > look to put before the loop the underlying assumption that
> > > (dev->data->nb_XX_queues < RTE_MAX_QUEUES_PER_PORT), rather than
> > > putting
> > > the assumption on "i".
> >
> > I would also prefer putting it outside the loop,
> > but it doesn't work in cases where the variable
> > is potentially modified inside the loop.
> > And here's the problem with that:
> > Passing it as a parameter to a logging macro
> > makes the compiler think it is "potentially modified".
> 
> I don't understand this part.
> "i" is not a pointer, so how it can be modified?

After a lot of speculation, it was the only reason I could come up with why the compiler would think "i" had been modified. I am not a compiler specialist, and haven't looked deep into GCC, so my conclusion might be wrong.
Also I don't have a deep understanding of GCC's builtin va_args implementation; but I have seen other implementations of the va_args macros using references, so passing "i" they effectively becomes "&i" when using the va_args macros.

Anyway, I have now tested putting the assumption outside the loop (TEST v11), and compilation fails with the same warnings. So the assumptions need to be put very close to the line causing the warning.

Test patch (TEST v8) with assumptions located like this patch, compilation succeeds:
https://patchwork.dpdk.org/project/dpdk/patch/20241025082338.1165360-1-mb@smartsharesystems.com/

Test patch (TEST v11) with assumption outside the loop, compilation fails:
https://patchwork.dpdk.org/project/dpdk/patch/20241107092649.1507375-1-mb@smartsharesystems.com/
http://mails.dpdk.org/archives/test-report/2024-November/821886.html

> 
> > And thus, I have to put it where it hurts, and decided to do it
> consistently.
> 
> Why doing something heavier consistently?
> I would prefer to catch the problematic cases only with this macro.

Agree. I put it only with the problematic cases.
By "consistently" I mean: always just before the line causing the warning, not sometimes outside the loop.

It's always a pain having work around compiler bugs, so I'm trying to keep the ugliness of this workaround at a minimum. :-(