linux-kernel - Re: [PATCH] zsmalloc: zs_page_migrate: not check inuse if migrate_mode is not MIGRATE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20170721050712.GA11758@bbox>
Date:   Fri, 21 Jul 2017 14:07:12 +0900
From:   Minchan Kim <minchan@...nel.org>
To:     Hui Zhu <teawater@...il.com>
Cc:     Hui Zhu <zhuhui@...omi.com>,
        "ngupta@...are.org" <ngupta@...are.org>,
        Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
        Linux Memory Management List <linux-mm@...ck.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] zsmalloc: zs_page_migrate: not check inuse if
 migrate_mode is not MIGRATE_ASYNC

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!