mm/slub: endless "No data" printing for alloc/free_traces attribute
Subsystems (3)
cover mm/slub: endless "No data" printing for alloc/free_traces attribute
Steffen reported endless printing of "No data" when reading from alloc/free_traces attribute in /sys/kernel/debug/slab/, at least on s390, but I don't see how this could be arch-specific.
I must admit that I am completely confused by the seq_file interface in general, but even more so from this specific implementation.
First I suspected a bug in slab_debugfs_next(), because of its very weird usage of the *v parameter, which seems totally useless, but not responsible for this bug. Still, out of curiosity, does anybody have a clue why it is done this way, and not e.g. like this?
mm/slub.c +1 -2
@@ -6105,10 +6105,9 @@ static void *slab_debugfs_next(struct seq_file *seq, void *v, loff_t *ppos) {struct loc_track *t = seq->private; - v = ppos; ++*ppos;if (*ppos <= t->count) -return v; +return ppos;return NULL ; }
Anyway, NULL is returned for the "*ppos > t->count" case, as described in Documentation/filesystems/seq_file.rst, for "if the sequence is complete", so that should be ok, but still very confusing. Unfortunately, the documentation does not seem to explain that *v parameter at all, or I missed it, but in this case here it seems to be simply ignored and misused w/o any need.
The documentation does mention that "in most cases the start() function should check for a "past end of file" condition and return NULL if need be". So I just added a similar check to slab_debugfs_start() and return NULL for "*ppos > t->count", which apparently fixed the bug. "No data" is now only printed once, like it was before commit 64dd68497be7 ("mm: slub: move sysfs slab alloc/free interfaces to debugfs").
However, since I obviously do not really understand the seq_file documentation, and also not the alloc/free_traces stuff in general, that fix might be wrong or incomplete. Comments are welcome.
Gerald Schaefer (1):
mm/slub: fix endless "No data" printing for alloc/free_traces
attribute
mm/slub.c | 5 +++++ 1 file changed, 5 insertions(+)
Signature · 1 line
2.25.1
RFC PATCH 1/1 mm/slub: fix endless "No data" printing for alloc/free_traces attribute
Reading from alloc/free_traces attribute in /sys/kernel/debug/slab/ results in an endless sequence of "No data". This is because slab_debugfs_start() does not check for a "past end of file" condition and return NULL.
Fix it by adding such a check and return NULL.
--- mm/slub.c | 5 +++++ 1 file changed, 5 insertions(+)
mm/slub.c +5 -0
@@ -6115,6 +6115,11 @@ static void *slab_debugfs_next(struct seq_file *seq, void *v, loff_t *ppos)static void *slab_debugfs_start (struct seq_file *seq,loff_t *ppos) { +struct loc_track *t = seq->private; + +if (*ppos > t->count) +return NULL ; +return ppos; }
Signature · 1 line
2.25.1
Re: [RFC PATCH 1/1] mm/slub: fix endless "No data" printing for alloc/free_traces attribute
Quoted 3 lines · Gerald Schaefer
Reading from alloc/free_traces attribute in
/sys/kernel/debug/slab/results in an endless sequence of "No data". This is becauseslab_debugfs_start()does not check for a "past end of file" condition and return NULL.
I still have no idea how that endless sequence happens.
To get it, we would have to call slab_debugfs_show() repeatedly with such v that *v == 0. Which should only happen with slab_debugfs_start() with *ppos == 0. Which your patch won't change because you add a '*ppos > t->count' condition, so *ppos has to be at least 1 to trigger this.
But yeah, AFAIK we should detect this in slab_debugfs_start() anyway. But I think the condition should be something like below, because we are past end of file already with *ppos == t->count. But if both are 0, we want to proceed for the "No data" output.
// to show the No data
if (!*ppos && !t->count)
return ppos;
if (*ppos >= t->count)
return ppos;
return ppos;
Quoted 27 lines
Fix it by adding such a check and return NULL.
Fixes 64dd68497be7 ("mm: slub: move sysfs slab alloc/free interfaces to debugfs")Cc <[email protected]> # v5.14+Reported-by Steffen Maier <[email protected]>Signed-off-by Gerald Schaefer <[email protected]>--- mm/slub.c | 5 +++++ 1 file changed, 5 insertions(+)mm/slub.c +5
-0@@ -6115,6 +6115,11 @@ static void *slab_debugfs_next(struct seq_file *seq, void *v, loff_t *ppos)static void *slab_debugfs_start (struct seq_file *seq,loff_t *ppos) { +struct loc_track *t = seq->private; + +if (*ppos > t->count) +return NULL ; +return ppos; }
Re: [RFC PATCH 1/1] mm/slub: fix endless "No data" printing for alloc/free_traces attribute
Quoted 10 lines · On Fri, 19 Nov 2021 11:41:38 +0100 Vlastimil Babka <[email protected]> wrote:
Quoted 3 lines · Gerald Schaefer
Reading from alloc/free_traces attribute in
/sys/kernel/debug/slab/results in an endless sequence of "No data". This is becauseslab_debugfs_start()does not check for a "past end of file" condition and return NULL.I still have no idea how that endless sequence happens.
To get it, we would have to callslab_debugfs_show()repeatedly with such v that *v == 0. Which should only happen withslab_debugfs_start()with *ppos == 0. Which your patch won't change because you add a '*ppos >t->count' condition, so *ppos has to be at least 1 to trigger this.
Yes, very strange. After a closer look to fs/seq_file.c, especially seq_read_iter(), it seems that op->next will only be called when m->count == 0, at least in the first while(1) loop. Printing "No data\n" sets m->count to 8, so it will continue after Fill:, then call op->next, which returns NULL and breaks the second while(1) loop, and also calls op->stop. Then it returns from seq_read_iter(), only to be called again, and again, ...
Only when op->start returns NULL it will end it for good, probably because seq_read_iter() will then return 0 instead of 8. Not sure if there is a better way to fix this than by adding a second "return NULL" to op->start, which feels a bit awkward and makes you wonder why the "return NULL" from op->next is not enough.
Quoted 5 lines
But yeah, AFAIK we should detect this in
slab_debugfs_start()anyway. But I think the condition should be something like below, because we are past end of file already with *ppos ==t->count. But if both are 0, we want to proceed for the "No data" output.
Ah ok, I wasn't sure about the "t->count > 0" case, i.e. if the check for "*ppos > t->count" would still be correct there. So apparently it wouldn't, and we need two checks, like you suggested
Quoted 7 lines
// to show the No data
if (!*ppos && !t->count)return ppos;if (*ppos >=
t->count)return ppos;
That should be return NULL here, right?
Quoted 3 lines
return ppos;
Will send a new patch, unless I find a better way after investigating the endless seq_read_iter() calls mentioned above.
Is there an easy way to test the "t->count > 0" case, i.e. what would need to be done to get some other reply than "No data"?
Re: [RFC PATCH 1/1] mm/slub: fix endless "No data" printing for alloc/free_traces attribute
Quoted 21 lines · Gerald Schaefer
Quoted 10 lines · On Fri, 19 Nov 2021 11:41:38 +0100 Vlastimil Babka <[email protected]> wrote:
Quoted 3 lines · Gerald Schaefer
Reading from alloc/free_traces attribute in
/sys/kernel/debug/slab/results in an endless sequence of "No data". This is becauseslab_debugfs_start()does not check for a "past end of file" condition and return NULL.I still have no idea how that endless sequence happens.
To get it, we would have to callslab_debugfs_show()repeatedly with such v that *v == 0. Which should only happen withslab_debugfs_start()with *ppos == 0. Which your patch won't change because you add a '*ppos >t->count' condition, so *ppos has to be at least 1 to trigger this.Yes, very strange. After a closer look to
fs/seq_file.c, especiallyseq_read_iter(), it seems thatop->nextwill only be called whenm->count== 0, at least in the first while(1) loop. Printing "No data\n" setsm->countto 8, so it will continue after Fill:, then callop->next, which returns NULL and breaks the second while(1) loop, and also callsop->stop. Then it returns fromseq_read_iter(), only to be called again, and again, ...
Quoted 2 lines
Only when
op->startreturns NULL it will end it for good, probably becauseseq_read_iter()will then return 0 instead of 8.
Ah, thanks for investigating.
Quoted 4 lines
Not sure if
there is a better way to fix this than by adding a second "return NULL" toop->start, which feels a bit awkward and makes you wonder why the "return NULL" fromop->nextis not enough.
I think it's fine to require op->start to return NULL, even if it didn't cause this infinite loop.
Quoted 11 lines
Quoted 5 lines
But yeah, AFAIK we should detect this in
slab_debugfs_start()anyway. But I think the condition should be something like below, because we are past end of file already with *ppos ==t->count. But if both are 0, we want to proceed for the "No data" output.Ah ok, I wasn't sure about the "
t->count> 0" case, i.e. if the check for "*ppos >t->count" would still be correct there. So apparently it wouldn't, and we need two checks, like you suggestedQuoted 1 line
Quoted 8 lines
Quoted 6 lines
// to show the No data
if (!*ppos && !t->count)return ppos;if (*ppos >=
t->count)return ppos;That should be return NULL here, right?
Doh, right.
Quoted 8 lines
Quoted 3 lines
return ppos;
Will send a new patch, unless I find a better way after investigating the endless
seq_read_iter()calls mentioned above.
Is there an easy way to test the "t->count> 0" case, i.e. what would need to be done to get some other reply than "No data"?
Hm the debugfs files alloc_tracess/free_traces for any cache with non-zero objects (see /proc/slabinfo for that) should have t->count > 0. If the files are created for a cache, it means the related SLAB_STORE_USER debugging was enabled both during config and boot-time. If you see only a few caches with alloc_tracess/free_traces (because they are from e.g. some test module that adds SLAB_STORE_USER explicitly) and all happen to have 0 objects, boot with slub_debug=U parameter and then all caches will have this enabled and many will have >0 objects.
Re: [RFC PATCH 1/1] mm/slub: fix endless "No data" printing for alloc/free_traces attribute
Quoted 27 lines · On Mon, 22 Nov 2021 16:02:28 +0100 Vlastimil Babka <[email protected]> wrote:
Quoted 21 lines · Gerald Schaefer
Quoted 10 lines · On Fri, 19 Nov 2021 11:41:38 +0100 Vlastimil Babka <[email protected]> wrote:
Quoted 3 lines · Gerald Schaefer
Reading from alloc/free_traces attribute in
/sys/kernel/debug/slab/results in an endless sequence of "No data". This is becauseslab_debugfs_start()does not check for a "past end of file" condition and return NULL.I still have no idea how that endless sequence happens.
To get it, we would have to callslab_debugfs_show()repeatedly with such v that *v == 0. Which should only happen withslab_debugfs_start()with *ppos == 0. Which your patch won't change because you add a '*ppos >t->count' condition, so *ppos has to be at least 1 to trigger this.Yes, very strange. After a closer look to
fs/seq_file.c, especiallyseq_read_iter(), it seems thatop->nextwill only be called whenm->count== 0, at least in the first while(1) loop. Printing "No data\n" setsm->countto 8, so it will continue after Fill:, then callop->next, which returns NULL and breaks the second while(1) loop, and also callsop->stop. Then it returns fromseq_read_iter(), only to be called again, and again, ...Quoted 2 lines
Only when
op->startreturns NULL it will end it for good, probably becauseseq_read_iter()will then return 0 instead of 8.Ah, thanks for investigating.
Quoted 7 lines
Quoted 4 lines
Not sure if
there is a better way to fix this than by adding a second "return NULL" toop->start, which feels a bit awkward and makes you wonder why the "return NULL" fromop->nextis not enough.I think it's fine to require
op->startto return NULL, even if it didn't cause this infinite loop.
Yes, this all seems to work as designed, and it is slowly beginning to make some sense. The only other way to stop it would be to return from op->show, w/o printing anything, like it would be done for t->count > 0, when idx becomes >= t->count. In theory at least, see below.
Quoted 32 lines
Quoted 12 lines
Quoted 6 lines
But yeah, AFAIK we should detect this in
slab_debugfs_start()anyway. But I think the condition should be something like below, because we are past end of file already with *ppos ==t->count. But if both are 0, we want to proceed for the "No data" output.Ah ok, I wasn't sure about the "
t->count> 0" case, i.e. if the check for "*ppos >t->count" would still be correct there. So apparently it wouldn't, and we need two checks, like you suggestedQuoted 1 line
Quoted 8 lines
Quoted 6 lines
// to show the No data
if (!*ppos && !t->count)return ppos;if (*ppos >=
t->count)return ppos;That should be return NULL here, right?
Doh, right.
Quoted 8 lines
Quoted 3 lines
return ppos;
Will send a new patch, unless I find a better way after investigating the endless
seq_read_iter()calls mentioned above.
Is there an easy way to test the "t->count> 0" case, i.e. what would need to be done to get some other reply than "No data"?
Quoted 8 lines
Hm the debugfs files alloc_tracess/free_traces for any cache with non-zero objects (see
/proc/slabinfofor that) should havet->count> 0. If the files are created for a cache, it means the relatedSLAB_STORE_USERdebugging was enabled both during config and boot-time. If you see only a few caches with alloc_tracess/free_traces (because they are from e.g. some test module that addsSLAB_STORE_USERexplicitly) and all happen to have 0 objects, boot with slub_debug=U parameter and then all caches will have this enabled and many will have >0 objects.
Thanks. While testing this properly, yet another bug showed up. The idx in op->show remains 0 in all iterations, so I always see the same line printed t->count times (or infinitely, ATM). Not sure if this only shows on s390 due to endianness, but the reason is this:
unsigned int idx = *(unsigned int *)v;
IIUC, void *v is always the same as loff_t *ppos, and therefore idx also should be *ppos. De-referencing the loff_t * with an unsigned int * only gives the upper 32 bit half of the 64 bit value, which remains 0.
This would be fixed e.g. with
unsigned int idx = (unsigned int) *(loff_t *) v;
With this fixed, my original patch actually also works for t->count > 0, because then op->show would return w/o printing anything when idx reaches t->count. For t->count > 0, it would even work w/o any extra checks in op->start because of that, only "No data" would be printed infinitely.
It probably still makes sense to make this explicit in op->start, by checking separately for !*ppos && !t->count, and returning NULL for *ppos >= t->count, as you suggested.
I think I will also make idx an unsigned long again, like it was before commit 64dd68497be7, and similar to t->count. Not sure if it needs to be, and with proper casting unsigned int is also possible, but why change it?
Re: [RFC PATCH 1/1] mm/slub: fix endless "No data" printing for alloc/free_traces attribute
On Mon, 22 Nov 2021 21:14:00 +0100
Gerald Schaefer <[email protected]> wrote:
[...]
Quoted 12 lines
Thanks. While testing this properly, yet another bug showed up. The idx in
op->showremains 0 in all iterations, so I always see the same line printedt->counttimes (or infinitely, ATM). Not sure if this only shows on s390 due to endianness, but the reason is this:unsigned int idx = *(unsigned int *)v;IIUC, void *v is always the same as
loff_t*ppos, and therefore idx also should be *ppos. De-referencing theloff_t* with an unsigned int * only gives the upper 32 bit half of the 64 bit value, which remains 0.
Quoted 8 lines
This would be fixed e.g. with
unsigned int idx = (unsigned int) *(loff_t *) v;
With this fixed, my original patch actually also works for
t->count> 0, because thenop->showwould return w/o printing anything when idx reachest->count. Fort->count> 0, it would even work w/o any extra checks inop->startbecause of that, only "No data" would be printed infinitely.
Oh, no, that would actually also fix the "No data" part, as op->show will then also return w/o printing in the next iteration, so that op->next would correctly end it all.
This could also explain why it might all have worked fine on x86 (haven't verified), and really only showed on big-endian s390.
Hmm, now I'm not so sure anymore if we really want the additional checks and return NULL in op->start, just to make it "double safe".
Quoted 9 lines
It probably still makes sense to make this explicit in
op->start, by checking separately for !*ppos && !t->count, and returning NULL for *ppos >=t->count, as you suggested.I think I will also make idx an unsigned long again, like it was before commit
64dd68497be7, and similar tot->count. Not sure if it needs to be, and with proper casting unsigned int is also possible, but why change it?
Re: [RFC PATCH 1/1] mm/slub: fix endless "No data" printing for alloc/free_traces attribute
Quoted 11 lines · Gerald Schaefer
On Mon, 22 Nov 2021 21:14:00 +0100
Gerald Schaefer <[email protected]> wrote:[...]
Quoted 7 lines
Thanks. While testing this properly, yet another bug showed up. The idx in
op->showremains 0 in all iterations, so I always see the same line printedt->counttimes (or infinitely, ATM). Not sure if this only shows on s390 due to endianness, but the reason is this:unsigned int idx = *(unsigned int *)v;
Uh, good catch. I was actually looking suspiciously at how we cast signed to unsigned, but didn't occur to me that shortening together with endiannes is the problem.
Quoted 8 lines
Quoted 8 lines
IIUC, void *v is always the same as
loff_t*ppos, and therefore idx also should be *ppos. De-referencing theloff_t* with an unsigned int * only gives the upper 32 bit half of the 64 bit value, which remains 0.This would be fixed e.g. with
unsigned int idx = (unsigned int) *(loff_t *) v;
With all this experience I'm now inclined to rather follow more the example in Documentation/filesystems/seq_file.rst and don't pass around the pointer that we got as ppos in slab_debugfs_start(), and that seq_file.c points to m->index.
In that example an own value is kmalloced:
loff_t *spos = kmalloc(sizeof(loff_t), GFP_KERNEL);
while we could just make this a field of loc_track?
Quoted 9 lines
Quoted 4 lines
With this fixed, my original patch actually also works for
t->count> 0, because thenop->showwould return w/o printing anything when idx reachest->count. Fort->count> 0, it would even work w/o any extra checks inop->startbecause of that, only "No data" would be printed infinitely.Oh, no, that would actually also fix the "No data" part, as
op->showwill then also return w/o printing in the next iteration, so thatop->nextwould correctly end it all.
Quoted 5 lines
This could also explain why it might all have worked fine on x86 (haven't verified), and really only showed on big-endian s390.
Hmm, now I'm not so sure anymore if we really want the additional checks and return NULL in
op->start, just to make it "double safe".
I guess we don't.
Quoted 10 lines
Quoted 9 lines
It probably still makes sense to make this explicit in
op->start, by checking separately for !*ppos && !t->count, and returning NULL for *ppos >=t->count, as you suggested.I think I will also make idx an unsigned long again, like it was before commit
64dd68497be7, and similar tot->count. Not sure if it needs to be, and with proper casting unsigned int is also possible, but why change it?
Re: [RFC PATCH 1/1] mm/slub: fix endless "No data" printing for alloc/free_traces attribute
Quoted 31 lines · On Tue, 23 Nov 2021 15:19:49 +0100 Vlastimil Babka <[email protected]> wrote:
Quoted 11 lines · Gerald Schaefer
On Mon, 22 Nov 2021 21:14:00 +0100
Gerald Schaefer <[email protected]> wrote:[...]
Quoted 7 lines
Thanks. While testing this properly, yet another bug showed up. The idx in
op->showremains 0 in all iterations, so I always see the same line printedt->counttimes (or infinitely, ATM). Not sure if this only shows on s390 due to endianness, but the reason is this:unsigned int idx = *(unsigned int *)v;Uh, good catch. I was actually looking suspiciously at how we cast signed to unsigned, but didn't occur to me that shortening together with endiannes is the problem.
Quoted 8 lines
Quoted 8 lines
IIUC, void *v is always the same as
loff_t*ppos, and therefore idx also should be *ppos. De-referencing theloff_t* with an unsigned int * only gives the upper 32 bit half of the 64 bit value, which remains 0.This would be fixed e.g. with
unsigned int idx = (unsigned int) *(loff_t *) v;
With all this experience I'm now inclined to rather follow more the example in
Documentation/filesystems/seq_file.rstand don't pass around the pointer that we got as ppos inslab_debugfs_start(), and thatseq_file.cpoints tom->index.
Quoted 5 lines
In that example an own value is kmalloced:
loff_t*spos =kmalloc(sizeof(loff_t), GFP_KERNEL);while we could just make this a field of
loc_track?
Yes, following the example sounds good, and it would also make proper use of *v in op->next, which might make the code more readable. It also looks like it already does exactly what is needed here, i.e. have a simple iterator that just counts the lines.
I don't think the iterator needs to be saved in loc_track. IIUC, it is already passed around like in the example, and can then be simply compared to t->count, similar to the existing code.
This is what I'm currently testing, and it seems to work fine. Will send a new patch, if there are no objections:
--- mm/slub.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-)
mm/slub.c +13 -8
@@ -6052,10 +6052,9 @@ __initcall(slab_sysfs_init);#if defined(CONFIG_SLUB_DEBUG ) && defined(CONFIG_DEBUG_FS )static int slab_debugfs_show (struct seq_file *seq,void *v) { - -struct location *l; -unsigned int idx = *(unsigned int *)v;struct loc_track *t = seq->private; +loff_t idx = *(loff_t *) v; +struct location *l;if (idx < t->count) { l = &t->loc[idx];
@@ -6099,23 +6098,29 @@ static int slab_debugfs_show(struct seq_file *seq, void *v)static void slab_debugfs_stop (struct seq_file *seq,void *v) { +kfree (v); }static void *slab_debugfs_next (struct seq_file *seq,void *v,loff_t *ppos) {struct loc_track *t = seq->private; +loff_t *idxp = (loff_t *) v; - v = ppos; - ++*ppos; -if (*ppos <= t->count) -return v; + *ppos = ++(*idxp); +if (*idxp <= t->count) +return idxp;return NULL ; }static void *slab_debugfs_start (struct seq_file *seq,loff_t *ppos) { -return ppos; +loff_t *idxp =kmalloc (sizeof (loff_t),GFP_KERNEL ); + +if (!idxp) +return NULL ; + *idxp = *ppos; +return idxp; }static const struct seq_operations slab_debugfs_sops = {
Signature · 1 line
2.32.0
Re: [RFC PATCH 1/1] mm/slub: fix endless "No data" printing for alloc/free_traces attribute
Quoted 45 lines · On Thu, 25 Nov 2021 17:13:10 +0100 Gerald Schaefer…
Quoted 31 lines · On Tue, 23 Nov 2021 15:19:49 +0100 Vlastimil Babka <[email protected]> wrote:
Quoted 11 lines · Gerald Schaefer
On Mon, 22 Nov 2021 21:14:00 +0100
Gerald Schaefer <[email protected]> wrote:[...]
Quoted 7 lines
Thanks. While testing this properly, yet another bug showed up. The idx in
op->showremains 0 in all iterations, so I always see the same line printedt->counttimes (or infinitely, ATM). Not sure if this only shows on s390 due to endianness, but the reason is this:unsigned int idx = *(unsigned int *)v;Uh, good catch. I was actually looking suspiciously at how we cast signed to unsigned, but didn't occur to me that shortening together with endiannes is the problem.
Quoted 8 lines
Quoted 8 lines
IIUC, void *v is always the same as
loff_t*ppos, and therefore idx also should be *ppos. De-referencing theloff_t* with an unsigned int * only gives the upper 32 bit half of the 64 bit value, which remains 0.This would be fixed e.g. with
unsigned int idx = (unsigned int) *(loff_t *) v;
With all this experience I'm now inclined to rather follow more the example in
Documentation/filesystems/seq_file.rstand don't pass around the pointer that we got as ppos inslab_debugfs_start(), and thatseq_file.cpoints tom->index.Quoted 5 lines
In that example an own value is kmalloced:
loff_t*spos =kmalloc(sizeof(loff_t), GFP_KERNEL);while we could just make this a field of
loc_track?Yes, following the example sounds good, and it would also make proper use of *v in
op->next, which might make the code more readable. It also looks like it already does exactly what is needed here, i.e. have a simple iterator that just counts the lines.
Quoted 6 lines
I don't think the iterator needs to be saved in
loc_track. IIUC, it is already passed around like in the example, and can then be simply compared tot->count, similar to the existing code.This is what I'm currently testing, and it seems to work fine. Will send a new patch, if there are no objections:
Oh well, I have one objection, returning NULL from op->next will be passed to op->stop, and then it will not free the allocated value.
The example is elegantly avoiding this, by not returning NULL anywhere, and also not stopping. Sigh.
Maybe not return NULL in op->next, but only from op->start, and only when no allocation was made or it was freed already? Or free it only/ already in op->next, when returning NULL?
Re: [RFC PATCH 1/1] mm/slub: fix endless "No data" printing for alloc/free_traces attribute
Quoted 43 lines · Gerald Schaefer
Quoted 40 lines · On Thu, 25 Nov 2021 17:13:10 +0100 Gerald Schaefer…
Quoted 36 lines · On Tue, 23 Nov 2021 15:19:49 +0100 Vlastimil Babka <[email protected]> wrote:
Quoted 11 lines · Gerald Schaefer
On Mon, 22 Nov 2021 21:14:00 +0100
Gerald Schaefer <[email protected]> wrote:[...]
Quoted 7 lines
Thanks. While testing this properly, yet another bug showed up. The idx in
op->showremains 0 in all iterations, so I always see the same line printedt->counttimes (or infinitely, ATM). Not sure if this only shows on s390 due to endianness, but the reason is this:unsigned int idx = *(unsigned int *)v;Uh, good catch. I was actually looking suspiciously at how we cast signed to unsigned, but didn't occur to me that shortening together with endiannes is the problem.
Quoted 8 lines
Quoted 8 lines
IIUC, void *v is always the same as
loff_t*ppos, and therefore idx also should be *ppos. De-referencing theloff_t* with an unsigned int * only gives the upper 32 bit half of the 64 bit value, which remains 0.This would be fixed e.g. with
unsigned int idx = (unsigned int) *(loff_t *) v;
With all this experience I'm now inclined to rather follow more the example in
Documentation/filesystems/seq_file.rstand don't pass around the pointer that we got as ppos inslab_debugfs_start(), and thatseq_file.cpoints tom->index.In that example an own value is kmalloced:
loff_t*spos =kmalloc(sizeof(loff_t), GFP_KERNEL);while we could just make this a field of
loc_track?
Quoted 8 lines
Quoted 8 lines
Yes, following the example sounds good, and it would also make proper use of *v in
op->next, which might make the code more readable. It also looks like it already does exactly what is needed here, i.e. have a simple iterator that just counts the lines.I don't think the iterator needs to be saved in
loc_track. IIUC, it is already passed around like in the example, and can then be simply compared tot->count, similar to the existing code.
Saving it the loc_track doesn't preclude using the pointer that's being passed around. It however avoids the extra kmalloc and turns out it should also solve the return NULL from op->next() issue you describe below?
Quoted 6 lines
Quoted 2 lines
This is what I'm currently testing, and it seems to work fine. Will send a new patch, if there are no objections:
Oh well, I have one objection, returning NULL from
op->nextwill be passed toop->stop, and then it will not free the allocated value.
Quoted 6 lines
The example is elegantly avoiding this, by not returning NULL anywhere, and also not stopping. Sigh.
Maybe not return NULL in
op->next, but only fromop->start, and only when no allocation was made or it was freed already? Or free it only/ already inop->next, when returning NULL?
From these two probably the "free in op->next". But still inclined to store it in loc_track.
Why does the API need to be so awkward...
Re: [RFC PATCH 1/1] mm/slub: fix endless "No data" printing for alloc/free_traces attribute
Quoted 49 lines · On Thu, 25 Nov 2021 23:00:47 +0100 Vlastimil Babka <[email protected]> wrote:
Quoted 48 lines · Gerald Schaefer
Quoted 45 lines · On Thu, 25 Nov 2021 17:13:10 +0100 Gerald Schaefer…
Quoted 31 lines · On Tue, 23 Nov 2021 15:19:49 +0100 Vlastimil Babka <[email protected]> wrote:
Quoted 11 lines · Gerald Schaefer
On Mon, 22 Nov 2021 21:14:00 +0100
Gerald Schaefer <[email protected]> wrote:[...]
Quoted 7 lines
Thanks. While testing this properly, yet another bug showed up. The idx in
op->showremains 0 in all iterations, so I always see the same line printedt->counttimes (or infinitely, ATM). Not sure if this only shows on s390 due to endianness, but the reason is this:unsigned int idx = *(unsigned int *)v;Uh, good catch. I was actually looking suspiciously at how we cast signed to unsigned, but didn't occur to me that shortening together with endiannes is the problem.
Quoted 8 lines
Quoted 8 lines
IIUC, void *v is always the same as
loff_t*ppos, and therefore idx also should be *ppos. De-referencing theloff_t* with an unsigned int * only gives the upper 32 bit half of the 64 bit value, which remains 0.This would be fixed e.g. with
unsigned int idx = (unsigned int) *(loff_t *) v;
With all this experience I'm now inclined to rather follow more the example in
Documentation/filesystems/seq_file.rstand don't pass around the pointer that we got as ppos inslab_debugfs_start(), and thatseq_file.cpoints tom->index.Quoted 5 lines
In that example an own value is kmalloced:
loff_t*spos =kmalloc(sizeof(loff_t), GFP_KERNEL);while we could just make this a field of
loc_track?Yes, following the example sounds good, and it would also make proper use of *v in
op->next, which might make the code more readable. It also looks like it already does exactly what is needed here, i.e. have a simple iterator that just counts the lines.
Quoted 7 lines
Quoted 3 lines
Quoted 3 lines
I don't think the iterator needs to be saved in
loc_track. IIUC, it is already passed around like in the example, and can then be simply compared tot->count, similar to the existing code.Saving it the
loc_trackdoesn't preclude using the pointer that's being passed around. It however avoids the extra kmalloc and turns out it should also solve the return NULL fromop->next()issue you describe below?
Yes, storing idx in loc_track seems straight-forward, and should also improve code readability.
Patch will follow right here. I made idx a loff_t, as it seemed easier to handle in op->start/next, then casted to unsigned long again in op->show. Not sure if the unsigned int had any benefit. Given that loff_t is a signed 64 bit type, I guess both are wrong if it should ever turn negative (can it, i.e. can ppos turn negative?). With unsigned long, chances are better that it will at least turn into something big enough to not pass the "idx < t->count" check.
Quoted 10 lines
Quoted 10 lines
Quoted 3 lines
This is what I'm currently testing, and it seems to work fine. Will send a new patch, if there are no objections:
Oh well, I have one objection, returning NULL from
op->nextwill be passed toop->stop, and then it will not free the allocated value.The example is elegantly avoiding this, by not returning NULL anywhere, and also not stopping. Sigh.
Quoted 7 lines
Quoted 3 lines
Maybe not return NULL in
op->next, but only fromop->start, and only when no allocation was made or it was freed already? Or free it only/ already inop->next, when returning NULL?From these two probably the "free in
op->next". But still inclined to store it inloc_track.
Why does the API need to be so awkward...
Maybe we are just holding it wrong :-)
PATCH 1/1 mm/slub: fix endianness bug for alloc/free_traces attributes
In reply to Vlastimil BabkaOn big-endian s390, the alloc/free_traces attributes produce endless output, because of always 0 idx in slab_debugfs_show().
idx is de-referenced from *v, which points to a loff_t value, with
unsigned int idx = *(unsigned int *)v;
This will only give the upper 32 bits on big-endian, which remain 0.
Instead of only fixing this de-reference, during discussion it seemed more appropriate to change the seq_ops so that they use an explicit iterator in private loc_track struct.
This patch adds idx to loc_track, which will also fix the endianness bug.
--- mm/slub.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)
mm/slub.c +9 -6
@@ -5081,6 +5081,7 @@ struct loc_track {unsigned long max;unsigned long count;struct location *loc; +loff_t idx; };static struct dentry *slab_debugfs_root;
@@ -6052,11 +6053,11 @@ __initcall(slab_sysfs_init);#if defined(CONFIG_SLUB_DEBUG ) && defined(CONFIG_DEBUG_FS )static int slab_debugfs_show (struct seq_file *seq,void *v) { - -struct location *l; -unsigned int idx = *(unsigned int *)v;struct loc_track *t = seq->private; +struct location *l; +unsigned long idx; + idx = (unsigned long ) t->idx;if (idx < t->count) { l = &t->loc[idx];
@@ -6105,16 +6106,18 @@ static void *slab_debugfs_next(struct seq_file *seq, void *v, loff_t *ppos) {struct loc_track *t = seq->private; - v = ppos; - ++*ppos; + t->idx = ++(*ppos);if (*ppos <= t->count) -return v; +return ppos;return NULL ; }static void *slab_debugfs_start (struct seq_file *seq,loff_t *ppos) { +struct loc_track *t = seq->private; + + t->idx = *ppos;return ppos; }
Signature · 1 line
2.32.0
Re: [PATCH] mm/slub: fix endianness bug for alloc/free_traces attributes
Quoted 13 lines · Gerald Schaefer
On big-endian s390, the alloc/free_traces attributes produce endless output, because of always 0 idx in
slab_debugfs_show().idx is de-referenced from *v, which points to a
loff_tvalue, withunsigned int idx = *(unsigned int *)v;
This will only give the upper 32 bits on big-endian, which remain 0.
Instead of only fixing this de-reference, during discussion it seemed more appropriate to change the
seq_opsso that they use an explicit iterator in privateloc_trackstruct.
Quoted 7 lines
This patch adds idx to
loc_track, which will also fix the endianness bug.Fixes 64dd68497be7 ("mm: slub: move sysfs slab alloc/free interfaces to debugfs")Cc <[email protected]> # v5.14+Reported-by Steffen Maier <[email protected]>Signed-off-by Gerald Schaefer <[email protected]>
With a nit below:
Quoted 32 lines
--- mm/slub.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)mm/slub.c +4
-3@@ -5081,6 +5081,7 @@ struct loc_track {unsigned long max;unsigned long count;struct location *loc; +loff_t idx; };static struct dentry *slab_debugfs_root;@@ -6052,11 +6053,11 @@ __initcall(slab_sysfs_init);#if defined(CONFIG_SLUB_DEBUG ) && defined(CONFIG_DEBUG_FS )static int slab_debugfs_show (struct seq_file *seq,void *v) { - -struct location *l; -unsigned int idx = *(unsigned int *)v;struct loc_track *t = seq->private; +struct location *l; +unsigned long idx; + idx = (unsigned long ) t->idx;if (idx < t->count) { l = &t->loc[idx];
mm/slub.c static void *slab_debugfs_next(struct seq_file *seq, void *v, loff_t *ppos) · line 6111
Quoted 9 lines
mm/slub.c +2
-3@@ -6105,16 +6106,18 @@ static void *slab_debugfs_next(struct seq_file *seq, void *v, loff_t *ppos) {struct loc_track *t = seq->private; - v = ppos; - ++*ppos; + t->idx = ++(*ppos);if (*ppos <= t->count) -return v; +return ppos;
What I had in mind, and to be more in line with the seq_file example, would be to return &t->idx here. Then it's what gets passed to slab_debugfs_show() as 'v'. But since we ignore 'v' there, it probably doesn't matter.
mm/slub.c static void *slab_debugfs_next(struct seq_file *seq, void *v, loff_t *ppos) · line 6121
Quoted 10 lines
mm/slub.c +3
-0return NULL ; }static void *slab_debugfs_start (struct seq_file *seq,loff_t *ppos) { +struct loc_track *t = seq->private; + + t->idx = *ppos;return ppos;
And same here.
Quoted 3 lines
mm/slub.c +0
-0}
Quoted 5 lines · Gerald Schaefer
Steffen reported endless printing of "No data" when reading from alloc/free_traces attribute in
/sys/kernel/debug/slab/, at least on s390, but I don't see how this could be arch-specific.I must admit that I am completely confused by the
seq_fileinterface
Me too, everytime I have to deal with it I relearn it.
Quoted 1 line
in general, but even more so from this specific implementation.
Right.
Quoted 11 lines
First I suspected a bug in
slab_debugfs_next(), because of its very weird usage of the *v parameter, which seems totally useless, but not responsible for this bug. Still, out of curiosity, does anybody have a clue why it is done this way, and not e.g. like this?mm/slub.c +0
-0@@ -6105,10 +6105,9 @@ static void *slab_debugfs_next(struct seq_file *seq, void *v, loff_t *ppos) {
mm/slub.c static void *slab_debugfs_next(struct seq_file *seq, void *v, loff_t *ppos) · line 6113
Quoted 10 lines
mm/slub.c +1
-2struct loc_track *t = seq->private; - v = ppos; ++*ppos;if (*ppos <= t->count) -return v; +return ppos;return NULL ; }
Yeah that should work and make it a bit less confusing.
Quoted 26 lines
Anyway, NULL is returned for the "*ppos >
t->count" case, as described inDocumentation/filesystems/seq_file.rst, for "if the sequence is complete", so that should be ok, but still very confusing. Unfortunately, the documentation does not seem to explain that *v parameter at all, or I missed it, but in this case here it seems to be simply ignored and misused w/o any need.The documentation does mention that "in most cases the
start()function should check for a "past end of file" condition and return NULL if need be". So I just added a similar check toslab_debugfs_start()and return NULL for "*ppos >t->count", which apparently fixed the bug. "No data" is now only printed once, like it was before commit64dd68497be7("mm: slub: move sysfs slab alloc/free interfaces to debugfs").However, since I obviously do not really understand the
seq_filedocumentation, and also not the alloc/free_traces stuff in general, that fix might be wrong or incomplete. Comments are welcome.Gerald Schaefer (1):
mm/slub: fix endless "No data" printing for alloc/free_traces
attributemm/slub.c | 5 +++++ 1 file changed, 5 insertions(+)