LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Haiyang Zhang <haiyangz@microsoft.com>
To: Dexuan Cui <decui@microsoft.com>,
	"linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>
Cc: KY Srinivasan <kys@microsoft.com>,
	Stephen Hemminger <sthemmin@microsoft.com>,
	Paul Rosswurm <paulros@microsoft.com>,
	Shachar Raindel <shacharr@microsoft.com>,
	"olaf@aepfle.de" <olaf@aepfle.de>, vkuznets <vkuznets@redhat.com>,
	"davem@davemloft.net" <davem@davemloft.net>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH net-next] mana: Add support for EQ sharing
Date: Sat, 21 Aug 2021 21:17:55 +0000	[thread overview]
Message-ID: <MN2PR21MB1295573318B3897A2039B094CAC29@MN2PR21MB1295.namprd21.prod.outlook.com> (raw)
In-Reply-To: <BYAPR21MB12708078CCAD0B60EAA1508BBFC29@BYAPR21MB1270.namprd21.prod.outlook.com>



> -----Original Message-----
> From: Dexuan Cui <decui@microsoft.com>
> Sent: Friday, August 20, 2021 8:33 PM
> To: Haiyang Zhang <haiyangz@microsoft.com>; linux-hyperv@vger.kernel.org;
> netdev@vger.kernel.org
> Cc: Haiyang Zhang <haiyangz@microsoft.com>; KY Srinivasan
> <kys@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>; Paul
> Rosswurm <paulros@microsoft.com>; Shachar Raindel
> <shacharr@microsoft.com>; olaf@aepfle.de; vkuznets <vkuznets@redhat.com>;
> davem@davemloft.net; linux-kernel@vger.kernel.org
> Subject: RE: [PATCH net-next] mana: Add support for EQ sharing
> 
> > Subject: [PATCH net-next] mana: Add support for EQ sharing
> 
> "mana:" --> "net: mana:"
Will do.

> 
> > The existing code uses (1 + #vPorts * #Queues) MSIXs, which may exceed
> > the device limit.
> >
> > Support EQ sharing, so that multiple vPorts can share the same set of
> > MSIXs.
> >
> > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> 
> The patch itself looks good to me, but IMO the changes are too big to be
> in one patch. :-) Can you please split it into some smaller ones and
> please document the important changes in the commit messages, e.g.
Will do.

> 1) move NAPI processing from EQ to CQ.
> 
> 2) report the EQ-sharing capability bit to the host, which means the
> host can potentially offer more vPorts and queues to the VM.
> 
> 3) support up to 256 virtual ports (it was 16).
> 
> 4) support up to 64 queues per net interface (it was 16). It looks like
> the default number of queues is also 64 if the VM has >=64 CPUs? --
> should we add a new field apc->default_queues and limit it to 16 or 32?
> We'd like to make sure typically the best performance can be achieved
> with the default number of queues.
I found on a 40 cpu VM, the mana_query_vport_cfg() returns max_txq:32, 
max_rxq:32, so I didn't further reduce the number (32) from PF. 

That's also the opinion from the host team -- if they upgrade the NIC 
HW in the future, they can adjust the setting from PF side without 
requiring VF driver change.


> 
> 5) If the VM has >=64 CPUs, with the patch we create 1 HWC EQ and 64 NIC
> EQs, and IMO the creation of the last NIC EQ fails since now the host PF
> driver allows only 64 MSI-X interrupts? If this is the case, I think
> mana_probe() -> mana_create_eq() fails and no net interface will be
> created. It looks like we should create up to 63 NIC EQs in this case,
> and make sure we don't create too many SQs/RQs accordingly.
> 
> At the end of mana_gd_query_max_resources(), should we add something
> like:
> 	if (gc->max_num_queues >= gc->num_msix_usable -1)
> 		gc->max_num_queues = gc->num_msix_usable -1;
As said, the PF allows 32 queues, and 64 MSI-X interrupts for now. 
The PF should increase the MSI-X limit if the #queues is increased to 
64+. 

But for robustness, I like your idea that add a check in VF like above.


> 
> 6) Since we support up to 256 ports, up to 64 NIC EQs and up to
> 64 SQ CQs and 64 RQ CQs per port, the size of one EQ should be at least
> 256*2*GDMA_EQE_SIZE = 256*2*16 = 8KB. Currently EQ_SIZE is hardcoded to
> 8 pages (i.e. 32 KB on x86-64), which should be big enough. Let's add
> the below just in case we support more ports in future:
> 
> BUILD_BUG_ON(MAX_PORTS_IN_MANA_DEV*2* GDMA_EQE_SIZE > EQ_SIZE);
Will do.

> 
> 7) In mana_gd_read_cqe(), can we add a WARN_ON_ONCE() in the case of
> overflow. Currently the error (which normally should not happen) is
> sliently ignored.
Will do.

Thank you for the detailed reviews!

- Haiyang


  reply	other threads:[~2021-08-21 21:18 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-20 20:42 Haiyang Zhang
2021-08-21  0:32 ` Dexuan Cui
2021-08-21 21:17   ` Haiyang Zhang [this message]
2021-08-24  1:38     ` Dexuan Cui

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=MN2PR21MB1295573318B3897A2039B094CAC29@MN2PR21MB1295.namprd21.prod.outlook.com \
    --to=haiyangz@microsoft.com \
    --cc=davem@davemloft.net \
    --cc=decui@microsoft.com \
    --cc=kys@microsoft.com \
    --cc=linux-hyperv@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=olaf@aepfle.de \
    --cc=paulros@microsoft.com \
    --cc=shacharr@microsoft.com \
    --cc=sthemmin@microsoft.com \
    --cc=vkuznets@redhat.com \
    --subject='RE: [PATCH net-next] mana: Add support for EQ sharing' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).