Re: [PATCH V2 2/2] change shrinker API by passing shrink_control struct

[Ying Han <yinghan@google.com>]

[-- Attachment #1: Type: text/plain, Size: 2299 bytes --]

On Mon, Apr 25, 2011 at 6:15 PM, KOSAKI Motohiro <
kosaki.motohiro@jp.fujitsu.com> wrote:

> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index 7a2f657..abc13ea 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -1137,16 +1137,19 @@ static inline void sync_mm_rss(struct task_struct
> *task, struct mm_struct *mm)
> >  struct shrink_control {
> >       unsigned long nr_scanned;
> >       gfp_t gfp_mask;
> > +
> > +     /* How many slab objects shrinker() should reclaim */
> > +     unsigned long nr_slab_to_reclaim;
>
> Wrong name. The original shrinker API is
>        int (*shrink)(struct shrinker *, int nr_to_scan, gfp_t gfp_mask);
>
> ie, shrinker get scanning target. not reclaiming target.
> You should have think folloing diff hunk is strange.
>

Ok, changed to "nr_slab_to_shrink"

>
> >  {
> >       struct xfs_mount *mp;
> >       struct xfs_perag *pag;
> >       xfs_agnumber_t  ag;
> >       int             reclaimable;
> > +     int nr_to_scan = sc->nr_slab_to_reclaim;
> > +     gfp_t gfp_mask = sc->gfp_mask;
>
> And, this very near meaning field .nr_scanned and .nr_slab_to_reclaim
> poped up new question.
> Why don't we pass more clever slab shrinker target? Why do we need pass
> similar two argument?
>

I renamed the nr_slab_to_reclaim and nr_scanned in shrink struct.

--Ying

> >  /*
> >   * A callback you can register to apply pressure to ageable caches.
> >   *
> > - * 'shrink' is passed a count 'nr_to_scan' and a 'gfpmask'.  It should
> > - * look through the least-recently-used 'nr_to_scan' entries and
> > - * attempt to free them up.  It should return the number of objects
> > - * which remain in the cache.  If it returns -1, it means it cannot do
> > - * any scanning at this time (eg. there is a risk of deadlock).
> > + * 'sc' is passed shrink_control which includes a count
> 'nr_slab_to_reclaim'
> > + * and a 'gfpmask'.  It should look through the least-recently-us
>
>                                                                  us?
>
>
> > + * 'nr_slab_to_reclaim' entries and attempt to free them up.  It should
> return
> > + * the number of objects which remain in the cache.  If it returns -1,
> it means
> > + * it cannot do any scanning at this time (eg. there is a risk of
> deadlock).
> >   *
>
>
>
>
>

[-- Attachment #2: Type: text/html, Size: 3186 bytes --]