Messages in this thread | | | Date | Tue, 3 Dec 2013 11:08:32 +0900 | From | Joonsoo Kim <> | Subject | Re: [PATCH 6/9] mm/rmap: use rmap_walk() in try_to_unmap() |
| |
On Mon, Dec 02, 2013 at 03:01:07PM -0800, Andrew Morton wrote: > On Thu, 28 Nov 2013 16:48:43 +0900 Joonsoo Kim <iamjoonsoo.kim@lge.com> wrote: > > > Now, we have an infrastructure in rmap_walk() to handle difference > > from variants of rmap traversing functions. > > > > So, just use it in try_to_unmap(). > > > > In this patch, I change following things. > > > > 1. enable rmap_walk() if !CONFIG_MIGRATION. > > 2. mechanical change to use rmap_walk() in try_to_unmap(). > > > > ... > > > > --- a/include/linux/rmap.h > > +++ b/include/linux/rmap.h > > @@ -190,7 +190,7 @@ int page_referenced_one(struct page *, struct vm_area_struct *, > > > > int try_to_unmap(struct page *, enum ttu_flags flags); > > int try_to_unmap_one(struct page *, struct vm_area_struct *, > > - unsigned long address, enum ttu_flags flags); > > + unsigned long address, void *arg); > > This change is ugly and unchangelogged. > > Also, "enum ttu_flags flags" was nice and meaningful, but "void *arg" > conveys far less information. A suitable way to address this > shortcoming is to document `arg' at the try_to_unmap_one() definition > site. try_to_unmap_one() doesn't actually have any documentation at > this stage - let's please fix that?
Okay. I will add some comments.
> > > > ... > > > > @@ -1509,6 +1510,11 @@ bool is_vma_temporary_stack(struct vm_area_struct *vma) > > return false; > > } > > > > +static int skip_vma_temporary_stack(struct vm_area_struct *vma, void *arg) > > +{ > > + return (int)is_vma_temporary_stack(vma); > > +} > > The (int) cast is unneeded - the compiler will turn a bool into an int. > > Should this function (and rmap_walk_control.skip()) really be returning > a bool?
Okay. Will do.
> > The name of this function is poor: "skip_foo" implies that the function > will skip over a foo. But that isn't what this function does. Please > choose something which accurately reflects the function's behavior.
Okay.
Thanks.
| |