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

[Juergen Gross <jgross@suse.com>]

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@suse.com>
>>>
>>> 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.

> As for the `ASSERT(!data)`, instances, I think it's the reverse: If
> `data` turns out to be non-null, then you'll leak memory, but otherwise
> keep working until you run out.  If you make those BUG_ON()s, then
> everything stops immediately.  I think ASSERT() is the right thing in
> those cases.

Yes.


Juergen

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