From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Hutchings Subject: Re: [PATCH 1/3 net-next] bnx2: Add support for ethtool --show-channels|--set-channels Date: Tue, 7 Feb 2012 22:36:27 +0000 Message-ID: <1328654187.3549.40.camel@bwh-desktop> References: <1328491480-13030-1-git-send-email-mchan@broadcom.com> <1328645954.3549.17.camel@bwh-desktop> <1328648307.8014.35.camel@nseg_linux_HP1.broadcom.com> <1328652088.3549.30.camel@bwh-desktop> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Cc: , To: Michael Chan Return-path: Received: from exchange.solarflare.com ([216.237.3.220]:17676 "EHLO ocex02.SolarFlarecom.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1756699Ab2BGWgb (ORCPT ); Tue, 7 Feb 2012 17:36:31 -0500 In-Reply-To: <1328652088.3549.30.camel@bwh-desktop> Sender: netdev-owner@vger.kernel.org List-ID: 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 > > > > --- > > > > 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.