RE: [PATCH net-next v3 08/12] net: ethernet: oa_tc6: implement transmit path to transfer tx ethernet frames

[Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>]

> -----Original Message-----
> From: Parthiban.Veerasooran@microchip.com
> <Parthiban.Veerasooran@microchip.com>
> Sent: Wednesday, March 20, 2024 3:43 AM
> To: andrew@lunn.ch
> Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; horms@kernel.org; saeedm@nvidia.com;
> anthony.l.nguyen@intel.com; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; corbet@lwn.net; linux-doc@vger.kernel.org;
> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> conor+dt@kernel.org; devicetree@vger.kernel.org;
> Horatiu.Vultur@microchip.com; ruanjinjie@huawei.com;
> Steen.Hegelund@microchip.com; vladimir.oltean@nxp.com;
> UNGLinuxDriver@microchip.com; Thorsten.Kummermehr@microchip.com;
> Piergiorgio Beruto <Pier.Beruto@onsemi.com>; Selvamani Rajagopal
> <Selvamani.Rajagopal@onsemi.com>; Nicolas.Ferre@microchip.com;
> benjamin.bigler@bernformulastudent.ch
> Subject: Re: [PATCH net-next v3 08/12] net: ethernet: oa_tc6: implement
> transmit path to transfer tx ethernet frames
> 
> [External Email]: This email arrived from an external source - Please exercise
> caution when opening any attachments or clicking on links.
> 
> Hi Andrew,
> 
> On 19/03/24 6:49 pm, Andrew Lunn wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know
> > the content is safe
> >
> > On Tue, Mar 19, 2024 at 12:54:30PM +0000,
> Parthiban.Veerasooran@microchip.com wrote:
> >> Hi Andrew,
> >>
> >> On 07/03/24 10:38 pm, Andrew Lunn wrote:
> >>> EXTERNAL EMAIL: Do not click links or open attachments unless you
> >>> know the content is safe
> >>>
> >>>> @@ -55,6 +77,14 @@
> >>>>                                                 (OA_TC6_CTRL_MAX_REGISTERS *\
> >>>>                                                 OA_TC6_CTRL_REG_VALUE_SIZE) +\
> >>>>
> >>>> OA_TC6_CTRL_IGNORED_SIZE)
> >>>> +#define OA_TC6_CHUNK_PAYLOAD_SIZE            64
> >>>> +#define OA_TC6_DATA_HEADER_SIZE                      4
> >>>> +#define OA_TC6_CHUNK_SIZE                    (OA_TC6_DATA_HEADER_SIZE
> +\
> >>>> +                                             OA_TC6_CHUNK_PAYLOAD_SIZE)
> >>>> +#define OA_TC6_TX_SKB_QUEUE_SIZE             100
> >>>
> >>> So you keep up to 100 packets in a queue. If use assume typical MTU
> >>> size packets, that is 1,238,400 bits. At 10Mbps, that is 120ms of
> >>> traffic. That is quite a lot of latency when a high priority packet
> >>> is added to the tail of the queue and needs to wait for all the
> >>> other packets to be sent first.
> >>>
> >>> Chunks are 64 bytes. So in practice, you only ever need two packets.
> >>> You need to be able to fill a chunk with the final part of one
> >>> packet, and the beginning of the next. So i would try using a much
> >>> smaller queue size. That will allow Linux queue disciplines to give
> >>> you the high priority packets first which you send with low latency.
> >> Thanks for the detailed explanation. If I understand you correctly,
> >>
> >> 1. The tx skb queue size (OA_TC6_TX_SKB_QUEUE_SIZE) should be 2 to
> >> avoid the latency when a high priority packet added.
> >>
> >> 2. Need to implement the handling part of the below case, In case if
> >> one packet ends in a chunk and that chunk still having some space
> >> left to accommodate some bytes from the next packet if available from
> >> network layer.
> >
> > This second part is clearly an optimisation. If you have lots of full
> > MTU packets, 1514 bytes, they take around 24 chunks. Having the last
> > chunk only 1/2 full does not waste too much bandwidth. But if you are
> > carrying lots of small packets, say voice, 130 bytes, the wasted
> > bandwidth starts to add up. But is there a use case for 10Mbps of
> > small packets? I doubt it.
> Yes, for sure there is a possibility to get into this scenario and the protocol also
> supports that. But as proposed by you below, let's implement it as part of
> optimization later.
> >
> > So if you don't have the ability to combine two packets into one
> > chunk, i would do that later. Lets get the basics merged first, it can
> > be optimised later.
> Yes, I agree with this proposal to get the basic version merged first.

While latency is important, so is using the available bandwidth efficiently. Here is a suggestion.  We know that the tx credit available basically tells us,
how many chunks could be transmitted without overflow. Instead of stopping the netif queue based on number of skbs queued, why not stop the queue based on
number of bytes accumulated? Basically, at any given point of time, we enqueue the tx_skb_q until we are have enough bytes to cross the threshold of (tc6->tc_credit * OA_TC6_CHUNK_PAYLOAD_SIZE).
This way, during the next transmit, we could utilize the whole available credits. Bandwidth utilization between bigger frames and smaller frames would be not be vastly different.

> 
> Best regards,
> Parthiban V
> >
> >          Andrew