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

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.