From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751941AbeBAVgq (ORCPT ); Thu, 1 Feb 2018 16:36:46 -0500 Received: from Galois.linutronix.de ([146.0.238.70]:49004 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751715AbeBAVgi (ORCPT ); Thu, 1 Feb 2018 16:36:38 -0500 Date: Thu, 1 Feb 2018 22:36:32 +0100 (CET) From: Thomas Gleixner To: Yang Shi cc: longman@redhat.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/2 v5] lib: debugobjects: handle objects free in a batch outside the loop In-Reply-To: <1517508959-63958-2-git-send-email-yang.shi@linux.alibaba.com> Message-ID: References: <1517508959-63958-1-git-send-email-yang.shi@linux.alibaba.com> <1517508959-63958-2-git-send-email-yang.shi@linux.alibaba.com> User-Agent: Alpine 2.21 (DEB 202 2017-01-01) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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