[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH RFC V2 01/45] xen/sched: add inline wrappers for calling per-scheduler functions
>>> On 09.05.19 at 14:03, <jgross@xxxxxxxx> wrote: > On 09/05/2019 13:50, Jan Beulich wrote: >>>>> 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 > > They are const already. > > The default scheduler's struct is copied to a non-const struct scheduler > in scheduler_init(). Exactly, and then we have things like static int rt_init(struct scheduler *ops) { ... ops->sched_data = prv; I.e. it would be quite easy for a specific scheduler to zap one or more of its pointers. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |