LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Gerald Schaefer <gerald.schaefer@linux.ibm.com>
To: Vlastimil Babka <vbabka@suse.cz>
Cc: Faiyaz Mohammed <faiyazm@codeaurora.org>,
	linux-mm <linux-mm@kvack.org>,
	LKML <linux-kernel@vger.kernel.org>,
	linux-s390 <linux-s390@vger.kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [RFC PATCH 1/1] mm/slub: fix endless "No data" printing for alloc/free_traces attribute
Date: Fri, 26 Nov 2021 18:11:49 +0100	[thread overview]
Message-ID: <20211126181149.230d6206@thinkpad> (raw)
In-Reply-To: <7c909b82-8e1c-a8ce-516d-e3aa9bc2fd81@suse.cz>

On Thu, 25 Nov 2021 23:00:47 +0100
Vlastimil Babka <vbabka@suse.cz> wrote:

> On 11/25/21 21:12, Gerald Schaefer wrote:
> > On Thu, 25 Nov 2021 17:13:10 +0100
> > Gerald Schaefer <gerald.schaefer@linux.ibm.com> wrote:
> > 
> >> On Tue, 23 Nov 2021 15:19:49 +0100
> >> Vlastimil Babka <vbabka@suse.cz> wrote:
> >>
> >>> On 11/22/21 21:33, Gerald Schaefer wrote:
> >>>> On Mon, 22 Nov 2021 21:14:00 +0100
> >>>> Gerald Schaefer <gerald.schaefer@linux.ibm.com> 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.
> 
> 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?

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.

> 
> >> 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?
> 
> 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...

Maybe we are just holding it wrong :-)

  reply	other threads:[~2021-11-26 17:14 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-11-17 19:39 [RFC PATCH 0/1] mm/slub: " Gerald Schaefer
2021-11-17 19:39 ` [RFC PATCH 1/1] mm/slub: fix " Gerald Schaefer
2021-11-19 10:41   ` Vlastimil Babka
2021-11-19 19:59     ` Gerald Schaefer
2021-11-22 15:02       ` Vlastimil Babka
2021-11-22 20:14         ` Gerald Schaefer
2021-11-22 20:33           ` Gerald Schaefer
2021-11-23 14:19             ` Vlastimil Babka
2021-11-25 16:13               ` Gerald Schaefer
2021-11-25 20:12                 ` Gerald Schaefer
2021-11-25 22:00                   ` Vlastimil Babka
2021-11-26 17:11                     ` Gerald Schaefer [this message]
2021-11-26 17:18                     ` [PATCH] mm/slub: fix endianness bug for alloc/free_traces attributes Gerald Schaefer
2021-11-29 14:26                       ` Vlastimil Babka
2021-11-19  9:43 ` [RFC PATCH 0/1] mm/slub: endless "No data" printing for alloc/free_traces attribute Vlastimil Babka

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20211126181149.230d6206@thinkpad \
    --to=gerald.schaefer@linux.ibm.com \
    --cc=akpm@linux-foundation.org \
    --cc=faiyazm@codeaurora.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux-s390@vger.kernel.org \
    --cc=vbabka@suse.cz \
    --subject='Re: [RFC PATCH 1/1] mm/slub: fix endless "No data" printing for alloc/free_traces attribute' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).