netdev - Re: [PATCH] octeontx2-pf: Use the napi_alloc

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200508130115.GO208718@lunn.ch>
Date:   Fri, 8 May 2020 15:01:15 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Sunil Kovvuri <sunil.kovvuri@...il.com>
Cc:     Kevin Hao <haokexin@...il.com>,
        Linux Netdev List <netdev@...r.kernel.org>,
        Sunil Goutham <sgoutham@...vell.com>,
        Geetha sowjanya <gakula@...vell.com>,
        Subbaraya Sundeep <sbhatta@...vell.com>,
        hariprasad <hkelam@...vell.com>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH] octeontx2-pf: Use the napi_alloc_frag() to alloc the
 pool buffers

On Fri, May 08, 2020 at 01:08:13PM +0530, Sunil Kovvuri wrote:
> On Fri, May 8, 2020 at 11:00 AM Kevin Hao <haokexin@...il.com> wrote:
> >
> > On Fri, May 08, 2020 at 10:18:27AM +0530, Sunil Kovvuri wrote:
> > > On Fri, May 8, 2020 at 9:43 AM Kevin Hao <haokexin@...il.com> wrote:
> > > >
> > > > In the current codes, the octeontx2 uses its own method to allocate
> > > > the pool buffers, but there are some issues in this implementation.
> > > > 1. We have to run the otx2_get_page() for each allocation cycle and
> > > >    this is pretty error prone. As I can see there is no invocation
> > > >    of the otx2_get_page() in otx2_pool_refill_task(), this will leave
> > > >    the allocated pages have the wrong refcount and may be freed wrongly.
> > >
> > > Thanks for pointing, will fix.
> > >
> > > > 2. It wastes memory. For example, if we only receive one packet in a
> > > >    NAPI RX cycle, and then allocate a 2K buffer with otx2_alloc_rbuf()
> > > >    to refill the pool buffers and leave the remain area of the allocated
> > > >    page wasted. On a kernel with 64K page, 62K area is wasted.
> > > >
> > > > IMHO it is really unnecessary to implement our own method for the
> > > > buffers allocate, we can reuse the napi_alloc_frag() to simplify
> > > > our code.
> > > >
> > > > Signed-off-by: Kevin Hao <haokexin@...il.com>
> > >
> > > Have you measured performance with and without your patch ?
> >
> > I will do performance compare later. But I don't think there will be measurable
> > difference.
> >
> > > I didn't use napi_alloc_frag() as it's too costly, if in one NAPI
> > > instance driver
> > > receives 32 pkts, then 32 calls to napi_alloc_frag() and updates to page ref
> > > count per fragment etc are costly.
> >
> > No, the page ref only be updated at the page allocation and all the space are
> > used. In general, the invocation of napi_alloc_frag() will not cause the update
> > of the page ref. So in theory, the count of updating page ref should be reduced
> > by using of napi_alloc_frag() compare to the current otx2 implementation.
> >
> 
> Okay, it seems i misunderstood it.

Hi Sunil

In general, you should not work around issues in the core, you should
improve the core. If your implementation really was more efficient
than the core code, it would of been better if you proposed fixes to
the core, not hide away better code in your own driver.

      Andrew