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
