On Tue, 23 Nov 2021 15:19:49 +0100
Vlastimil Babka <vbabka@xxxxxxx> wrote:
> On 11/22/21 21:33, Gerald Schaefer wrote:
> > On Mon, 22 Nov 2021 21:14:00 +0100
> > Gerald Schaefer <gerald.schaefer@xxxxxxxxxxxxx> wrote:
> >
> > [...]
> >>
> >> 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;
>
> 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.
>
> >>
> >> 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 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?
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(-)
diff --git a/mm/slub.c b/mm/slub.c
index a8626825a829..effb7b8d8f88 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -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 = {
--
2.32.0
