Re: [PATCH v15 4/7] mm: Introduce Reported pages

[Alexander Duyck]

On Tue, 2019-12-17 at 03:55 -0500, Nitesh Narayan Lal wrote:
> On 12/16/19 11:28 AM, Alexander Duyck wrote:
> > On Mon, 2019-12-16 at 05:17 -0500, Nitesh Narayan Lal wrote:
> > > On 12/5/19 11:22 AM, Alexander Duyck wrote:
> > > > From: Alexander Duyck <alexander.h.duyck@xxxxxxxxxxxxxxx>
> > > > 
> > > > In order to pave the way for free page reporting in virtualized
> > > > environments we will need a way to get pages out of the free lists and
> > > > identify those pages after they have been returned. To accomplish this,
> > > > this patch adds the concept of a Reported Buddy, which is essentially
> > > > meant to just be the Uptodate flag used in conjunction with the Buddy
> > > > page type.
> > > [...]
> > > 
> > > > +enum {
> > > > +	PAGE_REPORTING_IDLE = 0,
> > > > +	PAGE_REPORTING_REQUESTED,
> > > > +	PAGE_REPORTING_ACTIVE
> > > > +};
> > > > +
> > > > +/* request page reporting */
> > > > +static void
> > > > +__page_reporting_request(struct page_reporting_dev_info *prdev)
> > > > +{
> > > > +	unsigned int state;
> > > > +
> > > > +	/* Check to see if we are in desired state */
> > > > +	state = atomic_read(&prdev->state);
> > > > +	if (state == PAGE_REPORTING_REQUESTED)
> > > > +		return;
> > > > +
> > > > +	/*
> > > > +	 *  If reporting is already active there is nothing we need to do.
> > > > +	 *  Test against 0 as that represents PAGE_REPORTING_IDLE.
> > > > +	 */
> > > > +	state = atomic_xchg(&prdev->state, PAGE_REPORTING_REQUESTED);
> > > > +	if (state != PAGE_REPORTING_IDLE)
> > > > +		return;
> > > > +
> > > > +	/*
> > > > +	 * Delay the start of work to allow a sizable queue to build. For
> > > > +	 * now we are limiting this to running no more than once every
> > > > +	 * couple of seconds.
> > > > +	 */
> > > > +	schedule_delayed_work(&prdev->work, PAGE_REPORTING_DELAY);
> > > > +}
> > > > +
> > > I think you recently switched to using an atomic variable for maintaining page
> > > reporting status as I was doing in v12.
> > > Which is good, as we will not have a disagreement on it now.
> > There is still some differences between our approaches if I am not
> > mistaken. Specifically I have code in place so that any requests to report
> > while we are actively working on reporting will trigger another pass being
> > scheduled after we completed. I still believe you were lacking any logic
> > like that as I recall.
> > 
> 
> Yes, I was specifically referring to the atomic state variable.
> Though I am wondering if having an atomic variable to track page reporting state
> is better than having a page reporting specific unsigned long flag, which we can
> manipulate via __set_bit() and __clear_bit().

So the reason for using an atomic state variable is because I only really
have 3 possible states; idle, active, and requested. It allows for a
pretty simple state machine as any transition from idle indicates that we
need to schedule the worker, transition from requested to active when the
worker starts, and if at the end of a pass if we are still in the active
state it means we can transition back to idle, otherwise we reschedule the
worker.

In order to do the same sort of thing using the bitops would require at
least 2 bits. In addition with the requirement that I cannot use the zone
lock for protection of the state I cannot use the non-atomic versions of
things such as __set_bit and __clear_bit so they would require additional
locking protections.