Re: [PATCH 1/3 net-next] bnx2: Add support for ethtool --show-channels|--set-channels

[Ben Hutchings <bhutchings@solarflare.com>]

On Tue, 2012-02-07 at 22:01 +0000, Ben Hutchings wrote:
> On Tue, 2012-02-07 at 12:58 -0800, Michael Chan wrote:
> > On Tue, 2012-02-07 at 20:19 +0000, Ben Hutchings wrote:
> > > On Sun, 2012-02-05 at 17:24 -0800, Michael Chan wrote:
> > > > Allow the user to override the default number of RSS/TSS rings.
> > > > 
> > > > Signed-off-by: Michael Chan <mchan@broadcom.com>
> > > > ---
> > > >  drivers/net/ethernet/broadcom/bnx2.c |   99 ++++++++++++++++++++++++++++++---
> > > >  drivers/net/ethernet/broadcom/bnx2.h |    3 +
> > > >  2 files changed, 93 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/drivers/net/ethernet/broadcom/bnx2.c b/drivers/net/ethernet/broadcom/bnx2.c
> > > > index 0a4c540..2ab31da 100644
> > > > --- a/drivers/net/ethernet/broadcom/bnx2.c
> > > > +++ b/drivers/net/ethernet/broadcom/bnx2.c
> > > > @@ -6246,7 +6246,16 @@ static int
> > > >  bnx2_setup_int_mode(struct bnx2 *bp, int dis_msi)
> > > >  {
> > > >  	int cpus = num_online_cpus();
> > > > -	int msix_vecs = min(cpus + 1, RX_MAX_RINGS);
> > > > +	int msix_vecs;
> > > > +
> > > > +	if (!bp->num_req_rx_rings)
> > > > +		msix_vecs = max(cpus + 1, bp->num_req_tx_rings);
> > > > +	else if (!bp->num_req_tx_rings)
> > > > +		msix_vecs = max(cpus, bp->num_req_rx_rings);
> > > > +	else
> > > > +		msix_vecs = max(bp->num_req_rx_rings, bp->num_req_tx_rings);
> > > > +
> > > > +	msix_vecs = min(msix_vecs, RX_MAX_RINGS);
> > > 
> > > If I read this correctly, IRQs may be shared between RX and TX queues
> > > i.e. there may be 'combined channels'.
> > 
> > It is true that an IRQ can have a TX and a RX queue, but they don't both
> > have to be enabled.  Because of that, it is easier to treat them as
> > independent queues.  They are independent in all aspects except the IRQ.
> 
> Given that these numbers can be set independently, I can see that
> treating TX and RX queues as having separate sets of channels might make
> the set_channels operation easier to understand.
> 
> The kernel-doc actually committed for struct ethtool_channels is not at
> all clear on what is meant by a 'channel', but it was certainly my
> intent that a channel should correspond to one IRQ and the total number
> of IRQs used by a device would be equal to the sum of
> {rx,tx,other,combined}_count.  Which is certainly not the case for the
> implementation in bnx2.

It doesn't seem to be true for the original implementation in qlcnic
either.  That has 1 IRQ shared between RX and TX, with additional IRQs
for RX only, but it reports them as all separate.  (It also seems to
report the number of TX channels to be what the firmware reports as the
maximum, despite the fact the driver only uses 1.)

There really should be some way to report, and potentially change,
whether IRQs are shared between RX and TX queues.  I wonder if it isn't
too late to rename and redefine the max_combined and combined_count
fields...

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.