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);
> }
>
--
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
