netdev - RE: [PATCH net-next v2 3/4] octeon_ep: implement xmit

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

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