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

[SeongJae Park <sj38.park@gmail.com>]

On Wed, 11 Dec 2019 11:51:12 +0100 "Roger Pau Monné" <roger.pau@citrix.com> wrote:

> > On Tue, 10 Dec 2019 11:16:35 +0100 "Roger Pau Monné" <roger.pau@citrix.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.
>