| Message ID | 1515699458-6925-7-git-send-email-sandeen@sandeen.net (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
On Thu, Jan 11, 2018 at 01:37:36PM -0600, Eric Sandeen wrote: > Fake up the xfs_buf_zone allocated count a bit to account > for buffers freed to and allocated from cache. > > Signed-off-by: Eric Sandeen <sandeen@redhat.com> > Signed-off-by: Eric Sandeen <sandeen@sandeen.net> > --- > libxfs/rdwr.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c > index 474e5eb..c5ffd4d 100644 > --- a/libxfs/rdwr.c > +++ b/libxfs/rdwr.c > @@ -665,6 +665,7 @@ __libxfs_getbufr(int blen) > free(bp->b_maps); > bp->b_maps = NULL; > } > + xfs_buf_zone->allocated++; I'm confused. kmem_zone_zalloc increments zone->allocated and kmem_zone_free decrements it. Now we're messing with ->allocated ourselves, which means that buffers that have ended up on xfs_buf_freelist are still allocated (according to the heap) but the zone accounting thinks the buffer is freed? <goes away, reviews the next patch, comes back> The next patch has xfs_buf_freelist frees the buffer directly and bypassing kmem_zone_free. So I wonder, if you change the next patch to call kmem_zone_free and kill this patch, won't the accounting work out? (xfs buf zone allocated == 0) bp = _zone_alloc() xfs_buf_zone->allocated++ ...do stuff with buffer... xfs_buf_relse(bp) list_add(bp, xfs_buf_freelist) ...buffer still allocated, xfs_buf_zone->allocated == 1... libxfs_bcache_free() _zone_free(bp) xfs_buf_zone->allocated-- ...exit program, and: xfs_buf_zone->allocated == 0 I think killing this patch and teaching the next one to kmem_zone_free the buffer works, right? Or am I missing some subtlety here, in which case said subtlety needs comments. --D > } else > bp = kmem_zone_zalloc(xfs_buf_zone, 0); > pthread_mutex_unlock(&xfs_buf_freelist.cm_mutex); > @@ -1245,6 +1246,7 @@ libxfs_brelse( > "releasing dirty buffer to free list!"); > > pthread_mutex_lock(&xfs_buf_freelist.cm_mutex); > + xfs_buf_zone->allocated--; > list_add(&bp->b_node.cn_mru, &xfs_buf_freelist.cm_list); > pthread_mutex_unlock(&xfs_buf_freelist.cm_mutex); > } > @@ -1268,6 +1270,7 @@ libxfs_bulkrelse( > } > > pthread_mutex_lock(&xfs_buf_freelist.cm_mutex); > + xfs_buf_zone->allocated -= count; > list_splice(list, &xfs_buf_freelist.cm_list); > pthread_mutex_unlock(&xfs_buf_freelist.cm_mutex); > > -- > 1.8.3.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 1/11/18 2:20 PM, Darrick J. Wong wrote: > On Thu, Jan 11, 2018 at 01:37:36PM -0600, Eric Sandeen wrote: >> Fake up the xfs_buf_zone allocated count a bit to account >> for buffers freed to and allocated from cache. >> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com> >> Signed-off-by: Eric Sandeen <sandeen@sandeen.net> >> --- >> libxfs/rdwr.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c >> index 474e5eb..c5ffd4d 100644 >> --- a/libxfs/rdwr.c >> +++ b/libxfs/rdwr.c >> @@ -665,6 +665,7 @@ __libxfs_getbufr(int blen) >> free(bp->b_maps); >> bp->b_maps = NULL; >> } >> + xfs_buf_zone->allocated++; > > I'm confused. kmem_zone_zalloc increments zone->allocated and > kmem_zone_free decrements it. Now we're messing with ->allocated > ourselves, which means that buffers that have ended up on > xfs_buf_freelist are still allocated (according to the heap) but the > zone accounting thinks the buffer is freed? > > <goes away, reviews the next patch, comes back> > > The next patch has xfs_buf_freelist frees the buffer directly and > bypassing kmem_zone_free. So I wonder, if you change the next patch to > call kmem_zone_free and kill this patch, won't the accounting work out? > > (xfs buf zone allocated == 0) > > bp = _zone_alloc() > xfs_buf_zone->allocated++ > > ...do stuff with buffer... > > xfs_buf_relse(bp) > list_add(bp, xfs_buf_freelist) > > ...buffer still allocated, xfs_buf_zone->allocated == 1... > > libxfs_bcache_free() > _zone_free(bp) > xfs_buf_zone->allocated-- > > ...exit program, and: > xfs_buf_zone->allocated == 0 > > I think killing this patch and teaching the next one to kmem_zone_free > the buffer works, right? Or am I missing some subtlety here, in which > case said subtlety needs comments. Yes, it would work out. This patch was trying to fake up actual use by libxfs, because my main goal was to be able to find leaks in libxfs. Counting it as still allocated when libxfs thinks it's put it away seems to confuse that goal. In the end, it's just where I want to draw the line for the accounting. In this way I was hoping to keep the cache details out of that accounting. When libxfs says the buffer is done and puts the last ref, it's considered to be freed by libxfs, whether or not the cache chooses to hang on to it, discard it later, or whatever. Maybe I could fake up a special libxfs_zone_free for the buffer zone which hooks into the cache mechanisms, so we do nothing but normal zone alloc/free from the libxfs perspective? <handwave> -Eric -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 11, 2018 at 02:48:33PM -0600, Eric Sandeen wrote: > > > On 1/11/18 2:20 PM, Darrick J. Wong wrote: > > On Thu, Jan 11, 2018 at 01:37:36PM -0600, Eric Sandeen wrote: > >> Fake up the xfs_buf_zone allocated count a bit to account > >> for buffers freed to and allocated from cache. > >> > >> Signed-off-by: Eric Sandeen <sandeen@redhat.com> > >> Signed-off-by: Eric Sandeen <sandeen@sandeen.net> > >> --- > >> libxfs/rdwr.c | 3 +++ > >> 1 file changed, 3 insertions(+) > >> > >> diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c > >> index 474e5eb..c5ffd4d 100644 > >> --- a/libxfs/rdwr.c > >> +++ b/libxfs/rdwr.c > >> @@ -665,6 +665,7 @@ __libxfs_getbufr(int blen) > >> free(bp->b_maps); > >> bp->b_maps = NULL; > >> } > >> + xfs_buf_zone->allocated++; > > > > I'm confused. kmem_zone_zalloc increments zone->allocated and > > kmem_zone_free decrements it. Now we're messing with ->allocated > > ourselves, which means that buffers that have ended up on > > xfs_buf_freelist are still allocated (according to the heap) but the > > zone accounting thinks the buffer is freed? > > > > <goes away, reviews the next patch, comes back> > > > > The next patch has xfs_buf_freelist frees the buffer directly and > > bypassing kmem_zone_free. So I wonder, if you change the next patch to > > call kmem_zone_free and kill this patch, won't the accounting work out? > > > > (xfs buf zone allocated == 0) > > > > bp = _zone_alloc() > > xfs_buf_zone->allocated++ > > > > ...do stuff with buffer... > > > > xfs_buf_relse(bp) > > list_add(bp, xfs_buf_freelist) > > > > ...buffer still allocated, xfs_buf_zone->allocated == 1... > > > > libxfs_bcache_free() > > _zone_free(bp) > > xfs_buf_zone->allocated-- > > > > ...exit program, and: > > xfs_buf_zone->allocated == 0 > > > > I think killing this patch and teaching the next one to kmem_zone_free > > the buffer works, right? Or am I missing some subtlety here, in which > > case said subtlety needs comments. > > Yes, it would work out. This patch was trying to fake up actual use > by libxfs, because my main goal was to be able to find leaks in libxfs. > Counting it as still allocated when libxfs thinks it's put it away > seems to confuse that goal. > > In the end, it's just where I want to draw the line for the accounting. > > In this way I was hoping to keep the cache details out of that > accounting. When libxfs says the buffer is done and puts the last ref, > it's considered to be freed by libxfs, whether or not the cache chooses > to hang on to it, discard it later, or whatever. It's not the job of the zone allocator to track whether something is cached or not - it just tracks how many objects are allocated. Indeed, the libxfs cache iitself keeps track of how many objects it has remaining in it, and if you define CACHE_DEBUG it will issue a warning if the cache is not empty after it has been purged and is being torn down. i.e. we already have debug mechanisms to tell if we are leaking objects through the cache... Cheers, Dave.
diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c index 474e5eb..c5ffd4d 100644 --- a/libxfs/rdwr.c +++ b/libxfs/rdwr.c @@ -665,6 +665,7 @@ __libxfs_getbufr(int blen) free(bp->b_maps); bp->b_maps = NULL; } + xfs_buf_zone->allocated++; } else bp = kmem_zone_zalloc(xfs_buf_zone, 0); pthread_mutex_unlock(&xfs_buf_freelist.cm_mutex); @@ -1245,6 +1246,7 @@ libxfs_brelse( "releasing dirty buffer to free list!"); pthread_mutex_lock(&xfs_buf_freelist.cm_mutex); + xfs_buf_zone->allocated--; list_add(&bp->b_node.cn_mru, &xfs_buf_freelist.cm_list); pthread_mutex_unlock(&xfs_buf_freelist.cm_mutex); } @@ -1268,6 +1270,7 @@ libxfs_bulkrelse( } pthread_mutex_lock(&xfs_buf_freelist.cm_mutex); + xfs_buf_zone->allocated -= count; list_splice(list, &xfs_buf_freelist.cm_list); pthread_mutex_unlock(&xfs_buf_freelist.cm_mutex);