Re: [PATCH v2 2/3] mm: kmemleak: Factor object reference updating out of scan_block()

[Catalin Marinas]

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@xxxxxxxxxx>
> > Cc: Andy Lutomirski <luto@xxxxxxxxxxxxxx>
> > Cc: "Luis R. Rodriguez" <mcgrof@xxxxxxxxxx>
> > Signed-off-by: Catalin Marinas <catalin.marinas@xxxxxxx>
> > ---
> >  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