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 <jgr...@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
