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;
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) {
                        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;
-                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;

    work = obj_pool_free > debug_objects_pool_size && obj_cache;

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


    if (work) {
        hlist_add_head(&obj->node, &obj_to_free);
    ] else {
        hlist_add_head(&obj->node, &obj_pool);
    return work;

static void free_object(obj)
    if (__free_object(obj))

and then use __free_object() in __debug_check_no_obj_freed()

          bool work = false;

               work |= __free_object(obj);

     if (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.


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




Reply via email to