> -----Original Message----- > From: Dexuan Cui <decui@xxxxxxxxxxxxx> > Sent: Friday, August 20, 2021 8:33 PM > To: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>; linux-hyperv@xxxxxxxxxxxxxxx; > netdev@xxxxxxxxxxxxxxx > Cc: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>; KY Srinivasan > <kys@xxxxxxxxxxxxx>; Stephen Hemminger <sthemmin@xxxxxxxxxxxxx>; Paul > Rosswurm <paulros@xxxxxxxxxxxxx>; Shachar Raindel > <shacharr@xxxxxxxxxxxxx>; olaf@xxxxxxxxx; vkuznets <vkuznets@xxxxxxxxxx>; > davem@xxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx > 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@xxxxxxxxxxxxx> > > 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