netdev - Re: [PATCH net-next] net: lan743x: Fix the multi queue overflow issue

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:   Wed, 7 Sep 2022 11:47:49 +0530
From:   Raju Lakkaraju <Raju.Lakkaraju@...rochip.com>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     <netdev@...r.kernel.org>, <davem@...emloft.net>,
        <linux-kernel@...r.kernel.org>, <bryan.whitehead@...rochip.com>,
        <richardcochran@...il.com>, <UNGLinuxDriver@...rochip.com>,
        <Ian.Saturley@...rochip.com>
Subject: Re: [PATCH net-next] net: lan743x: Fix the multi queue overflow issue

Hi Jakub,

Thank you for review comments.
I accepted all your comments.
I will fix in V1 patch series and also change the subject line to 
"net: lan743x: Fix to use multiqueue start/stop APIs"

Thanks,
Raju
The 08/10/2022 22:35, Jakub Kicinski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Tue, 9 Aug 2022 14:06:28 +0530 Raju Lakkaraju wrote:
> > Fix the Tx multi-queue overflow issue
> >
> > Tx ring size of 128 (for TCP) provides ability to handle way more data than
> > what Rx can (Rx buffers are constrained to one frame or even less during a
> > dynamic mtu size change)
> >
> > TX napi weight dependent of the ring size like it is now (ring size -1)
> > because there is an express warning in the kernel about not registering weight
> > values > NAPI_POLL_WEIGHT (currently 64)
> 
> I've read this message 3 times, I don't understand what you're saying.
> Could you please rewrite it and add necessary details?
> 
> > diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
> > index a9a1dea6d731..d7c14ee7e413 100644
> > --- a/drivers/net/ethernet/microchip/lan743x_main.c
> > +++ b/drivers/net/ethernet/microchip/lan743x_main.c
> > @@ -2064,8 +2064,10 @@ static void lan743x_tx_frame_end(struct lan743x_tx *tx,
> >  static netdev_tx_t lan743x_tx_xmit_frame(struct lan743x_tx *tx,
> >                                        struct sk_buff *skb)
> >  {
> > +     struct lan743x_adapter *adapter = tx->adapter;
> >       int required_number_of_descriptors = 0;
> >       unsigned int start_frame_length = 0;
> > +     netdev_tx_t retval = NETDEV_TX_OK;
> >       unsigned int frame_length = 0;
> >       unsigned int head_length = 0;
> >       unsigned long irq_flags = 0;
> > @@ -2083,9 +2085,13 @@ static netdev_tx_t lan743x_tx_xmit_frame(struct lan743x_tx *tx,
> >               if (required_number_of_descriptors > (tx->ring_size - 1)) {
> >                       dev_kfree_skb_irq(skb);
> >               } else {
> > -                     /* save to overflow buffer */
> > -                     tx->overflow_skb = skb;
> > -                     netif_stop_queue(tx->adapter->netdev);
> > +                     /* save how many descriptors we needed to restart the queue */
> > +                     tx->rqd_descriptors = required_number_of_descriptors;
> > +                     retval = NETDEV_TX_BUSY;
> > +                     if (is_pci11x1x_chip(adapter))
> > +                             netif_stop_subqueue(adapter->netdev, tx->channel_number);
> 
> Is tx->channel_number not 0 for devices other than pci11x1x ?
> netif_stop_queue() is just an alias for queue 0 IIRC so
> you can save all the ifs, most likely?
> 
Yes.
I will fix.

> > +                     else
> > +                             netif_stop_queue(adapter->netdev);
> >               }
> >               goto unlock;
> >       }
> > @@ -2093,7 +2099,7 @@ static netdev_tx_t lan743x_tx_xmit_frame(struct lan743x_tx *tx,
> >       /* space available, transmit skb  */
> >       if ((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
> >           (tx->ts_flags & TX_TS_FLAG_TIMESTAMPING_ENABLED) &&
> > -         (lan743x_ptp_request_tx_timestamp(tx->adapter))) {
> > +         (lan743x_ptp_request_tx_timestamp(adapter))) {
> 
> If this is a fix you should hold off on refactoring like adding the
> local variable for adapter to make backports easier.
> 

Accepted.
I will fix.

> >               skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> >               do_timestamp = true;
> >               if (tx->ts_flags & TX_TS_FLAG_ONE_STEP_SYNC)
> 
> > @@ -1110,7 +1109,7 @@ struct lan743x_tx_buffer_info {
> >       unsigned int    buffer_length;
> >  };
> >
> > -#define LAN743X_TX_RING_SIZE    (50)
> > +#define LAN743X_TX_RING_SIZE    (128)
> 
> So the ring size is getting increased? I did not get that from the
> commit message at all :S

-- 
--------
Thanks,
Raju