Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
MIME-Version: 1.0
References: <20231024145119.2366588-1-srasheed@marvell.com>
 <20231024145119.2366588-4-srasheed@marvell.com> <20231024172151.5fd1b29a@kernel.org>
 <PH0PR18MB473482180622D7C163B487ADC7DDA@PH0PR18MB4734.namprd18.prod.outlook.com>
In-Reply-To: <PH0PR18MB473482180622D7C163B487ADC7DDA@PH0PR18MB4734.namprd18.prod.outlook.com>
From:   Eric Dumazet <edumazet@google.com>
Date:   Thu, 26 Oct 2023 10:28:44 +0200
Message-ID: <CANn89iKAn0-KBr00qrVibeM9Y7OqxDsg-keQQkty9oD1aCVtcA@mail.gmail.com>
Subject: Re: [EXT] Re: [PATCH net-next v2 3/4] octeon_ep: implement xmit_more
 in transmit
To:     Shinas Rasheed <srasheed@marvell.com>
Cc:     Jakub Kicinski <kuba@kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Haseeb Gani <hgani@marvell.com>,
        Vimlesh Kumar <vimleshk@marvell.com>,
        "egallen@redhat.com" <egallen@redhat.com>,
        "mschmidt@redhat.com" <mschmidt@redhat.com>,
        "pabeni@redhat.com" <pabeni@redhat.com>,
        "horms@kernel.org" <horms@kernel.org>,
        "davem@davemloft.net" <davem@davemloft.net>,
        "wizhao@redhat.com" <wizhao@redhat.com>,
        "konguyen@redhat.com" <konguyen@redhat.com>,
        Veerasenareddy Burru <vburru@marvell.com>,
        Sathesh B Edara <sedara@marvell.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Oct 26, 2023 at 9:58=E2=80=AFAM Shinas Rasheed <srasheed@marvell.co=
m> 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 u=
ses 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, a=
s hardware can pick up SG frags from separate memory and doesn't require se=
parate descriptors.
>
> >If so - consider stopping stopping the queue when the condition is not t=
rue.
> >> We do stop the queue if tx queue is full, as in octep_iq_full_check ea=
rlier 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=3Dhttps-3A__www.kernel.org_doc=
_html_next_networking_driver.html-23transmit-2Dpath-2Dguidelines&d=3DDwICAg=
&c=3DnKjWec2b6R0mOyPaz7xtfQ&r=3D1OxLD4y-oxrlgQ1rjXgWtmLz1pnaDjD96sDq-cKUwK4=
&m=3DFyJHTb5Z2u9DTFSYPU38S5kPcP5KvwGWzY-DPcqOl1gdnm7ToZhTFpyvhLMqh1hd&s=3Dd=
BMmwfWKAi0UH3nrz7j9kYnAodDjuN3LZ5tC2aL_Prs&e=3D
>
> >> In the skeleton code above, as I understand each tx desc holds a skb f=
rag and hence there can be possibility of receiving a full-sized skb, stopp=
ing the queue but on receiving another normal skb we should observe our que=
ue 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 rea=
d) tx descriptors are processed in napi context and queues awoken.
>
> Please correct me if I'm wrong anywhere (sorry if so) to further my under=
standing, 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 earl=
ier.