RE: [PATCH net-next v2 3/4] octeon_ep: implement xmit_more in transmit

[Shinas Rasheed]

Hi David,

> -----Original Message-----
> From: David Laight <David.Laight@xxxxxxxxxx>
> Sent: Monday, October 30, 2023 9:00 PM
> To: Shinas Rasheed <srasheed@xxxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx
> > > > -	/* 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