lkml.org 
[lkml]   [2023]   [Nov]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH net-next v2 3/4] octeon_ep: implement xmit_more in transmit
Date

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

\
 
 \ /
  Last update: 2023-11-02 14:25    [W:0.233 / U:0.148 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site