* [Xen-devel] [PATCH] xen/sched: remove wrong assertions in csched2_free_pdata()
@ 2019-11-08 7:38 Juergen Gross
2019-11-12 15:52 ` George Dunlap
2019-11-12 18:45 ` Dario Faggioli
0 siblings, 2 replies; 4+ messages in thread
From: Juergen Gross @ 2019-11-08 7:38 UTC (permalink / raw)
To: xen-devel; +Cc: Juergen Gross, George Dunlap, Dario Faggioli
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Xen-devel] [PATCH] xen/sched: remove wrong assertions in csched2_free_pdata()
2019-11-08 7:38 [Xen-devel] [PATCH] xen/sched: remove wrong assertions in csched2_free_pdata() Juergen Gross
@ 2019-11-12 15:52 ` George Dunlap
2019-11-12 16:10 ` Jürgen Groß
2019-11-12 18:45 ` Dario Faggioli
1 sibling, 1 reply; 4+ messages in thread
From: George Dunlap @ 2019-11-12 15:52 UTC (permalink / raw)
To: Juergen Gross; +Cc: Xen-devel, George Dunlap, Dario Faggioli
> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Xen-devel] [PATCH] xen/sched: remove wrong assertions in csched2_free_pdata()
2019-11-12 15:52 ` George Dunlap
@ 2019-11-12 16:10 ` Jürgen Groß
0 siblings, 0 replies; 4+ messages in thread
From: Jürgen Groß @ 2019-11-12 16:10 UTC (permalink / raw)
To: George Dunlap; +Cc: Xen-devel, Dario Faggioli
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Xen-devel] [PATCH] xen/sched: remove wrong assertions in csched2_free_pdata()
2019-11-08 7:38 [Xen-devel] [PATCH] xen/sched: remove wrong assertions in csched2_free_pdata() Juergen Gross
2019-11-12 15:52 ` George Dunlap
@ 2019-11-12 18:45 ` Dario Faggioli
1 sibling, 0 replies; 4+ messages in thread
From: Dario Faggioli @ 2019-11-12 18:45 UTC (permalink / raw)
To: Juergen Gross, xen-devel; +Cc: george.dunlap
[-- Attachment #1.1: Type: text/plain, Size: 2993 bytes --]
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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-11-12 18:46 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-08 7:38 [Xen-devel] [PATCH] xen/sched: remove wrong assertions in csched2_free_pdata() Juergen Gross
2019-11-12 15:52 ` George Dunlap
2019-11-12 16:10 ` Jürgen Groß
2019-11-12 18:45 ` Dario Faggioli
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).