David,
On Tue, Jun 05, 2007 at 06:34:56PM -0700, David Rientjes wrote:
>
> > > > +int pfm_get_task(struct pfm_context *ctx, pid_t pid, struct
> > > > task_struct **task)
> > > > +{
> > >
> > > This function could be marked static even though it's exported through
> > > perfmon.h in patch 13. It is unreferenced elsewhere.
> > >
> > No because it is used in another module on IA-64 (for compatibility with
> > older versions).
> >
>
> Is this ia64 patch the one you mentioned that you did not post to LKML
> because it was too large in patch 0? Is there any way you could break
> that patch up itself and post it for comments?
>
Yes, this is the patch. It would be hard to break up in pieces.
The reason it is big is because it has to remove the older IA-64-only
implementation which was all in a single file whose size
was bigger than 100kB. It is hard to break this, unless I explicitely
remove the 'remove old file' diff from the patch.
> > > Why can't this be done with just struct task_struct *task as the third
> > > formal and change the assignment later to task = p?
> > >
> > Because we need to carry the errno back: ESRCH or EPERM.
> >
>
> Your formal is "struct task_struct **task" yet the only actual to this
> function is the memory address of a pointer to a single struct task_struct
> (i.e. it's never passed an array of struct task_struct pointers, which
> "struct task_struct **task" is).
>
> And since you only ever use this has *task to get the pointer, you can
> change the formal to just be "struct task_struct *task" and then pass in a
> pointer to a single struct task_struct.
>
I must be missing something here. I am modifying the address of task * in
the function. This is my second return value.
int pfm_get_task(void **p)
{
*p = 0x1000;
return 0;
}
int main(void)
{
void *p;
p = 0x2000;
printf("p=%p\n", p);
pfm_get_task(&p);
printf("p=%p\n", p);
return 0;
}
I am not passing a pointer to an array of struct ask *, but merely the address
of a pointer
to struct task *.
> > > > +
> > > > +asmlinkage long sys_pfm_write_pmcs(int fd, struct pfarg_pmc __user
> > > > *ureq, int count)
> > > > +{
> > > > + struct pfm_context *ctx;
> > > > + struct file *filp;
> > > > + struct pfarg_pmc pmcs[PFM_PMC_STK_ARG];
> > > > + struct pfarg_pmc *req;
> > > > + void *fptr;
> > > > + unsigned long flags;
> > > > + size_t sz;
> > > > + int ret, fput_needed;
> > > > +
> > >
> > > Could this have a stack overflow on powerpc?
> > >
> > The PFM_PMC_STK_ARG is per-arch, so you could chose a very low value.
> > I think it is set to 4. pfarg_pmc s 48 bytes and pfarg_pmd is 176 bytes
> > regardless of LP64 vs. ILP32.
> >
>
> Stack overflows like that are annoying to track down and powerpc has the
> highest PFM_PMC_STK_ARG of the entire patchset.
>
The function using this is a system call, so it is not too deep in the call
stack
and then the perfmon function never go very deep.
>
> I'm looking forward to seeing the next patchset and I'll give it a
> thorough test run on x86_64. It'd probably be best to base that patchset
> off 2.6.22 when it's released.
>
Very good thanks. I still need to go through all your other comments.
--
-Stephane
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
