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

From: Yang Shi
Date: Fri Feb 02 2018 - 14:53:16 EST



On 2/1/18 3:01 PM, Yang Shi wrote:


On 2/1/18 1:36 PM, Thomas Gleixner wrote:
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().

OK, will move the reuse logic into 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....

So I should move the accounting to where the obj is deleted from the list? It should look like:

I got your point here. Yes, obj_pool_used should be not decreased here since it has not been allocated from pool list.

But, I think obj_nr_tofree counter should be cleared since all the objs are *NOT* on the global free list anymore. They will be freed later. And, we can't decrease the obj_nr_tofree counter later without acquiring pool lock.


if (obj_nr_tofree > 0)
hlist_move_list(&obj_to_free, &tofree);

...

if (!hlist_empty(&tofree)) {
hlist_for_each_entry_safe(obj, tmp, &tofree, node) {
hlist_del(&obj->node);
obj_nr_tofree--;
kmem_cache_free(obj_cache, obj);
}
}


@@ -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++;

Should it be decreased here since the obj is being dequeued from hlist?

Thanks,
Yang

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?

Sure, will refactor free_object() and split the patches in newer version.

Thanks,
Yang


Thanks,

tglx