Re: [PATCH] zsmalloc: zs_page_migrate: not check inuse if migrate_mode is not MIGRATE_ASYNC

[Minchan Kim]

Hi Hui,

On Thu, Jul 20, 2017 at 05:33:45PM +0800, Hui Zhu wrote:

< snip >

> >> >> +++ b/mm/zsmalloc.c
> >> >> @@ -1982,6 +1982,7 @@ int zs_page_migrate(struct address_space *mapping, struct page *newpage,
> >> >>       unsigned long old_obj, new_obj;
> >> >>       unsigned int obj_idx;
> >> >>       int ret = -EAGAIN;
> >> >> +     int inuse;
> >> >>
> >> >>       VM_BUG_ON_PAGE(!PageMovable(page), page);
> >> >>       VM_BUG_ON_PAGE(!PageIsolated(page), page);
> >> >> @@ -1996,21 +1997,24 @@ int zs_page_migrate(struct address_space *mapping, struct page *newpage,
> >> >>       offset = get_first_obj_offset(page);
> >> >>
> >> >>       spin_lock(&class->lock);
> >> >> -     if (!get_zspage_inuse(zspage)) {
> >> >> +     inuse = get_zspage_inuse(zspage);
> >> >> +     if (mode == MIGRATE_ASYNC && !inuse) {
> >> >>               ret = -EBUSY;
> >> >>               goto unlock_class;
> >> >>       }
> >> >>
> >> >>       pos = offset;
> >> >>       s_addr = kmap_atomic(page);
> >> >> -     while (pos < PAGE_SIZE) {
> >> >> -             head = obj_to_head(page, s_addr + pos);
> >> >> -             if (head & OBJ_ALLOCATED_TAG) {
> >> >> -                     handle = head & ~OBJ_ALLOCATED_TAG;
> >> >> -                     if (!trypin_tag(handle))
> >> >> -                             goto unpin_objects;
> >> >> +     if (inuse) {
> >
> > I don't want to add inuse check for every loop. It might avoid unncessary
> > looping in every loop of zs_page_migrate so it is for optimization, not
> > correction. As I consider it would happen rarely, I think we don't need
> > to add the check. Could you just remove get_zspage_inuse check, instead?
> >
> > like this.
> >
> >
> > diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
> > index 013eea76685e..2d3d75fb0f16 100644
> > --- a/mm/zsmalloc.c
> > +++ b/mm/zsmalloc.c
> > @@ -1980,14 +1980,9 @@ int zs_page_migrate(struct address_space *mapping, struct page *newpage,
> >         pool = mapping->private_data;
> >         class = pool->size_class[class_idx];
> >         offset = get_first_obj_offset(page);
> > +       pos = offset;
> >
> >         spin_lock(&class->lock);
> > -       if (!get_zspage_inuse(zspage)) {
> > -               ret = -EBUSY;
> > -               goto unlock_class;
> > -       }
> > -
> > -       pos = offset;
> >         s_addr = kmap_atomic(page);
> >         while (pos < PAGE_SIZE) {
> >                 head = obj_to_head(page, s_addr + pos);
> >
> >
> 
> What about set pos to avoid the loops?
> 
> @@ -1997,8 +1997,10 @@ int zs_page_migrate(struct address_space
> *mapping, struct page *newpage,
> 
>         spin_lock(&class->lock);
>         if (!get_zspage_inuse(zspage)) {
> -               ret = -EBUSY;
> -               goto unlock_class;
> +               /* The page is empty.
> +                  Set "offset" to the end of page.
> +                  Then the loops of page will be avoided.  */
> +               offset = PAGE_SIZE;

Good idea. Just a nitpick:

/*
 * set "offset" to end of the page so that every loops
 * skips unnecessary object scanning.
 */

Thanks!