xen/sched: remove wrong assertions in csched2_free_pdata()

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

Posted by Juergen Gross 4 years, 5 months ago

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(-)

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);
 }
 
-- 
2.16.4


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

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

Posted by George Dunlap 4 years, 5 months ago

> 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

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

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

Posted by Jürgen Groß 4 years, 5 months ago

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

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

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

Posted by Dario Faggioli 4 years, 5 months ago

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)

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