Re: [PATCH 17/33] readahead: context based method

Peter Zijlstra
2006-05-24T15:53:52Z

On Wed, 2006-05-24 at 21:33 +0800, Wu Fengguang wrote:
> On Wed, May 24, 2006 at 02:37:48PM +0200, Peter Zijlstra wrote:
> > On Wed, 2006-05-24 at 19:13 +0800, Wu Fengguang wrote:
> >
> > > +#define PAGE_REFCNT_0 0
> > > +#define PAGE_REFCNT_1 (1 << PG_referenced)
> > > +#define PAGE_REFCNT_2 (1 << PG_active)
> > > +#define PAGE_REFCNT_3 ((1 << PG_active) | (1 << PG_referenced))
> > > +#define PAGE_REFCNT_MASK PAGE_REFCNT_3
> > > +
> > > +/*
> > > + * STATUS REFERENCE COUNT
> > > + * __ 0
> > > + * _R PAGE_REFCNT_1
> > > + * A_ PAGE_REFCNT_2
> > > + * AR PAGE_REFCNT_3
> > > + *
> > > + * A/R: Active / Referenced
> > > + */
> > > +static inline unsigned long page_refcnt(struct page *page)
> > > +{
> > > + return page->flags & PAGE_REFCNT_MASK;
> > > +}
> > > +
> > > +/*
> > > + * STATUS REFERENCE COUNT TYPE
> > > + * __ 0 fresh
> > > + * _R PAGE_REFCNT_1 stale
> > > + * A_ PAGE_REFCNT_2 disturbed once
> > > + * AR PAGE_REFCNT_3 disturbed twice
> > > + *
> > > + * A/R: Active / Referenced
> > > + */
> > > +static inline unsigned long cold_page_refcnt(struct page *page)
> > > +{
> > > + if (!page || PageActive(page))
> > > + return 0;
> > > +
> > > + return page_refcnt(page);
> > > +}
> > > +
> >
> > Why all of this if all you're ever going to use is cold_page_refcnt.
>
> Well, the two functions have a long history...
>
> There has been a PG_activate which makes the two functions quite
> different. It was later removed for fear of the behavior changes it
> introduced. However, there's still possibility that someone
> reintroduce similar flags in the future :)
>
> > What about something like this:
> >
> > static inline int cold_page_referenced(struct page *page)
> > {
> > if (!page || PageActive(page))
> > return 0;
> > return !!PageReferenced(page);
> > }
>
> Ah, here's another theory: the algorithm uses reference count
> conceptually, so it may be better to retain the current form.

Reference count of what exactly, if you were to say of the page, I'd
have expected only the first function, page_refcnt().

What I don't exactly understand is why you specialise to the inactive
list. Why do you need that?

The reason I'm asking is that when I merge this with my page replacement
work, I need to find a generalised concept. cold_page_refcnt() would
become to mean something like: number of references for those pages that
are direct reclaim candidates. And honestly, that doesn't make a lot of
sense.

If you could explain the concept behind this, I'd be grateful.

Peter