Re: [PATCH 2/2 v5] lib: debugobjects: handle objects free in a batch outside the loop
On Fri, 2 Feb 2018, Yang Shi wrote:
> On 2/1/18 3:01 PM, Yang Shi wrote:
> > On 2/1/18 1:36 PM, Thomas Gleixner wrote:
> > > 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.
Right, when you split the list to the temporary tofree list head then you
need to reset obj_nr_tofree as well. And that's correct because the objects
are not longer reachable through one of the global lists.
> > > 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?
I guess so :)
Thanks,
tglx
Re: [PATCH 2/2 v5] lib: debugobjects: handle objects free in a batch outside the loop
On Fri, 2 Feb 2018, Yang Shi wrote:
> On 2/1/18 3:01 PM, Yang Shi wrote:
> > On 2/1/18 1:36 PM, Thomas Gleixner wrote:
> > > 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.
Right, when you split the list to the temporary tofree list head then you
need to reset obj_nr_tofree as well. And that's correct because the objects
are not longer reachable through one of the global lists.
> > > 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?
I guess so :)
Thanks,
tglx
Re: [PATCH 2/2 v5] lib: debugobjects: handle objects free in a batch outside the loop
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(_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(>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(>node);
+hlist_add_head(>node, _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(_to_free, );
+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(_to_free, );
...
if (!hlist_empty()) {
hlist_for_each_entry_safe(obj, tmp, , node) {
hlist_del(>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(>node);
-hlist_add_head(>node, );
+/* Put obj on the global free list */
+raw_spin_lock(_lock);
+hlist_add_head(>node, _to_free);
+/* Update the counter of objs on the global
freelist */
+obj_nr_tofree++;
+raw_spin_unlock(_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(>node, _to_free);
] else {
obj_pool_free++;
hlist_add_head(>node, _pool);
}
unlock(pool);
return work;
}
static void free_object(obj)
{
if (__free_object(obj))
schedule_work(_obj_work);
}
and then use __free_object() in __debug_check_no_obj_freed()
bool work = false;
...
work |= __free_object(obj);
...
if (work)
schedule_work(_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
Re: [PATCH 2/2 v5] lib: debugobjects: handle objects free in a batch outside the loop
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(_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(>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(>node);
+hlist_add_head(>node, _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(_to_free, );
+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(_to_free, );
...
if (!hlist_empty()) {
hlist_for_each_entry_safe(obj, tmp, , node) {
hlist_del(>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(>node);
-hlist_add_head(>node, );
+/* Put obj on the global free list */
+raw_spin_lock(_lock);
+hlist_add_head(>node, _to_free);
+/* Update the counter of objs on the global
freelist */
+obj_nr_tofree++;
+raw_spin_unlock(_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(>node, _to_free);
] else {
obj_pool_free++;
hlist_add_head(>node, _pool);
}
unlock(pool);
return work;
}
static void free_object(obj)
{
if (__free_object(obj))
schedule_work(_obj_work);
}
and then use __free_object() in __debug_check_no_obj_freed()
bool work = false;
...
work |= __free_object(obj);
...
if (work)
schedule_work(_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
Re: [PATCH 2/2 v5] lib: debugobjects: handle objects free in a batch outside the loop
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(_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(>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(>node);
+ hlist_add_head(>node, _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(_to_free, );
+ 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:
if (obj_nr_tofree > 0)
hlist_move_list(_to_free, );
...
if (!hlist_empty()) {
hlist_for_each_entry_safe(obj, tmp, , node) {
hlist_del(>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(>node);
- hlist_add_head(>node, );
+ /* Put obj on the global free list */
+ raw_spin_lock(_lock);
+ hlist_add_head(>node, _to_free);
+ /* Update the counter of objs on the global
freelist */
+ obj_nr_tofree++;
+ raw_spin_unlock(_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(>node, _to_free);
] else {
obj_pool_free++;
hlist_add_head(>node, _pool);
}
unlock(pool);
return work;
}
static void free_object(obj)
{
if (__free_object(obj))
schedule_work(_obj_work);
}
and then use __free_object() in __debug_check_no_obj_freed()
bool work = false;
...
work |= __free_object(obj);
...
if (work)
schedule_work(_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
Re: [PATCH 2/2 v5] lib: debugobjects: handle objects free in a batch outside the loop
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(_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(>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(>node);
+ hlist_add_head(>node, _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(_to_free, );
+ 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:
if (obj_nr_tofree > 0)
hlist_move_list(_to_free, );
...
if (!hlist_empty()) {
hlist_for_each_entry_safe(obj, tmp, , node) {
hlist_del(>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(>node);
- hlist_add_head(>node, );
+ /* Put obj on the global free list */
+ raw_spin_lock(_lock);
+ hlist_add_head(>node, _to_free);
+ /* Update the counter of objs on the global
freelist */
+ obj_nr_tofree++;
+ raw_spin_unlock(_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(>node, _to_free);
] else {
obj_pool_free++;
hlist_add_head(>node, _pool);
}
unlock(pool);
return work;
}
static void free_object(obj)
{
if (__free_object(obj))
schedule_work(_obj_work);
}
and then use __free_object() in __debug_check_no_obj_freed()
bool work = false;
...
work |= __free_object(obj);
...
if (work)
schedule_work(_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
Re: [PATCH 2/2 v5] lib: debugobjects: handle objects free in a batch outside the loop
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(_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(>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(>node);
> + hlist_add_head(>node, _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(_to_free, );
> + 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(>node);
> - hlist_add_head(>node, );
> + /* Put obj on the global free list */
> + raw_spin_lock(_lock);
> + hlist_add_head(>node, _to_free);
> + /* Update the counter of objs on the global
> freelist */
> + obj_nr_tofree++;
> + raw_spin_unlock(_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(>node, _to_free);
] else {
obj_pool_free++;
hlist_add_head(>node, _pool);
}
unlock(pool);
return work;
}
static void free_object(obj)
{
if (__free_object(obj))
schedule_work(_obj_work);
}
and then use __free_object() in __debug_check_no_obj_freed()
bool work = false;
...
work |= __free_object(obj);
...
if (work)
schedule_work(_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
Re: [PATCH 2/2 v5] lib: debugobjects: handle objects free in a batch outside the loop
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(_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(>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(>node);
> + hlist_add_head(>node, _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(_to_free, );
> + 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(>node);
> - hlist_add_head(>node, );
> + /* Put obj on the global free list */
> + raw_spin_lock(_lock);
> + hlist_add_head(>node, _to_free);
> + /* Update the counter of objs on the global
> freelist */
> + obj_nr_tofree++;
> + raw_spin_unlock(_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(>node, _to_free);
] else {
obj_pool_free++;
hlist_add_head(>node, _pool);
}
unlock(pool);
return work;
}
static void free_object(obj)
{
if (__free_object(obj))
schedule_work(_obj_work);
}
and then use __free_object() in __debug_check_no_obj_freed()
bool work = false;
...
work |= __free_object(obj);
...
if (work)
schedule_work(_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
