| Message ID | 20191108073837.5797-1-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/sched: remove wrong assertions in csched2_free_pdata() | expand |
> On Nov 8, 2019, at 7:38 AM, Juergen Gross <JGross@suse.com> wrote: > > The assertions in csched2_free_pdata() are wrong as in case it is > called by schedule_cpu_add() after a failure of sched_alloc_udata() > the init pdata function won't have been called. I’m a bit confused by this, as the comment says that the ASSERT()s should be OK with that case; i.e., that they should check *either* that pdata hasn’t been called, or that dinit_pdata() has been called: > - * xfree() does not really mind, but we want to be sure that either > - * init_pdata has never been called, or deinit_pdata has been called > - * already. So which of the following conditions will fail if sched_alloc_udata() fails? It looks to me like they should both be fine. > - ASSERT(!pcpu || spc->runq_id == -1); > - ASSERT(!cpumask_test_cpu(cpu, &csched2_priv(ops)->initialized)); -George
On 12.11.19 16:52, George Dunlap wrote: > > >> On Nov 8, 2019, at 7:38 AM, Juergen Gross <JGross@suse.com> wrote: >> >> The assertions in csched2_free_pdata() are wrong as in case it is >> called by schedule_cpu_add() after a failure of sched_alloc_udata() >> the init pdata function won't have been called. > > I’m a bit confused by this, as the comment says that the ASSERT()s should be OK with that case; i.e., that they should check *either* that pdata hasn’t been called, or that dinit_pdata() has been called: > >> - * xfree() does not really mind, but we want to be sure that either >> - * init_pdata has never been called, or deinit_pdata has been called >> - * already. > > So which of the following conditions will fail if sched_alloc_udata() fails? It looks to me like they should both be fine. You are right, this patch is not needed. Sorry for the noise, Juergen
On Fri, 2019-11-08 at 08:38 +0100, Juergen Gross wrote: > The assertions in csched2_free_pdata() are wrong as in case it is > called by schedule_cpu_add() after a failure of sched_alloc_udata() > the init pdata function won't have been called. > Sorry, maybe too much time has passed since when I wrote this code, and I'm rusty, but the comment says: "we want to be sure that either init_pdata has never been called, or deinit_pdata has been called already" So, the case of init_pdata not having been called is considered. And yet, you are saying it is wrong because: "in case it is called [..] after a failure of sched_alloc_udata() the init pdata function won't have been called" But, as just said, init_pdata not having been called was one of the possibilities... wasn't it? Or am I misunderstanding the meaning of the sentence above? Don't get me wrong, I never particularly loved these ASSERT()s and I'd be more than fine seeing them go... :-) Have you seen them triggering inappropriately, either before or after the core-scheduling series (and either with core-scheduling on or off)? Regards (leaving the patch in context on purpose, in case it's useful) > --- > xen/common/sched_credit2.c | 16 ---------------- > 1 file changed, 16 deletions(-) > > diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c > index af58ee161d..a995ff838f 100644 > --- a/xen/common/sched_credit2.c > +++ b/xen/common/sched_credit2.c > @@ -3914,10 +3914,6 @@ csched2_deinit_pdata(const struct scheduler > *ops, void *pcpu, int cpu) > > write_lock_irqsave(&prv->lock, flags); > > - /* > - * alloc_pdata is not implemented, so pcpu must be NULL. On the > other > - * hand, init_pdata must have been called for this pCPU. > - */ > /* > * Scheduler specific data for this pCPU must still be there and > and be > * valid. In fact, if we are here: > @@ -3969,18 +3965,6 @@ csched2_deinit_pdata(const struct scheduler > *ops, void *pcpu, int cpu) > static void > csched2_free_pdata(const struct scheduler *ops, void *pcpu, int cpu) > { > - struct csched2_pcpu *spc = pcpu; > - > - /* > - * pcpu either points to a valid struct csched2_pcpu, or is NULL > (if > - * CPU bringup failed, and we're beeing called from > CPU_UP_CANCELLED). > - * xfree() does not really mind, but we want to be sure that > either > - * init_pdata has never been called, or deinit_pdata has been > called > - * already. > - */ > - ASSERT(!pcpu || spc->runq_id == -1); > - ASSERT(!cpumask_test_cpu(cpu, &csched2_priv(ops)->initialized)); > - > xfree(pcpu); > } >
diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c index af58ee161d..a995ff838f 100644 --- a/xen/common/sched_credit2.c +++ b/xen/common/sched_credit2.c @@ -3914,10 +3914,6 @@ csched2_deinit_pdata(const struct scheduler *ops, void *pcpu, int cpu) write_lock_irqsave(&prv->lock, flags); - /* - * alloc_pdata is not implemented, so pcpu must be NULL. On the other - * hand, init_pdata must have been called for this pCPU. - */ /* * Scheduler specific data for this pCPU must still be there and and be * valid. In fact, if we are here: @@ -3969,18 +3965,6 @@ csched2_deinit_pdata(const struct scheduler *ops, void *pcpu, int cpu) static void csched2_free_pdata(const struct scheduler *ops, void *pcpu, int cpu) { - struct csched2_pcpu *spc = pcpu; - - /* - * pcpu either points to a valid struct csched2_pcpu, or is NULL (if - * CPU bringup failed, and we're beeing called from CPU_UP_CANCELLED). - * xfree() does not really mind, but we want to be sure that either - * init_pdata has never been called, or deinit_pdata has been called - * already. - */ - ASSERT(!pcpu || spc->runq_id == -1); - ASSERT(!cpumask_test_cpu(cpu, &csched2_priv(ops)->initialized)); - xfree(pcpu); }
The assertions in csched2_free_pdata() are wrong as in case it is called by schedule_cpu_add() after a failure of sched_alloc_udata() the init pdata function won't have been called. So just remove the (wrong) comment and ASSERT() statements. While at it remove the wrong comment in csched2_deinit_pdata(), too. Signed-off-by: Juergen Gross <jgross@suse.com> --- xen/common/sched_credit2.c | 16 ---------------- 1 file changed, 16 deletions(-)