Re: [PATCH 2/2 v5] lib: debugobjects: handle objects free in a batch outside the loop

From: Thomas Gleixner
Date: Thu Feb 01 2018 - 16:37:48 EST


On Fri, 2 Feb 2018, Yang Shi wrote:

> /*
> - * Allocate a new object. If the pool is empty, switch off the debugger.
> + * Allocate a new object. Retrieve from global freelist first. If the pool is
> + * empty, switch off the debugger.
> * Must be called with interrupts disabled.
> */
> static struct debug_obj *
> @@ -150,6 +154,13 @@ static struct debug_obj *lookup_object(void *addr, struct debug_bucket *b)
> struct debug_obj *obj = NULL;
>
> raw_spin_lock(&pool_lock);

Why in alloc_object() and not in fill_pool()?

> + if (obj_nr_tofree > 0 && (obj_pool_free < obj_pool_min_free)) {
> + obj = hlist_entry(obj_to_free.first, typeof(*obj), node);
> + obj_nr_tofree--;
> + hlist_del(&obj->node);
> + goto out;
> + }

Errm. This is wrong. It does not reinitialize the object. Please do that
shuffling in fill_pool().

> if (obj_pool.first) {
> obj = hlist_entry(obj_pool.first, typeof(*obj), node);

....

> + /* When pool list is not full move free objs to pool list */
> + while (obj_pool_free < debug_objects_pool_size) {
> + if (obj_nr_tofree <= 0)
> + break;
> +
> + obj = hlist_entry(obj_to_free.first, typeof(*obj), node);
> + hlist_del(&obj->node);
> + hlist_add_head(&obj->node, &obj_pool);
> + obj_pool_free++;
> + obj_pool_used--;
> + obj_nr_tofree--;
> + }
> +
> + /*
> + * pool list is already full, and there are still objs on the free list,
> + * move remaining free objs to a separate list to free the memory later.
> + */
> + if (obj_nr_tofree > 0) {
> + hlist_move_list(&obj_to_free, &tofree);
> + obj_nr_tofree = 0;
> + }

The accounting is inconsistent. You leak obj_pool_used. But that's wrong
anyway because an object should not be accounted for in two places. It's
only on _ONE_ list....

> @@ -716,7 +762,6 @@ static void __debug_check_no_obj_freed(const void *address, unsigned long size)
> {
> unsigned long flags, oaddr, saddr, eaddr, paddr, chunks;
> struct hlist_node *tmp;
> - HLIST_HEAD(freelist);
> struct debug_obj_descr *descr;
> enum debug_obj_state state;
> struct debug_bucket *db;
> @@ -752,18 +797,17 @@ static void __debug_check_no_obj_freed(const void *address, unsigned long size)
> goto repeat;
> default:
> hlist_del(&obj->node);
> - hlist_add_head(&obj->node, &freelist);
> + /* Put obj on the global free list */
> + raw_spin_lock(&pool_lock);
> + hlist_add_head(&obj->node, &obj_to_free);
> + /* Update the counter of objs on the global freelist */
> + obj_nr_tofree++;
> + raw_spin_unlock(&pool_lock);

As we have to take pool_lock anyway, we simply can change free_object() to:

static bool __free_object(obj)
{
bool work;

lock(pool);
work = obj_pool_free > debug_objects_pool_size && obj_cache;
obj_pool_used++;
if (work) {
obj_nr_tofree++;
hlist_add_head(&obj->node, &obj_to_free);
] else {
obj_pool_free++;
hlist_add_head(&obj->node, &obj_pool);
}
unlock(pool);
return work;
}

static void free_object(obj)
{
if (__free_object(obj))
schedule_work(&debug_obj_work);
}

and then use __free_object() in __debug_check_no_obj_freed()

bool work = false;

...
work |= __free_object(obj);
...

if (work)
schedule_work(&debug_obj_work);

That makes the whole thing simpler and the accounting is matching the place
where the object is:

obj_pool_free counts the number of objects enqueued in obj_pool
obj_nr_tofree counts the number of objects enqueued in obj_to_free
obr_pool_used counts the number of objects enqueued in the hash lists

Ideally you split that patch into pieces:

1) Introduce obj_to_free/obj_nr_tofree and add the removing/freeing from it
in fill_pool() and free_obj_work(). Nothing adds an object at this point
to obj_to_free.

2) Change free_object() to use obj_to_free and split it apart

3) Change __debug_check_no_obj_freed() to use __free_object()

That makes it simpler to review and to follow.

Hmm?

Thanks,

tglx