Re: [patch 20/24] perfmon: system calls interface

[stephane eranian]

Ingo,

On Wed, Nov 26, 2008 at 3:08 PM, Ingo Molnar <mingo@xxxxxxx> wrote:
>
> * eranian@xxxxxxxxxxxxxx <eranian@xxxxxxxxxxxxxx> wrote:
>
>> +/*
>> + * unlike the other perfmon system calls, this one returns a file descriptor
>> + * or a value < 0 in case of error, very much like open() or socket()
>> + */
>> +asmlinkage long sys_pfm_create(int flags, struct pfarg_sinfo __user *ureq)
>> +{
>> +     struct pfm_context *new_ctx;
>> +     struct pfarg_sinfo sif;
>> +     int ret;
>> +
>> +     PFM_DBG("flags=0x%x sif=%p", flags, ureq);
>> +
>> +     if (perfmon_disabled)
>> +             return -ENOSYS;
>
> uhm. So we have a dynamic 'we dont support perfmon' flag. Which is
> global and defined as:
>
>  +int perfmon_disabled;  /* >0 if perfmon is disabled */
>
This variable says, there is perfmon support in the kernel but
initialization of the
subsystem failed. Thus the subsystem is disabled.

> (sidenote: that should be __read_mostly)
>

>> +     ret = __pfm_create_context(flags, &sif, &new_ctx);
>
> where we get:
>
> +int __pfm_create_context(__u32 ctx_flags,
> +                        struct pfarg_sinfo *sif,
> +                        struct pfm_context **new_ctx)
> +{
> +       struct pfm_context *ctx;
> +       struct file *filp = NULL;
> +       int fd = 0, ret = -EINVAL;
> +
> +       if (!pfm_pmu_conf)
> +               return -ENOSYS;
> +
>
> _ANOTHER_ global dynamic flag to tell us that ... in essence 'we dont
> support perfmon'. Which flag is again:
>
That one says that we do not have support for that particular PMU model.
In the fully-featured version, there PMU description table is loaded
via a kernel
module. So you can be in a situation where the subsystem initialized properly
but no PMU description module is loaded or none exists for this processor.
That's what this second test is about. Note that if all PMU description tables
are compiled in, we can still get into a situation where none support the
host processor.

> +struct pfm_pmu_config  *pfm_pmu_conf;
>
> ... which should be __read_mostly at minimum.
>
> +EXPORT_SYMBOL(pfm_pmu_conf);
>
> and _MUST_ be exported as EXPORT_SYMBOL_GPL. If exported at all. Why
> are any symbols exported here? perfmom does core kernel system calls
> and is non-modular:
>
In the patchset, yes. I can drop that for now.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/