| Message ID | 1495726937-23557-3-git-send-email-catalin.marinas@arm.com |
|---|---|
| State | Accepted |
| Commit | 04f70d13ca274d62a347815ca01fea871f9b9a40 |
| Headers | show |
| Series | mm: kmemleak: Improve vmalloc() false positives for thread stack allocation | expand |
On Thu, May 25, 2017 at 04:42:16PM +0100, Catalin Marinas wrote: > The scan_block() function updates the number of references (pointers) to > objects, adding them to the gray_list when object->min_count is reached. > The patch factors out this functionality into a separate update_refs() > function. > > Cc: Michal Hocko <mhocko@kernel.org> > Cc: Andy Lutomirski <luto@amacapital.net> > Cc: "Luis R. Rodriguez" <mcgrof@kernel.org> > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> > --- > mm/kmemleak.c | 43 +++++++++++++++++++++++++------------------ > 1 file changed, 25 insertions(+), 18 deletions(-) > > diff --git a/mm/kmemleak.c b/mm/kmemleak.c > index 964b12eba2c1..266482f460c2 100644 > --- a/mm/kmemleak.c > +++ b/mm/kmemleak.c > @@ -1188,6 +1188,30 @@ static bool update_checksum(struct kmemleak_object *object) > } > > /* > + * Update an object's references. object->lock must be held by the caller. > + */ > +static void update_refs(struct kmemleak_object *object) > +{ > + if (!color_white(object)) { > + /* non-orphan, ignored or new */ > + return; > + } > + > + /* > + * Increase the object's reference count (number of pointers to the > + * memory block). If this count reaches the required minimum, the > + * object's color will become gray and it will be added to the > + * gray_list. > + */ > + object->count++; > + if (color_gray(object)) { > + /* put_object() called when removing from gray_list */ > + WARN_ON(!get_object(object)); > + list_add_tail(&object->gray_list, &gray_list); > + } > +} > + > +/* > * Memory scanning is a long process and it needs to be interruptable. This > * function checks whether such interrupt condition occurred. > */ > @@ -1259,24 +1283,7 @@ static void scan_block(void *_start, void *_end, > * enclosed by scan_mutex. > */ > spin_lock_nested(&object->lock, SINGLE_DEPTH_NESTING); > - if (!color_white(object)) { > - /* non-orphan, ignored or new */ > - spin_unlock(&object->lock); > - continue; > - } > - > - /* > - * Increase the object's reference count (number of pointers > - * to the memory block). If this count reaches the required > - * minimum, the object's color will become gray and it will be > - * added to the gray_list. > - */ > - object->count++; > - if (color_gray(object)) { > - /* put_object() called when removing from gray_list */ > - WARN_ON(!get_object(object)); > - list_add_tail(&object->gray_list, &gray_list); > - } > + update_refs(object); > spin_unlock(&object->lock); FWIW, I've tested this patchset and I don't see kmemleak triggering the false positives anymore. I've also done a quick review and couldn't find anything obviously incorrect, just a question: why didn't you moved the spin_lock/unlock into update_refs() too? It would save you 2 lines in the next patch :) Cheers, -- Luís > } > read_unlock_irqrestore(&kmemleak_lock, flags); >
On Fri, May 26, 2017 at 05:09:17PM +0100, Luis Henriques wrote: > On Thu, May 25, 2017 at 04:42:16PM +0100, Catalin Marinas wrote: > > The scan_block() function updates the number of references (pointers) to > > objects, adding them to the gray_list when object->min_count is reached. > > The patch factors out this functionality into a separate update_refs() > > function. > > > > Cc: Michal Hocko <mhocko@kernel.org> > > Cc: Andy Lutomirski <luto@amacapital.net> > > Cc: "Luis R. Rodriguez" <mcgrof@kernel.org> > > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> > > --- > > mm/kmemleak.c | 43 +++++++++++++++++++++++++------------------ > > 1 file changed, 25 insertions(+), 18 deletions(-) > > > > diff --git a/mm/kmemleak.c b/mm/kmemleak.c > > index 964b12eba2c1..266482f460c2 100644 > > --- a/mm/kmemleak.c > > +++ b/mm/kmemleak.c > > @@ -1188,6 +1188,30 @@ static bool update_checksum(struct kmemleak_object *object) > > } > > > > /* > > + * Update an object's references. object->lock must be held by the caller. > > + */ > > +static void update_refs(struct kmemleak_object *object) > > +{ > > + if (!color_white(object)) { > > + /* non-orphan, ignored or new */ > > + return; > > + } > > + > > + /* > > + * Increase the object's reference count (number of pointers to the > > + * memory block). If this count reaches the required minimum, the > > + * object's color will become gray and it will be added to the > > + * gray_list. > > + */ > > + object->count++; > > + if (color_gray(object)) { > > + /* put_object() called when removing from gray_list */ > > + WARN_ON(!get_object(object)); > > + list_add_tail(&object->gray_list, &gray_list); > > + } > > +} > > + > > +/* > > * Memory scanning is a long process and it needs to be interruptable. This > > * function checks whether such interrupt condition occurred. > > */ > > @@ -1259,24 +1283,7 @@ static void scan_block(void *_start, void *_end, > > * enclosed by scan_mutex. > > */ > > spin_lock_nested(&object->lock, SINGLE_DEPTH_NESTING); > > - if (!color_white(object)) { > > - /* non-orphan, ignored or new */ > > - spin_unlock(&object->lock); > > - continue; > > - } > > - > > - /* > > - * Increase the object's reference count (number of pointers > > - * to the memory block). If this count reaches the required > > - * minimum, the object's color will become gray and it will be > > - * added to the gray_list. > > - */ > > - object->count++; > > - if (color_gray(object)) { > > - /* put_object() called when removing from gray_list */ > > - WARN_ON(!get_object(object)); > > - list_add_tail(&object->gray_list, &gray_list); > > - } > > + update_refs(object); > > spin_unlock(&object->lock); > > FWIW, I've tested this patchset and I don't see kmemleak triggering the > false positives anymore. Thanks for re-testing (I dropped your tested-by from the initial patch since I made a small modification). > I've also done a quick review and couldn't find anything obviously > incorrect, just a question: why didn't you moved the spin_lock/unlock into > update_refs() too? It would save you 2 lines in the next patch :) There is a small difference: for the first object it needs to check color_gray() and access object->excess_ref while the lock is held. It doesn't need this in the second case. I could've written it in different ways but probably with a similar number of lines; I just found this clearer. -- Catalin
On Fri, May 26, 2017 at 05:21:08PM +0100, Catalin Marinas wrote: > On Fri, May 26, 2017 at 05:09:17PM +0100, Luis Henriques wrote: > > On Thu, May 25, 2017 at 04:42:16PM +0100, Catalin Marinas wrote: > > > The scan_block() function updates the number of references (pointers) to > > > objects, adding them to the gray_list when object->min_count is reached. > > > The patch factors out this functionality into a separate update_refs() > > > function. > > > > > > Cc: Michal Hocko <mhocko@kernel.org> > > > Cc: Andy Lutomirski <luto@amacapital.net> > > > Cc: "Luis R. Rodriguez" <mcgrof@kernel.org> > > > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> [...] > > FWIW, I've tested this patchset and I don't see kmemleak triggering the > > false positives anymore. > > Thanks for re-testing (I dropped your tested-by from the initial patch > since I made a small modification). Sorry, the "re-testing" comment was meant at the other Luis on cc ;) (Luis R. Rodriguez). It's been a long day... -- Catalin
On Fri, May 26, 2017 at 05:23:30PM +0100, Catalin Marinas wrote: > On Fri, May 26, 2017 at 05:21:08PM +0100, Catalin Marinas wrote: > > On Fri, May 26, 2017 at 05:09:17PM +0100, Luis Henriques wrote: > > > On Thu, May 25, 2017 at 04:42:16PM +0100, Catalin Marinas wrote: > > > > The scan_block() function updates the number of references (pointers) to > > > > objects, adding them to the gray_list when object->min_count is reached. > > > > The patch factors out this functionality into a separate update_refs() > > > > function. > > > > > > > > Cc: Michal Hocko <mhocko@kernel.org> > > > > Cc: Andy Lutomirski <luto@amacapital.net> > > > > Cc: "Luis R. Rodriguez" <mcgrof@kernel.org> > > > > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> > [...] > > > FWIW, I've tested this patchset and I don't see kmemleak triggering the > > > false positives anymore. > > > > Thanks for re-testing (I dropped your tested-by from the initial patch > > since I made a small modification). > > Sorry, the "re-testing" comment was meant at the other Luis on cc ;) > (Luis R. Rodriguez). It's been a long day... Heh, no worries! What are the odds of having 2 different guys named Luis testing the same patch? :-) Cheers, -- Luís
diff --git a/mm/kmemleak.c b/mm/kmemleak.c index 964b12eba2c1..266482f460c2 100644 --- a/mm/kmemleak.c +++ b/mm/kmemleak.c @@ -1188,6 +1188,30 @@ static bool update_checksum(struct kmemleak_object *object) } /* + * Update an object's references. object->lock must be held by the caller. + */ +static void update_refs(struct kmemleak_object *object) +{ + if (!color_white(object)) { + /* non-orphan, ignored or new */ + return; + } + + /* + * Increase the object's reference count (number of pointers to the + * memory block). If this count reaches the required minimum, the + * object's color will become gray and it will be added to the + * gray_list. + */ + object->count++; + if (color_gray(object)) { + /* put_object() called when removing from gray_list */ + WARN_ON(!get_object(object)); + list_add_tail(&object->gray_list, &gray_list); + } +} + +/* * Memory scanning is a long process and it needs to be interruptable. This * function checks whether such interrupt condition occurred. */ @@ -1259,24 +1283,7 @@ static void scan_block(void *_start, void *_end, * enclosed by scan_mutex. */ spin_lock_nested(&object->lock, SINGLE_DEPTH_NESTING); - if (!color_white(object)) { - /* non-orphan, ignored or new */ - spin_unlock(&object->lock); - continue; - } - - /* - * Increase the object's reference count (number of pointers - * to the memory block). If this count reaches the required - * minimum, the object's color will become gray and it will be - * added to the gray_list. - */ - object->count++; - if (color_gray(object)) { - /* put_object() called when removing from gray_list */ - WARN_ON(!get_object(object)); - list_add_tail(&object->gray_list, &gray_list); - } + update_refs(object); spin_unlock(&object->lock); } read_unlock_irqrestore(&kmemleak_lock, flags);
The scan_block() function updates the number of references (pointers) to objects, adding them to the gray_list when object->min_count is reached. The patch factors out this functionality into a separate update_refs() function. Cc: Michal Hocko <mhocko@kernel.org> Cc: Andy Lutomirski <luto@amacapital.net> Cc: "Luis R. Rodriguez" <mcgrof@kernel.org> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> --- mm/kmemleak.c | 43 +++++++++++++++++++++++++------------------ 1 file changed, 25 insertions(+), 18 deletions(-)