Re: [Xen-devel] [PATCH] xen/sched: remove wrong assertions in csched2_free_pdata()

[Dario Faggioli <dfaggioli@xxxxxxxx>]

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)

Attachment: signature.asc
Description: This is a digitally signed message part

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