netdev - Re: [PATCH net] ionic: Fix allocation of q/cq info structures from device local node

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: <20230411124704.GX182481@unreal>
Date:   Tue, 11 Apr 2023 15:47:04 +0300
From:   Leon Romanovsky <leon@...nel.org>
To:     Brett Creeley <bcreeley@....com>
Cc:     Brett Creeley <brett.creeley@....com>, davem@...emloft.net,
        netdev@...r.kernel.org, kuba@...nel.org, drivers@...sando.io,
        shannon.nelson@....com, neel.patel@....com
Subject: Re: [PATCH net] ionic: Fix allocation of q/cq info structures from
 device local node

On Mon, Apr 10, 2023 at 11:16:03AM -0700, Brett Creeley wrote:
> On 4/9/2023 3:52 AM, Leon Romanovsky wrote:
> > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> > 
> > 
> > On Fri, Apr 07, 2023 at 04:36:45PM -0700, Brett Creeley wrote:
> > > Commit 116dce0ff047 ("ionic: Use vzalloc for large per-queue related
> > > buffers") made a change to relieve memory pressure by making use of
> > > vzalloc() due to the structures not requiring DMA mapping. However,
> > > it overlooked that these structures are used in the fast path of the
> > > driver and allocations on the non-local node could cause performance
> > > degredation. Fix this by first attempting to use vzalloc_node()
> > > using the device's local node and if that fails try again with
> > > vzalloc().
> > > 
> > > Fixes: 116dce0ff047 ("ionic: Use vzalloc for large per-queue related buffers")
> > > Signed-off-by: Neel Patel <neel.patel@....com>
> > > Signed-off-by: Brett Creeley <brett.creeley@....com>
> > > Signed-off-by: Shannon Nelson <shannon.nelson@....com>
> > > ---
> > >   .../net/ethernet/pensando/ionic/ionic_lif.c   | 24 ++++++++++++-------
> > >   1 file changed, 16 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> > > index 957027e546b3..2c4e226b8cf1 100644
> > > --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> > > +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> > > @@ -560,11 +560,15 @@ static int ionic_qcq_alloc(struct ionic_lif *lif, unsigned int type,
> > >        new->q.dev = dev;
> > >        new->flags = flags;
> > > 
> > > -     new->q.info = vzalloc(num_descs * sizeof(*new->q.info));
> > > +     new->q.info = vzalloc_node(num_descs * sizeof(*new->q.info),
> > > +                                dev_to_node(dev));
> > >        if (!new->q.info) {
> > > -             netdev_err(lif->netdev, "Cannot allocate queue info\n");
> > > -             err = -ENOMEM;
> > > -             goto err_out_free_qcq;
> > > +             new->q.info = vzalloc(num_descs * sizeof(*new->q.info));
> > > +             if (!new->q.info) {
> > > +                     netdev_err(lif->netdev, "Cannot allocate queue info\n");
> > 
> > Kernel memory allocator will try local node first and if memory is
> > depleted it will go to remote nodes. So basically, you open-coded that
> > behaviour but with OOM splash when first call to vzalloc_node fails and
> > with custom error message about memory allocation failure.
> > 
> > Thanks
> 
> Leon,
> 
> We want to allocate memory from the node local to our PCI device, which is
> not necessarily the same as the node that the thread is running on where
> vzalloc() first tries to alloc.

I'm not sure about it as you are running kernel thread which is
triggered directly by device and most likely will run on same node as
PCI device.

> Since it wasn't clear to us that vzalloc_node() does any fallback, 

vzalloc_node() doesn't do fallback, but vzalloc will find the right node
for you.

> we followed the example in the ena driver to follow up with a more
> generic vzalloc() request.

I don't know about ENA implementation, maybe they have right reasons to
do it, but maybe they don't.

> 
> Also, the custom message helps us quickly figure out exactly which
> allocation failed.

If OOM is missing some info to help debug allocation failures, let's add
it there, but please do not add any custom prints after alloc failures.

Thanks

> 
> Thanks,
> 
> Brett