[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
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