Subject: Re: [patch v2 0/3] OProfile support for System z's hardware sampling

Heinz,

On 21.01.11 05:06:51, Heinz Graalfs wrote:
> I'm resending yesterday's mail because I missed to specify the correct sender information.
>
> This is a re-posting of the patch series originally posted last month:
>
> http://marc.info/?l=linux-s390&m=129285043619973&w=2
>
> Heinz
>
> Changes in
>
> v2:
> - kernel module hwsampler removed, everything is now in oprofile kernel module
> - functions from hwsampler-main.c and smpctl.c merged into arch/s390/oprofile/hwsampler.c
> - functions made static
> - arch/s390/include/asm/hwsampler.h moved to arch/s390/oprofile/hwsampler.h
> - structs have now hws_ prefix
> - config variables changed, HAVE_HWSAMPLER used only
> - original patch 4 (handle_munmap.patch) removed

thanks for you patches, I have applied them with modifications to:

git://git.kernel.org/pub/scm/linux/kernel/git/rric/oprofile.git s390

For atomic compilation of single patches I changed patch order and the
Makefile where necessary. The branch also includes my follow-on
patches.

I will be travelling the next weeks and wanted to have this series in
linux-next now for later inclusion to .39. We would run out of time
otherwise (my bad). Please review and test the changes I made and send
me possibly updates.

Though I applied the patches I want to have the following changes:

* Merge init.c and hwsampler_file.c, two files are bloated here and
hwsampler_file.c is a bad and too long naming.

* Rework functions in cpu_buffer.c (log_sample,
__oprofile_add_ext_sample, oprofile_add_ext_hw_sample, etc.). All
the (static) functions can be merged to a single functino by
implementing a struct that holds all current function arguments.
Something like:

static void __oprofile_add_ext_sample(struct *foobar fb);
void oprofile_add_sample(struct pt_regs * const regs, unsigned long event)
{
struct foobar fb = { .regs = regs, .event = event };
__oprofile_add_ext_sample(&fb);
}

The naming and description of oprofile_add_ext_hw_sample() is also
not the best. As interface in include/linux/oprofile.h we could then
merge oprofile_add_ext_sample() and oprofile_add_ext_hw_sample() to
a single function.

It would be nice if you could implement this.

For all patches in the oprofile/s390 branch I also need the
s390 maintainer's ack.

Martin or Heiko, please ack.

Thanks,

-Robert

--
Advanced Micro Devices, Inc.
Operating System Research Center


2011-02-15 07:17:08

by Heiko Carstens

[permalink] [raw]
Subject: Re: [patch v2 0/3] OProfile support for System z's hardware sampling

On Mon, Feb 14, 2011 at 08:42:06PM +0100, Robert Richter wrote:
> For all patches in the oprofile/s390 branch I also need the
> s390 maintainer's ack.
>
> Martin or Heiko, please ack.

Acked-by: Heiko Carstens <[email protected]>

2011-02-15 17:04:47

by Heinz Graalfs

[permalink] [raw]
Subject: Re: [patch v2 0/3] OProfile support for System z's hardware sampling

Hello Robert,

thanks for the positive reply.

Please see my comments below. I suppose I still need some more detailed
directions from you.
Unfortunately I will not be in the office Thursday and Friday.

Heinz

