Re: [Xen-devel] [PATCH RFC V2 01/45] xen/sched: add inline wrappers for calling per-scheduler functions

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.05.19 at 12:56, <jgross@xxxxxxxx> wrote:
> On 09/05/2019 12:04, George Dunlap wrote:
>> On 5/9/19 6:32 AM, Juergen Gross wrote:
>>> On 08/05/2019 18:24, George Dunlap wrote:
>>>> On 5/6/19 7:56 AM, Juergen Gross wrote:
>>>>> Instead of using the SCHED_OP() macro to call the different scheduler
>>>>> specific functions add inline wrappers for that purpose.
>>>>>
>>>>> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
>>>>
>>>> This seems like a great idea.  One minor comment...
>>>>
>>>>> +static inline int sched_init(struct scheduler *s)
>>>>> +{
>>>>> +    ASSERT(s->init);
>>>>> +    return s->init(s);
>>>>> +}
>>>>> +
>>>>> +static inline void sched_deinit(struct scheduler *s)
>>>>> +{
>>>>> +    ASSERT(s->deinit);
>>>>> +    s->deinit(s);
>>>>> +}
>>>>
>>>> I think these would better as BUG_ON()s.  These aren't hot paths, and if
>>>> we do somehow hit this situation in production, 1) it's safer to
>>>> BUG_ON() than dereferencing NULL, and 2) you'll get a more helpful error
>>>> message.
>>>
>>> Only for those 2 instances above? Or would you like BUG_ON() instead of
>>> ASSERT() in the other added inlines, too (maybe not for pick_cpu, but
>>> e.g. the ones in free_*) ?
>> 
>> Why not for pick_cpu()?  It's the same basic logic -- in production, if
>> it *is* NULL, then you'll either crash with a segfault, or start
>> executing an exploit.  Much better to BUG_ON().
> 
> pick_cpu is called rather often, so maybe we should avoid additional
> tests.
> 
> Hmm, thinking more about it: why don't we just drop those ASSERT/BUG_ON
> for mandatory functions and test them when doing the global_init() loop
> over all schedulers. We could just reject schedulers with missing
> functions.

This would imply pointers can't be zapped off the structures.
IMO this would require, as minimal (language level) protection,
that all instances of struct scheduler be const, which doesn't
look doable without some further rework

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel