[v2,1/4] xen: sched: refector code around vcpu_deassign() in null scheduler
diff mbox series

Message ID 156412235104.2385.3911161728130674771.stgit@Palanthas
State New, archived
Headers show
Series
  • xen: sched: support vcpu hotplug/hotunplug in the 'null scheduler'
Related show

Commit Message

Dario Faggioli July 26, 2019, 6:25 a.m. UTC
vcpu_deassign() is called only once (in _vcpu_remove()).

Let's consolidate the two functions into one.

No functional change intended.

Signed-off-by: Dario Faggioli <dfaggioli@suse.com>
Acked-by: George Dunlap <george.dunlap@citrix.com>
---
 xen/common/sched_null.c |   76 ++++++++++++++++++++++-------------------------
 1 file changed, 35 insertions(+), 41 deletions(-)

Comments

Jan Beulich Aug. 5, 2019, 3:58 p.m. UTC | #1
On 26.07.2019 08:25, Dario Faggioli wrote:
> vcpu_deassign() is called only once (in _vcpu_remove()).
> 
> Let's consolidate the two functions into one.
> 
> No functional change intended.
> 
> Signed-off-by: Dario Faggioli <dfaggioli@suse.com>
> Acked-by: George Dunlap <george.dunlap@citrix.com>

I'm puzzled by commit a397471278, in two ways:

1) The commit is empty, presumably because I did apply the patch a few
days ago already.

2) The committer is recorded as "Patchew Importer <importer@patchew.org>".
Do we really want to hide the fact who has been committing a patch?
While it's mostly mechanical, there's still the human decision of "this
is ready to go in" involved, which I don't think a bot can reliably take
in all cases.

Jan
George Dunlap Aug. 5, 2019, 4:04 p.m. UTC | #2
On 8/5/19 4:58 PM, Jan Beulich wrote:
> On 26.07.2019 08:25, Dario Faggioli wrote:
>> vcpu_deassign() is called only once (in _vcpu_remove()).
>>
>> Let's consolidate the two functions into one.
>>
>> No functional change intended.
>>
>> Signed-off-by: Dario Faggioli <dfaggioli@suse.com>
>> Acked-by: George Dunlap <george.dunlap@citrix.com>
> 
> I'm puzzled by commit a397471278, in two ways:
> 
> 1) The commit is empty, presumably because I did apply the patch a few
> days ago already.
> 
> 2) The committer is recorded as "Patchew Importer <importer@patchew.org>".
> Do we really want to hide the fact who has been committing a patch?
> While it's mostly mechanical, there's still the human decision of "this
> is ready to go in" involved, which I don't think a bot can reliably take
> in all cases.

Both of these are mistakes, and due to the fact that I `git fetch`ed
patchew's commit rather than doing `git am` of the mbox provided by
patchew.  (And I used patchew's commit because somehow 4/4 didn't reach
my inbox.)

Re #1, I re-reviewed v2 from the commit itself; but then rebased before
pushing, and didn't notice that the commit ended up  being empty.

Re #2, I guess that means I shouldn't really be pushing from patchew's
commit anyway.

Either way, sorry about the mistake; I'll try to remember to avoid using
patchew's commit in the future.

 -George
Dario Faggioli Aug. 5, 2019, 5:30 p.m. UTC | #3
On Mon, 2019-08-05 at 17:04 +0100, George Dunlap wrote:
> On 8/5/19 4:58 PM, Jan Beulich wrote:
> > On 26.07.2019 08:25, Dario Faggioli wrote:
> > > 
> > 1) The commit is empty, presumably because I did apply the patch a
> > few
> > days ago already.
> > 
> > 2) The committer is recorded as "Patchew Importer <
> > importer@patchew.org>".
> > Do we really want to hide the fact who has been committing a patch?
> > While it's mostly mechanical, there's still the human decision of
> > "this
> > is ready to go in" involved, which I don't think a bot can reliably
> > take
> > in all cases.
> 
> Both of these are mistakes, and due to the fact that I `git fetch`ed
> patchew's commit rather than doing `git am` of the mbox provided by
> patchew.  (And I used patchew's commit because somehow 4/4 didn't
> reach
> my inbox.)
>
And this last part is apparently my fault, as your email address is
actually wrong, in the Cc line of that patch.

So I guess I had a part in this as well... Sorry about that! :-)

Regards
George Dunlap Aug. 6, 2019, 7:45 a.m. UTC | #4
> On Aug 5, 2019, at 6:30 PM, Dario Faggioli <dfaggioli@suse.com> wrote:
> 
> On Mon, 2019-08-05 at 17:04 +0100, George Dunlap wrote:
>> On 8/5/19 4:58 PM, Jan Beulich wrote:
>>> On 26.07.2019 08:25, Dario Faggioli wrote:
>>>> 
>>> 1) The commit is empty, presumably because I did apply the patch a
>>> few
>>> days ago already.
>>> 
>>> 2) The committer is recorded as "Patchew Importer <
>>> importer@patchew.org>".
>>> Do we really want to hide the fact who has been committing a patch?
>>> While it's mostly mechanical, there's still the human decision of
>>> "this
>>> is ready to go in" involved, which I don't think a bot can reliably
>>> take
>>> in all cases.
>> 
>> Both of these are mistakes, and due to the fact that I `git fetch`ed
>> patchew's commit rather than doing `git am` of the mbox provided by
>> patchew.  (And I used patchew's commit because somehow 4/4 didn't
>> reach
>> my inbox.)
>> 
> And this last part is apparently my fault, as your email address is
> actually wrong, in the Cc line of that patch.

