Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 30 Sep 2022 08:57:02 -0600
From:   Keith Busch <kbusch@kernel.org>
To:     Serge Semin <fancer.lancer@gmail.com>
Cc:     Christoph Hellwig <hch@lst.de>,
        Serge Semin <Sergey.Semin@baikalelectronics.ru>,
        Jens Axboe <axboe@kernel.dk>, Jens Axboe <axboe@fb.com>,
        Sagi Grimberg <sagi@grimberg.me>,
        Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>,
        Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>,
        Thomas Bogendoerfer <tsbogend@alpha.franken.de>,
        linux-nvme@lists.infradead.org, linux-block@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/3] nvme-hwmon: Return error on kzalloc failure
Message-ID: <YzcDvmlslPki8gBj@kbusch-mbp.dhcp.thefacebook.com>
References: <20220929224648.8997-1-Sergey.Semin@baikalelectronics.ru>
 <20220929224648.8997-2-Sergey.Semin@baikalelectronics.ru>
 <YzYwB7lRGW80r4HA@kbusch-mbp.dhcp.thefacebook.com>
 <20220930095247.vqtdc53rr66uaiwv@mobilestation>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220930095247.vqtdc53rr66uaiwv@mobilestation>
Precedence: bulk

On Fri, Sep 30, 2022 at 12:52:47PM +0300, Serge Semin wrote:
> On Thu, Sep 29, 2022 at 05:53:43PM -0600, Keith Busch wrote:
> > On Fri, Sep 30, 2022 at 01:46:46AM +0300, Serge Semin wrote:
> > > Inability to allocate a buffer is a critical error which shouldn't be
> > > tolerated since most likely the rest of the driver won't work correctly.
> > > Thus instead of returning the zero status let's return the -ENOMEM error
> > > if the nvme_hwmon_data structure instance couldn't be created.
> > 
> 
> > Nak for this one. The hwmon is not necessary for the rest of the driver to
> > function, so having the driver detach from the device seems a bit harsh.
> 
> Even if it is as you say, neither the method semantic nor the way it's
> called imply that. Any failures except the allocation one are
> perceived as erroneous.

This is called by nvme_init_ctrl_finish(), and returns the error to
nvme_reset_work() only if it's < 0, which indicates we can't go on and the
driver unbinds.

This particular condition for hwmon is not something that prevents us from
making forward progress.
 
> > The
> > driver can participate in memory reclaim, so failing on a low memory condition
> > can make matters worse.
> 
> Yes it can, so can many other places in the driver utilizing kmalloc
> with just GFP_KERNEL flag passed including on the same path as the
> nvme_hwmon_init() execution. Kmalloc will make sure the reclaim is
> either finished or executed in background anyway in all cases. 

This path is in the first initialization before we've set up a namespace that
can be used as a reclaim destination.

> Don't
> really see why memory allocation failure is less worse in this case
> than in many others in the same driver especially seeing as I said

The other initialization kmalloc's are required to make forward progress toward
setting up a namespace. This one is not required.