RE: [patch 13/18] x86, ds: add leakage warning

Metzger, Markus T
2009-04-03T06:42:26Z

>-----Original Message-----
>From: Ingo Molnar [mailto:mingo@elte.hu]
>Sent: Thursday, April 02, 2009 9:28 PM
>To: Metzger, Markus T
>Cc: tglx@linutronix.de; hpa@zytor.com; markus.t.metzger@gmail.com; roland@redhat.com;
>eranian@googlemail.com; oleg@redhat.com; Villacis, Juan; ak@linux.jf.intel.com; linux-
>kernel@vger.kernel.org
>Subject: Re: [patch 13/18] x86, ds: add leakage warning
>
>
>* markus.t.metzger@intel.com <markus.t.metzger@intel.com> wrote:
>
>> +static inline void ds_warn_leak(struct task_struct *tsk)
>> +{
>> + WARN(tsk->thread.ds_ctx, "leaking DS context\n");
>> +}
>
>> + ds_warn_leak(tsk);
>
>since we normally expect X86_DS enabled i'd suggest this
>simplification: always have thread.ds_ctx around so the WARN_ON can
>go there unconditionally. (it wont trigger in the !DS case)

OK.

>> struct task_struct;
>> struct mm_struct;
>> +struct ds_context;
>
>why is this needed in processor.h?

This seems to be common practice.

The only thing we need here is a forward declaration in order to put a pointer
field into the thread_struct.

The same holds for sched.h, which uses bts_context. Since bts_context again has a pointer
to a task_struct, we would have circular dependencies, so either one needs a forward
declaration.

If I split ds.h into ds_types.h and ds_api.h, the only place where this could
be used would be processor.h. All the rest needs to include both - or, rather,
ds_api.h, which then includes ds_types.h.

>ds.h should be split into ds_types.h and ds_api.h, with the pure
>data type definitions in ds_types.h. processor.h (which needs to
>know about the DS context type) then includes ds_types.h - which
>is a lightweight header.
>
>Actual DS using facilities also disable ds_api.h.
>
>This keeps data types nicely separated from methods and inlines.
>
> Ingo