That would explain it. :-)  But that’s by no means your “fault”: It’s very common for me to only be CC’d on a subset of patches in a series (e.g., because I’m the maintainer for some patches but not others), so “receiving a only a subset of patches” is something my workflow should handle effectively but didn’t.

 -George

Patch
diff mbox series

diff --git a/xen/common/sched_null.c b/xen/common/sched_null.c
index c02c1b9c1f..c47c1b5aae 100644
--- a/xen/common/sched_null.c
+++ b/xen/common/sched_null.c
@@ -358,9 +358,14 @@  static void vcpu_assign(struct null_private *prv, struct vcpu *v,
     }
 }
 
-static void vcpu_deassign(struct null_private *prv, struct vcpu *v,
-                          unsigned int cpu)
+static void vcpu_deassign(struct null_private *prv, struct vcpu *v)
 {
+    unsigned int bs;
+    unsigned int cpu = v->processor;
+    struct null_vcpu *wvc;
+
+    ASSERT(list_empty(&null_vcpu(v)->waitq_elem));
+
     per_cpu(npc, cpu).vcpu = NULL;
     cpumask_set_cpu(cpu, &prv->cpus_free);
 
@@ -377,6 +382,32 @@  static void vcpu_deassign(struct null_private *prv, struct vcpu *v,
         d.cpu = cpu;
         __trace_var(TRC_SNULL_VCPU_DEASSIGN, 1, sizeof(d), &d);
     }
+
+    spin_lock(&prv->waitq_lock);
+
+    /*
+     * If v is assigned to a pCPU, let's see if there is someone waiting,
+     * suitable to be assigned to it (prioritizing vcpus that have
+     * soft-affinity with cpu).
+     */
+    for_each_affinity_balance_step( bs )
+    {
+        list_for_each_entry( wvc, &prv->waitq, waitq_elem )
+        {
+            if ( bs == BALANCE_SOFT_AFFINITY && !has_soft_affinity(wvc->vcpu) )
+                continue;
+
+            if ( vcpu_check_affinity(wvc->vcpu, cpu, bs) )
+            {
+                list_del_init(&wvc->waitq_elem);
+                vcpu_assign(prv, wvc->vcpu, cpu);
+                cpu_raise_softirq(cpu, SCHEDULE_SOFTIRQ);
+                spin_unlock(&prv->waitq_lock);
+                return;
+            }
+        }
+    }
+    spin_unlock(&prv->waitq_lock);
 }
 
 /* Change the scheduler of cpu to us (null). */
@@ -459,43 +490,6 @@  static void null_vcpu_insert(const struct scheduler *ops, struct vcpu *v)
     SCHED_STAT_CRANK(vcpu_insert);
 }
 
-static void _vcpu_remove(struct null_private *prv, struct vcpu *v)
-{
-    unsigned int bs;
-    unsigned int cpu = v->processor;
-    struct null_vcpu *wvc;
-
-    ASSERT(list_empty(&null_vcpu(v)->waitq_elem));
-
-    vcpu_deassign(prv, v, cpu);
-
-    spin_lock(&prv->waitq_lock);
-
-    /*
-     * If v is assigned to a pCPU, let's see if there is someone waiting,
-     * suitable to be assigned to it (prioritizing vcpus that have
-     * soft-affinity with cpu).
-     */
-    for_each_affinity_balance_step( bs )
-    {
-        list_for_each_entry( wvc, &prv->waitq, waitq_elem )
-        {
-            if ( bs == BALANCE_SOFT_AFFINITY && !has_soft_affinity(wvc->vcpu) )
-                continue;
-
-            if ( vcpu_check_affinity(wvc->vcpu, cpu, bs) )
-            {
-                list_del_init(&wvc->waitq_elem);
-                vcpu_assign(prv, wvc->vcpu, cpu);
-                cpu_raise_softirq(cpu, SCHEDULE_SOFTIRQ);
-                spin_unlock(&prv->waitq_lock);
-                return;
-            }
-        }
-    }
-    spin_unlock(&prv->waitq_lock);
-}
-
 static void null_vcpu_remove(const struct scheduler *ops, struct vcpu *v)
 {
     struct null_private *prv = null_priv(ops);
@@ -519,7 +513,7 @@  static void null_vcpu_remove(const struct scheduler *ops, struct vcpu *v)
     ASSERT(per_cpu(npc, v->processor).vcpu == v);
     ASSERT(!cpumask_test_cpu(v->processor, &prv->cpus_free));
 
-    _vcpu_remove(prv, v);
+    vcpu_deassign(prv, v);
 
  out:
     vcpu_schedule_unlock_irq(lock, v);
@@ -605,7 +599,7 @@  static void null_vcpu_migrate(const struct scheduler *ops, struct vcpu *v,
      */
     if ( likely(list_empty(&nvc->waitq_elem)) )
     {
-        _vcpu_remove(prv, v);
+        vcpu_deassign(prv, v);
         SCHED_STAT_CRANK(migrate_running);
     }
     else