linux-kernel - Re: Re: [PATCH v5 1/2] xenbus/backend: Add memory pressure handler callback

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: <20191211105112.GK980@Air-de-Roger>
Date:   Wed, 11 Dec 2019 11:51:12 +0100
From:   Roger Pau Monné <roger.pau@...rix.com>
To:     SeongJae Park <sj38.park@...il.com>
CC:     <sjpark@...zon.com>, <axboe@...nel.dk>, <konrad.wilk@...cle.com>,
        <linux-block@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <pdurrant@...zon.com>, <xen-devel@...ts.xenproject.org>,
        SeongJae Park <sjpark@...zon.de>
Subject: Re: Re: [PATCH v5 1/2] xenbus/backend: Add memory pressure handler
 callback

On Wed, Dec 11, 2019 at 04:50:58AM +0100, SeongJae Park wrote:
> On Tue, 10 Dec 2019 11:16:35 +0100 "Roger Pau Monné" <roger.pau@...rix.com> wrote:
> > > diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h
> > > index 869c816d5f8c..cdb075e4182f 100644
> > > --- a/include/xen/xenbus.h
> > > +++ b/include/xen/xenbus.h
> > > @@ -104,6 +104,7 @@ struct xenbus_driver {
> > >  	struct device_driver driver;
> > >  	int (*read_otherend_details)(struct xenbus_device *dev);
> > >  	int (*is_ready)(struct xenbus_device *dev);
> > > +	unsigned (*reclaim)(struct xenbus_device *dev);
> > 
> > ... hence I wonder why it's returning an unsigned when it's just
> > ignored.
> > 
> > IMO it should return an int to signal errors, and the return should be
> > ignored.
> 
> I first thought similarly and set the callback to return something.  However,
> as this callback is called to simply notify the memory pressure and ask the
> driver to free its memory as many as possible, I couldn't easily imagine what
> kind of errors that need to be handled by its caller can occur in the callback,
> especially because current blkback's callback implementation has no such error.
> So, if you and others agree, I would like to simply set the return type to
> 'void' for now and defer the error handling to a future change.

Yes, I also wondered the same, but seeing you returned an integer I
assumed there was interest in returning some kind of value. If there's
nothing to return let's just make it void.

> > 
> > Also, I think it would preferable for this function to take an extra
> > parameter to describe the resource the driver should attempt to free
> > (ie: memory or interrupts for example). I'm however not able to find
> > any existing Linux type to describe such resources.
> 
> Yes, such extention would be the right direction.  However, because there is no
> existing Linux type to describe the type of resources to reclaim as you also
> mentioned, there could be many different opinions about its implementation
> detail.  In my opinion, it could be also possible to simply add another
> callback for another resource type.  That said, because currently we have an
> use case and an implementation for the memory pressure only, I would like to
> let it as is for now and defer the extension as a future work, if you and
> others have no objection.

Ack, can I please ask the callback to be named reclaim_memory or some
such then?

Thanks, Roger.