Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <1544533009-12425-1-git-send-email-mw@semihalf.com>
 <20181212104304.49026ed2@xhacker.debian> <CAPv3WKctDNJVq7SgHBZLO_TU50ponCLu7h5oMKLTndBiDadhaA@mail.gmail.com>
 <20181212172006.0814cf03@xhacker.debian>
In-Reply-To: <20181212172006.0814cf03@xhacker.debian>
From:   Marcin Wojtas <mw@semihalf.com>
Date:   Wed, 12 Dec 2018 12:04:58 +0100
Message-ID: <CAPv3WKd71XBeUo2d7199f-LNs3ZVKOMBDBB69Mtufy_Qr_w81Q@mail.gmail.com>
Subject: Re: [PATCH net] net: mvneta: fix operation for 64K PAGE_SIZE
To:     Jisheng.Zhang@synaptics.com
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-arm-kernel@lists.infradead.org,
        netdev <netdev@vger.kernel.org>,
        Antoine Tenart <antoine.tenart@bootlin.com>,
        Grzegorz Jaszczyk <jaz@semihalf.com>,
        =?UTF-8?Q?Gr=C3=A9gory_Clement?= <gregory.clement@bootlin.com>,
        Russell King - ARM Linux <linux@armlinux.org.uk>,
        Maxime Chevallier <maxime.chevallier@bootlin.com>,
        nadavh@marvell.com,
        Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
        Stefan Chulski <stefanc@marvell.com>,
        "David S. Miller" <davem@davemloft.net>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Jisheng,

=C5=9Br., 12 gru 2018 o 10:25 Jisheng Zhang <Jisheng.Zhang@synaptics.com> n=
apisa=C5=82(a):
>
> Hi Marcin,
>
> On Wed, 12 Dec 2018 09:22:57 +0100 Marcin Wojtas <mw@semihalf.com> wrote:
>
> > Hi Jisheng,
> >
> > =C5=9Br., 12 gru 2018 o 03:48 Jisheng Zhang <Jisheng.Zhang@synaptics.co=
m> napisa=C5=82(a):
> > >
> > > Hi,
> > >
> > > On Tue, 11 Dec 2018 13:56:49 +0100 Marcin Wojtas wrote:
> > >
> > > > Recent changes in the mvneta driver reworked allocation
> > > > and handling of the ingress buffers to use entire pages.
> > > > Apart from that in SW BM scenario the HW must be informed
> > > > via PRXDQS about the biggest possible incoming buffer
> > > > that can be propagated by RX descriptors.
> > > >
> > > > The BufferSize field was filled according to the MTU-dependent
> > > > pkt_size value. Later change to PAGE_SIZE broke RX operation
> > > > when usin 64K pages, as the field is simply too small.
> > > >
> > > > This patch conditionally limits the value passed to the BufferSize
> > > > of the PRXDQS register, depending on the PAGE_SIZE used.
> > > > On the occasion remove now unused frag_size field of the mvneta_por=
t
> > > > structure.
> > > >
> > > > Fixes: 562e2f467e71 ("net: mvneta: Improve the buffer allocation
> > > > method for SWBM")
> > >
> > > IMHO, we'd better revert 562e2f467e71 and 7e47fd84b56bb
> > >
> > > The issue commit 562e2f467e71 wants to solve is due to commit 7e47fd8=
4b56bb
> > > It looks a bit wired, to introduce regression then submit another com=
mit(in
> > > the same patch set) solve it
> > >
> > > Per my test, after reverting 562e2f467e71 and 7e47fd84b56bb, I can't =
reproduce
> > > what's claimed in commit 562e2f467e71 -- "With system having a small =
memory
> > > (around 256MB), the state "cannot allocate memory to refill with new =
buffer"
> > > is reach pretty quickly."
> >
> > I am not the one to decide about patch reverting. From what I
> > understand, commit 7e47fd84b56bb was intorduced in order to increase
> > performance thanks to replacing mvneta_frag_alloc/free with using
> > entire pages for RX buffers. I have 2 questions:
> > - without reverting anything, do you observe memory allocation
> > problems during refill?
>
> I see memory waste: For normal 1500 MTU, before commit 7e47fd84b56bb we
> allocate 1920Bytes for rx. After commit 7e47fd84b56bb, we always allocate
> PAGE_SIZE bytes, if PAGE_SIZE=3D4096, we waste 53% memory for each rx buf=
.
>
> > - are you able to check L2 forwarding numbers on top of the pure
> > mainline branch and after reverting the mentioned patches? I'm
> > wondering what would be the performance penalty (if any).
>
> I didn't have the numbers. IMHO, when the performance number should
> be put into the commit msg when introducing commit 7e47fd84b56bb.
>

In general I agree with you about the memory waste and lack of numbers
backing the 7e47fd84b56bb change. However the improved refill
mechanism from 562e2f467e71 is something IMO worth to keep, so simple
reverts may not be the best idea. We should focus on dropping the full
page per descriptor dependency - I want to do it, but since it's a
slightly bigger rework, I cannot promise it will happen fast.

Best regards,
Marcin