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

  Add xmit_more handling in tx datapath for octeon_ep pf.

Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
---
V2:
  - Updated changelog to have imperative tone.
V1: https://lore.kernel.org/all/20231023114449.2362147-3-srasheed@marvell.com/

 .../ethernet/marvell/octeon_ep/octep_config.h |  2 +-
 .../ethernet/marvell/octeon_ep/octep_main.c   | 19 +++++++++++++++----
 2 files changed, 16 insertions(+), 5 deletions(-)
  

On Tue, 24 Oct 2023 07:51:18 -0700 Shinas Rasheed wrote:
>  	iq->host_write_index = wi;
> +	if (xmit_more &&
> +	    (atomic_read(&iq->instr_pending) <
> +	     (iq->max_count - OCTEP_WAKE_QUEUE_THRESHOLD)) &&
> +	    iq->fill_cnt < iq->fill_threshold)
> +		return NETDEV_TX_OK;

Does this guarantee that a full-sized skb can be accommodated?
If so - consider stopping stopping the queue when the condition
is not true. The recommended way of implementing 'driver flow control'
is to stop the queue once next packet may not fit, and then use
netif_xmit_stopped() when deciding whether we need to flush or we can
trust xmit_more. see 
https://www.kernel.org/doc/html/next/networking/driver.html#transmit-path-guidelines