On Mon, 2011-02-14 at 20:42 +0100, Robert Richter wrote:
> Heinz,
>
> On 21.01.11 05:06:51, Heinz Graalfs wrote:
> > I'm resending yesterday's mail because I missed to specify the correct sender information.
> >
> > This is a re-posting of the patch series originally posted last month:
> >
> > http://marc.info/?l=linux-s390&m=129285043619973&w=2
> >
> > Heinz
> >
> > Changes in
> >
> > v2:
> > - kernel module hwsampler removed, everything is now in oprofile kernel module
> > - functions from hwsampler-main.c and smpctl.c merged into arch/s390/oprofile/hwsampler.c
> > - functions made static
> > - arch/s390/include/asm/hwsampler.h moved to arch/s390/oprofile/hwsampler.h
> > - structs have now hws_ prefix
> > - config variables changed, HAVE_HWSAMPLER used only
> > - original patch 4 (handle_munmap.patch) removed
>
> thanks for you patches, I have applied them with modifications to:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/rric/oprofile.git s390
>
> For atomic compilation of single patches I changed patch order and the
> Makefile where necessary. The branch also includes my follow-on
> patches.
>
> I will be travelling the next weeks and wanted to have this series in
> linux-next now for later inclusion to .39. We would run out of time
> otherwise (my bad). Please review and test the changes I made and send
> me possibly updates.
>
> Though I applied the patches I want to have the following changes:
>
> * Merge init.c and hwsampler_file.c, two files are bloated here and
> hwsampler_file.c is a bad and too long naming.
>

OK, I've merged all hwsampler_files.c contents into init.c

> * Rework functions in cpu_buffer.c (log_sample,
> __oprofile_add_ext_sample, oprofile_add_ext_hw_sample, etc.). All
> the (static) functions can be merged to a single functino by
> implementing a struct that holds all current function arguments.
> Something like:
>
> static void __oprofile_add_ext_sample(struct *foobar fb);
> void oprofile_add_sample(struct pt_regs * const regs, unsigned long event)
> {
> struct foobar fb = { .regs = regs, .event = event };
> __oprofile_add_ext_sample(&fb);
> }

OK, I've done this

>
> The naming and description of oprofile_add_ext_hw_sample() is also
> not the best. As interface in include/linux/oprofile.h we could then
> merge oprofile_add_ext_sample() and oprofile_add_ext_hw_sample() to
> a single function.
>
> It would be nice if you could implement this.

sure, I will do this, however I'm not sure what you exactly mean.
Could you specify the interface in oprofile.h what you basically have in
mind?

>
> For all patches in the oprofile/s390 branch I also need the
> s390 maintainer's ack.
>
> Martin or Heiko, please ack.
>
> Thanks,
>
> -Robert
>

Subject: Re: [patch v2 0/3] OProfile support for System z's hardware sampling

On 15.02.11 11:59:29, Heinz Graalfs wrote:

> > * Merge init.c and hwsampler_file.c, two files are bloated here and
> > hwsampler_file.c is a bad and too long naming.
> >
>
> OK, I've merged all hwsampler_files.c contents into init.c
>
> > * Rework functions in cpu_buffer.c (log_sample,
> > __oprofile_add_ext_sample, oprofile_add_ext_hw_sample, etc.). All
> > the (static) functions can be merged to a single functino by
> > implementing a struct that holds all current function arguments.
> > Something like:
> >
> > static void __oprofile_add_ext_sample(struct *foobar fb);
> > void oprofile_add_sample(struct pt_regs * const regs, unsigned long event)
> > {
> > struct foobar fb = { .regs = regs, .event = event };
> > __oprofile_add_ext_sample(&fb);
> > }
>
> OK, I've done this
>
> >
> > The naming and description of oprofile_add_ext_hw_sample() is also
> > not the best. As interface in include/linux/oprofile.h we could then
> > merge oprofile_add_ext_sample() and oprofile_add_ext_hw_sample() to
> > a single function.
> >
> > It would be nice if you could implement this.
>
> sure, I will do this, however I'm not sure what you exactly mean.
> Could you specify the interface in oprofile.h what you basically have in
> mind?

I mean to replace oprofile_add_ext_sample() and
oprofile_add_ext_hw_sample() by a new one. The interface would be in
the form of:

struct foobar {
...
}

static void __oprofile_add_ext_sample(struct *foobar fb);

Hope this makes sense. The advantage would be that we don't need to
extend the functions argument list anymore, we simply extend the
struct.

Please send me delta patches to oprofile/s390.

-Robert

--
Advanced Micro Devices, Inc.
Operating System Research Center