DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 9DC0520831
MIME-Version: 1.0
References: <1540808552-28738-1-git-send-email-wright.feng@cypress.com>
 <1540808552-28738-2-git-send-email-wright.feng@cypress.com>
 <CA+8PC_f-EMCWnn1oBorAZjNY2xJXcJbhbP+8fU9wYJXnnZto5w@mail.gmail.com> <55fc2684-bd07-95ae-9193-0d383787ebf8@cypress.com>
In-Reply-To: <55fc2684-bd07-95ae-9193-0d383787ebf8@cypress.com>
From:   Franky Lin <franky.lin@broadcom.com>
Date:   Fri, 2 Nov 2018 12:51:54 -0700
Message-ID: <CA+8PC_fVWy=R1VPH4RVPgdTY6D0cA89B4L990F_Kt0Cevgt4+w@mail.gmail.com>
Subject: Re: [PATCH 1/3] brcmfmac: calling skb_orphan before sending skb to
 SDIO bus
To:     Wright Feng <Wright.Feng@cypress.com>
Cc:     "open list:BROADCOM BRCM80211 IEEE802.11n WIRELESS DRIVER" 
        <linux-wireless@vger.kernel.org>,
        Arend Van Spriel <arend.vanspriel@broadcom.com>,
        Hante Meuleman <hante.meuleman@broadcom.com>,
        Kalle Valo <kvalo@codeaurora.org>,
        Chi-Hsien Lin <Chi-Hsien.Lin@cypress.com>,
        brcm80211-dev-list.pdl@broadcom.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On Thu, Nov 1, 2018 at 8:08 PM Wright Feng <Wright.Feng@cypress.com> wrote:
>
>
>
> On 2018/10/30 =E4=B8=8A=E5=8D=88 02:50, Franky Lin wrote:
> > On Mon, Oct 29, 2018 at 3:27 AM Wright Feng <Wright.Feng@cypress.com> w=
rote:
> >>
> >> Linux 3.6 introduces TSQ which has a per socket threshold for TCP Tx
> >> packets to reduce latency. In fcmode 1 and fcmode 2, host driver enque=
ues
> >> skb in hanger and TCP doesn't push new skb until host frees the skb wh=
en
> >> receiving fwstatus event. So using skb_orphan before sending skb to bu=
s
> >> will make the skb removing the ownership of socket. With this patch, w=
e
> >> are able to get better throughput in fcmode 1 and fcmode 2.
> >>
> >> Tested 43455 TCP throughput in 20 MHz bandwidth with / without this pa=
tch.
> >> fcmode 0: 59.5 / 59.6 (mbps)
> >> fcmode 1: 59.3 / 23.4 (mbps)
> >> fcmode 2: 59.6 / 21.5 (mbps)
> >>
> >> Signed-off-by: Wright Feng <wright.feng@cypress.com>
> >> ---
> >>   drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 1 +
> >>   1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b=
/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> >> index b2e1ab5..519b25d 100644
> >> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> >> @@ -2298,6 +2298,7 @@ static uint brcmf_sdio_sendfromq(struct brcmf_sd=
io *bus, uint maxframes)
> >>                                                &prec_out);
> >>                          if (pkt =3D=3D NULL)
> >>                                  break;
> >> +                       skb_orphan(pkt);
> >
> > TSQ allows device driver to tweak for a deeper queue now. [1]. We
> > should use that instead of orphaning the packet before handing over to
> > firmware which would remove the bufferbloat protection by TSQ.
> >
> > Thanks,
> > -Franky
> >
> Since the aggregation problem has been fixed by setting sk_pacing_shift,
> I will submit v2 without this one.
> Thanks for the information.

No the problem is not fixed. The patch just provides an interface
sk_pacing_shift_update so the device driver can tweak the scale to
allow more packets being queued. You need to call
sk_pacing_shift_update somewhere in brcmfmac.

Thanks,
-Franky
>
> -Wright
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/=
commit/?id=3D3a9b76fd0db9f
> >
> >>                          __skb_queue_tail(&pktq, pkt);
> >>                  }
> >>                  spin_unlock_bh(&bus->txq_lock);
> >> --
> >> 1.9.1
> >>