lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <PH0PR18MB47344FB387047F03B2D74A08C7A6A@PH0PR18MB4734.namprd18.prod.outlook.com> Date: Thu, 2 Nov 2023 13:24:26 +0000 From: Shinas Rasheed <srasheed@...vell.com> To: David Laight <David.Laight@...LAB.COM>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org> CC: Haseeb Gani <hgani@...vell.com>, Vimlesh Kumar <vimleshk@...vell.com>, "egallen@...hat.com" <egallen@...hat.com>, "mschmidt@...hat.com" <mschmidt@...hat.com>, "pabeni@...hat.com" <pabeni@...hat.com>, "horms@...nel.org" <horms@...nel.org>, "kuba@...nel.org" <kuba@...nel.org>, "davem@...emloft.net" <davem@...emloft.net>, "wizhao@...hat.com" <wizhao@...hat.com>, "konguyen@...hat.com" <konguyen@...hat.com>, Veerasenareddy Burru <vburru@...vell.com>, Sathesh B Edara <sedara@...vell.com>, Eric Dumazet <edumazet@...gle.com> Subject: RE: [PATCH net-next v2 3/4] octeon_ep: implement xmit_more in transmit Hi David, > -----Original Message----- > From: David Laight <David.Laight@...LAB.COM> > Sent: Monday, October 30, 2023 9:00 PM > To: Shinas Rasheed <srasheed@...vell.com>; netdev@...r.kernel.org; > linux-kernel@...r.kernel.org > > > > - /* Ring Doorbell to notify the NIC there is a new packet */ > > > > - writel(1, iq->doorbell_reg); > > > > - iq->stats.instr_posted++; > > > > + /* Ring Doorbell to notify the NIC of new packets */ > > > > + writel(iq->fill_cnt, iq->doorbell_reg); > > > > + iq->stats.instr_posted += iq->fill_cnt; > > > > + iq->fill_cnt = 0; > > > > return NETDEV_TX_OK; > > > > > > Does that really need the count? > > > A 'doorbell' register usually just tells the MAC engine > > > to go and look at the transmit ring. > > > It then continues to process transmits until it fails > > > to find a packet. > > > So if the transmit is active you don't need to set the bit. > > > (Although that is actually rather hard to detect.) > > > > The way the octeon hardware works is that it expects number of newly > updated packets > > to be written to the doorbell register,which effectively increments the > doorbell > > count which shall be decremented by hardware as it reads these packets. > So in essence, > > the doorbell count also indicates outstanding packets to be read by > hardware. > > Unusual - I wouldn't call that a doorbell register. I double checked in earlier implementations as well as the reference manuals. This is how the hardware register is prescribed to be used. > If you do writes for every packet then the hardware can get on with > sending the first packet and might be able to do bulk reads > for the next packet(s) when that finishes. > > The extra code you are adding could easily (waving hands) > be more expensive than the posted PCIe write. > (Especially if you have to add an atomic operation.) > > Unless, of course, you have to wait for it to send that batch > of packets before you can give it any more. > Which would be rather entirely broken and would really require > you do the write in the end-of-transit path. The atomic operation is replaced in the very next patch. Other than that, what is it that you suggest we should 'fix' in this implementation of handling xmit_more? > The loop is something like: > while (get_packet()) { > per_cpu->xmit_more = !queue_empty(); > if (transmit_packet() != TX_OK) > break; > } > So if a packet is added while all the transmit setup code is running > it isn't detected. > I managed to repeatedly get that to loop when xmit_more wasn't set > and in a driver where the 'doorbell' write wasn't entirely trivial. How are you suggesting we handle or diagnose such a case, in the driver? Can you provide an example reference which handles adequately this special case? When the net-next opens again, I can submit the patches again with the added changes you suggested. Thanks for reviewing! Shinas
Powered by blists - more mailing lists