linux-kernel - Re: 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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 11 Dec 2019 12:52:08 +0100
From:   SeongJae Park <sj38.park@...il.com>
To:     Roger Pau Monné <roger.pau@...rix.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: Re: [PATCH v5 1/2] xenbus/backend: Add memory pressure handler callback

On Wed, 11 Dec 2019 11:51:12 +0100 "Roger Pau Monné" <roger.pau@...rix.com> 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?

Yes, I will change the name.


Thanks,
SeongJae Park

> 
> Thanks, Roger.
>