>  	/* Flush the hw descriptor before writing to doorbell */
>  	wmb();
> -
> -	/* 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;
  

Hi Jakub, 

Again, thanks for your review.

> Does this guarantee that a full-sized skb can be accommodated?
>> I assume by full-sized skb you mean a non-linear skb with MAX_SG_FRAGS elements in frags array.  Yes, this can be accommodated and the hardware uses separate SG list memory to siphon these skb frags instead of obtaining them from the same tx hardware queue. What I'm trying to say is, this means that a single tx descriptor will be enough for a full-sized skb as well, as hardware can pick up SG frags from separate memory and doesn't require separate descriptors.

>If so - consider stopping stopping the queue when the condition is not true.
>> We do stop the queue if tx queue is full, as in octep_iq_full_check earlier on.

>The recommended way of implementing 'driver flow control'
is to stop the queue once next packet may not fit, and then use
netif_xmit_stopped() when deciding whether we need to flush or we can
trust xmit_more. see 
https://urldefense.proofpoint.com/v2/url?u=https-3A__www.kernel.org_doc_html_next_networking_driver.html-23transmit-2Dpath-2Dguidelines&d=DwICAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=1OxLD4y-oxrlgQ1rjXgWtmLz1pnaDjD96sDq-cKUwK4&m=FyJHTb5Z2u9DTFSYPU38S5kPcP5KvwGWzY-DPcqOl1gdnm7ToZhTFpyvhLMqh1hd&s=dBMmwfWKAi0UH3nrz7j9kYnAodDjuN3LZ5tC2aL_Prs&e= 

>> In the skeleton code above, as I understand each tx desc holds a skb frag and hence there can be possibility of receiving a full-sized skb, stopping the queue but on receiving another normal skb we should observe our queue to be stopped. This doesn't arise in our case as even if the skb is full-sized, it will only use a single tx descriptor so we can be sure if queue has been stopped, the write index will only be updated once posted (and read) tx descriptors are processed in napi context and queues awoken.

Please correct me if I'm wrong anywhere (sorry if so) to further my understanding, and again thanks for your time!
  

On Thu, Oct 26, 2023 at 9:58 AM Shinas Rasheed <srasheed@marvell.com> wrote:
>
> Hi Jakub,
>
> Again, thanks for your review.
>
> > Does this guarantee that a full-sized skb can be accommodated?
> >> I assume by full-sized skb you mean a non-linear skb with MAX_SG_FRAGS elements in frags array.  Yes, this can be accommodated and the hardware uses separate SG list memory to siphon these skb frags instead of obtaining them from the same tx hardware queue. What I'm trying to say is, this means that a single tx descriptor will be enough for a full-sized skb as well, as hardware can pick up SG frags from separate memory and doesn't require separate descriptors.
>
> >If so - consider stopping stopping the queue when the condition is not true.
> >> We do stop the queue if tx queue is full, as in octep_iq_full_check earlier on.
>
> >The recommended way of implementing 'driver flow control'
> is to stop the queue once next packet may not fit, and then use
> netif_xmit_stopped() when deciding whether we need to flush or we can
> trust xmit_more. see
> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.kernel.org_doc_html_next_networking_driver.html-23transmit-2Dpath-2Dguidelines&d=DwICAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=1OxLD4y-oxrlgQ1rjXgWtmLz1pnaDjD96sDq-cKUwK4&m=FyJHTb5Z2u9DTFSYPU38S5kPcP5KvwGWzY-DPcqOl1gdnm7ToZhTFpyvhLMqh1hd&s=dBMmwfWKAi0UH3nrz7j9kYnAodDjuN3LZ5tC2aL_Prs&e=
>
> >> In the skeleton code above, as I understand each tx desc holds a skb frag and hence there can be possibility of receiving a full-sized skb, stopping the queue but on receiving another normal skb we should observe our queue to be stopped. This doesn't arise in our case as even if the skb is full-sized, it will only use a single tx descriptor so we can be sure if queue has been stopped, the write index will only be updated once posted (and read) tx descriptors are processed in napi context and queues awoken.
>
> Please correct me if I'm wrong anywhere (sorry if so) to further my understanding, and again thanks for your time!

Fact that octep_start_xmit() can return NETDEV_TX_BUSY is very suspicious.

I do not think a driver can implement xmit_more and keep
NETDEV_TX_BUSY at the same time.

Please make sure to remove NETDEV_TX_BUSY first, by stopping the queue earlier.
  

On Thu, 26 Oct 2023 07:57:54 +0000 Shinas Rasheed wrote:
> >The recommended way of implementing 'driver flow control'  
> is to stop the queue once next packet may not fit, and then use
> netif_xmit_stopped() when deciding whether we need to flush or we can
> trust xmit_more. see 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.kernel.org_doc_html_next_networking_driver.html-23transmit-2Dpath-2Dguidelines&d=DwICAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=1OxLD4y-oxrlgQ1rjXgWtmLz1pnaDjD96sDq-cKUwK4&m=FyJHTb5Z2u9DTFSYPU38S5kPcP5KvwGWzY-DPcqOl1gdnm7ToZhTFpyvhLMqh1hd&s=dBMmwfWKAi0UH3nrz7j9kYnAodDjuN3LZ5tC2aL_Prs&e= 
> 
> >> In the skeleton code above, as I understand each tx desc holds a skb frag and hence there can be possibility of receiving a full-sized skb, stopping the queue but on receiving another normal skb we should observe our queue to be stopped. This doesn't arise in our case as even if the skb is full-sized, it will only use a single tx descriptor so we can be sure if queue has been stopped, the write index will only be updated once posted (and read) tx descriptors are processed in napi context and queues awoken.  
> 
> Please correct me if I'm wrong anywhere (sorry if so) to further my understanding, and again thanks for your time!

Could you please do some training on how to use normal / mailing list
style email at Marvell? Multiple people from Marvell can't quote
replies correctly, it makes it hard to have a conversation and help
y'all.
  

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Thursday, October 26, 2023 8:15 PM
> To: Shinas Rasheed <srasheed@marvell.com>
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Haseeb Gani
> <hgani@marvell.com>; Vimlesh Kumar <vimleshk@marvell.com>;
> egallen@redhat.com; mschmidt@redhat.com; pabeni@redhat.com;
> horms@kernel.org; davem@davemloft.net; wizhao@redhat.com;
> konguyen@redhat.com; Veerasenareddy Burru <vburru@marvell.com>;
> Sathesh B Edara <sedara@marvell.com>; Eric Dumazet
> <edumazet@google.com>
> Subject: Re: [EXT] Re: [PATCH net-next v2 3/4] octeon_ep: implement
> xmit_more in transmit
> 
> On Thu, 26 Oct 2023 07:57:54 +0000 Shinas Rasheed wrote:
> > >The recommended way of implementing 'driver flow control'
> > is to stop the queue once next packet may not fit, and then use
> > netif_xmit_stopped() when deciding whether we need to flush or we can
> > trust xmit_more. see
> > https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__www.kernel.org_doc_html_next_networking_driver.html-23transmit-
> 2Dpath-2Dguidelines&d=DwICAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=1OxLD4y-
> oxrlgQ1rjXgWtmLz1pnaDjD96sDq-
> cKUwK4&m=FyJHTb5Z2u9DTFSYPU38S5kPcP5KvwGWzY-
> DPcqOl1gdnm7ToZhTFpyvhLMqh1hd&s=dBMmwfWKAi0UH3nrz7j9kYnAodDj
> uN3LZ5tC2aL_Prs&e=
> >
> > >> In the skeleton code above, as I understand each tx desc holds a skb frag
> and hence there can be possibility of receiving a full-sized skb, stopping the
> queue but on receiving another normal skb we should observe our queue to
> be stopped. This doesn't arise in our case as even if the skb is full-sized, it will
> only use a single tx descriptor so we can be sure if queue has been stopped,
> the write index will only be updated once posted (and read) tx descriptors
> are processed in napi context and queues awoken.
> >
> > Please correct me if I'm wrong anywhere (sorry if so) to further my
> understanding, and again thanks for your time!
> 
> Could you please do some training on how to use normal / mailing list
> style email at Marvell? Multiple people from Marvell can't quote
> replies correctly, it makes it hard to have a conversation and help
> y'all.
Hi Jacub, apologizing for format errors on my part, will try to rectify in coming mails. Sorry again.


> -----Original Message-----
> From: Eric Dumazet <edumazet@google.com>
> Sent: Thursday, October 26, 2023 1:59 PM
> To: Shinas Rasheed <srasheed@marvell.com>
> Cc: Jakub Kicinski <kuba@kernel.org>; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; Haseeb Gani <hgani@marvell.com>; Vimlesh Kumar
> <vimleshk@marvell.com>; egallen@redhat.com; mschmidt@redhat.com;
> pabeni@redhat.com; horms@kernel.org; davem@davemloft.net;
> wizhao@redhat.com; konguyen@redhat.com; Veerasenareddy Burru
> <vburru@marvell.com>; Sathesh B Edara <sedara@marvell.com>
> Subject: Re: [EXT] Re: [PATCH net-next v2 3/4] octeon_ep: implement
> xmit_more in transmit
> 
> Fact that octep_start_xmit() can return NETDEV_TX_BUSY is very suspicious.
> 
> I do not think a driver can implement xmit_more and keep
> NETDEV_TX_BUSY at the same time.
> 
> Please make sure to remove NETDEV_TX_BUSY first, by stopping the queue
> earlier.

Hi Eric, I think I understand your point and shall submit an updated patch. Thanks your time!
  

From: Shinas Rasheed
> Sent: 24 October 2023 15:51
> 
> Add xmit_more handling in tx datapath for octeon_ep pf.
> 
...
> -
> -	/* 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 'xmit_more' flag is useful if (the equivalent of) writing
the doorbell register is expensive since it can be delayed
to a later frame and only done once - adding a slight latency
to the earlier transmits if the mac engine was idle.

I'm not sure how much (if any) performance gain you actually
get from avoiding the writel().
Single PCIe writes are 'posted' and pretty much completely
asynchronous.

The other problem I've seen is that netdev_xmit_more() is
the state of the queue when the transmit was started, not
the current state.
If a packet is added while the earlier transmit setup code
is running (setting up the descriptors etc) the it isn't set.
So the fast path doesn't get taken.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
  

Hi,

I understand the window is closed, but just replying to a pending comment on the thread.

> -----Original Message-----
> From: David Laight <David.Laight@ACULAB.COM>
> ----------------------------------------------------------------------
> From: Shinas Rasheed
> > Sent: 24 October 2023 15:51
> >
> > Add xmit_more handling in tx datapath for octeon_ep pf.
> >
> ...
> > -
> > -	/* 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.
 
> The 'xmit_more' flag is useful if (the equivalent of) writing
> the doorbell register is expensive since it can be delayed
> to a later frame and only done once - adding a slight latency
> to the earlier transmits if the mac engine was idle.
> 
> I'm not sure how much (if any) performance gain you actually
> get from avoiding the writel().
> Single PCIe writes are 'posted' and pretty much completely
> asynchronous.

Can you elaborate what you are suggesting here to do? The driver is trying to make use of the 'xmit_more'
hint from the network stack, as any network driver might opt to do. I think avoiding continuous PCIe posts for each packet
shall still be wasteful as the hardware can bulk read from the queue if we give it a batch of packets.

> The other problem I've seen is that netdev_xmit_more() is
> the state of the queue when the transmit was started, not
> the current state.
> If a packet is added while the earlier transmit setup code
> is running (setting up the descriptors etc) the it isn't set.
> So the fast path doesn't get taken.

By the next packet the kernel sends, the xmit_more should be set
as far I understand, right? (as the xmit_more bool is set if skb->next is present, if the transmit path follows dev_hard_start_xmit).
  

From: Shinas Rasheed <srasheed@marvell.com>
> Sent: 30 October 2023 14:15
> 
> Hi,
> 
> I understand the window is closed, but just replying to a pending comment on the thread.
> 
> > -----Original Message-----
> > From: David Laight <David.Laight@ACULAB.COM>
> > ----------------------------------------------------------------------
> > From: Shinas Rasheed
> > > Sent: 24 October 2023 15:51
> > >
> > > Add xmit_more handling in tx datapath for octeon_ep pf.
> > >
> > ...
> > > -
> > > -	/* 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.

> > The 'xmit_more' flag is useful if (the equivalent of) writing
> > the doorbell register is expensive since it can be delayed
> > to a later frame and only done once - adding a slight latency
> > to the earlier transmits if the mac engine was idle.
> >
> > I'm not sure how much (if any) performance gain you actually
> > get from avoiding the writel().
> > Single PCIe writes are 'posted' and pretty much completely
> > asynchronous.
> 
> Can you elaborate what you are suggesting here to do? The driver is trying
> to make use of the 'xmit_more' hint from the network stack, as any network
> driver might opt to do.

There are some drivers where waking up the MAC engine is expensive.
If you need to do a PCIe read then they are expensive.
There might also be drivers that need to send a USB message.
I don't actually know which one it was added for.

> I think avoiding continuous PCIe posts for each packet shall still be wasteful
> as the hardware can bulk read from the queue if we give it a batch of packets.

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 other problem I've seen is that netdev_xmit_more() is
> > the state of the queue when the transmit was started, not
> > the current state.
> > If a packet is added while the earlier transmit setup code
> > is running (setting up the descriptors etc) the it isn't set.
> > So the fast path doesn't get taken.
> 
> By the next packet the kernel sends, the xmit_more should be set
> as far I understand, right? (as the xmit_more bool is set if skb->next
> is present, if the transmit path follows dev_hard_start_xmit).

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.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
  

Hi David,

> -----Original Message-----
> From: David Laight <David.Laight@ACULAB.COM>
> Sent: Monday, October 30, 2023 9:00 PM
> To: Shinas Rasheed <srasheed@marvell.com>; netdev@vger.kernel.org;
> linux-kernel@vger.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