Re: [patch 1/14] s390: statistics infrastructure.

[Andrew Morton <akpm@osdl.org>]

Martin Peschke3 <MPESCHKE@de.ibm.com> wrote:
>
> Andrew,
> 
> > I agree with Christoph on this: please present it as a Kconfigurable
> > generic option
> 
> Sure. Scanning through the kernel configuration with menuconfig, I see
> several options:
>   general setup,
>   library routines,
>   profiling support,
>   device drivers (the most obvious potential exploiters,
>                   though not the only ones),
>   kernel hacking.
> Please advise.

Nothing really fits, does it.

There is a patch in -mm which moves oprofile and kprobes into a new
"instrumentation" menu.

ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.14-rc5/2.6.14-rc5-mm1/broken-out/moving-kprobes-and-oprofile-to-instrumentation-support-menu.patch

I held off on merging that because the oprofile guys asked "why bother".  I
guess the statistics infrastructure answers that question.  I'll send it
on.

> 
> ...
>
> > (If we end up deciding to keep all this in arch/s390 then I guess
> > we can live with s390 peculiarities though)
> 
> I will be happy to see some feature like this included outside arch/s390.
> What is about lib/, or kernel/?

lib/, I guess.

It could concievably go in fs/debugfs, depending upon how tightly coupled
it is to debugfs.

> > > +        list_for_each_entry(seg, &rb->seg_lh, list)
> > > +                    break;
> >
> > eh?   Something's gone rather wrong here.
> 
> Intention here is to find the entry at the list's head.
> Should work, though it might look fishy at first sight.
> A substitute for this construct would look like this:
> 
>    seg = list_entry(rb->seg_lh.next, struct sgrb_seg, list);
> 
> I don't feel very comfortably messing with pointers inside struct
> list_head. I know many people don't object. But IMHO the next and prev
> pointers look like implementation specifics of list heads that are nicely
> abstracted away for almost any purpose by list_for_each() and friends -
> with the exception of something like the above.
> 
> Anyway, I will change these and similar lines if you want me to.

Yes, we do poke inside list_head a lot and yes, it does feel a bit wrong.

Do whatever you think is best here.  A little explanatory comment would
help.

> > > +/**
> > > + * sgrb_produce_nooverwrite - put an entry into the ringbuffer
> > > + *     if there is room whithout the need to overwrite the oldest
> >
> > spello
> 
> Thanks. Will fix it along with a few more that have gone unnoticed so far.

I noticed that there was a /* in there somewhere where a kerneldoc /** was
intended.

> I don't think it would be beneficial to do locking inside these
> functions, because exploiters most likely do some locking anyway.

Yes, caller-provided locking is superior.

> ...
> > Can you help us decide whether there's actually any point in making
> > it generic?
> > What problems was it designed to solve, and what additional
> > problems might it all be useful for?
> 
> I am convinced that its worthwile to provide some statictics facility
> for any device driver programmer or whoever thinks about implementing
> statistics for his devices or algorithms.

OK, thanks.  Let's take a closer look at the actual facilities in the next
iteration.