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

[Roger Pau Monné]

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@xxxxxxxxxx> 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.