From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] introduce and used relaxed cpumask operations Date: Wed, 21 Jan 2015 12:21:37 +0000 Message-ID: <54BF99D1.9050709@citrix.com> References: <54BD379C0200007800056965@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============6767245141047122787==" Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1YDuI4-0007zn-Tw for xen-devel@lists.xenproject.org; Wed, 21 Jan 2015 12:21:45 +0000 In-Reply-To: <54BD379C0200007800056965@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich , xen-devel Cc: George Dunlap , Ian Jackson , Keir Fraser , Ian Campbell , Tim Deegan List-Id: xen-devel@lists.xenproject.org --===============6767245141047122787== Content-Type: multipart/alternative; boundary="------------060506060809080303070107" --------------060506060809080303070107 Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: quoted-printable On 19/01/15 15:58, Jan Beulich wrote: > --- a/xen/common/core_parking.c > +++ b/xen/common/core_parking.c > @@ -75,11 +75,10 @@ static unsigned int core_parking_perform > if ( core_weight < core_tmp ) > { > core_weight =3D core_tmp; > - cpumask_clear(&core_candidate_map); > - cpumask_set_cpu(cpu, &core_candidate_map); > + cpumask_copy(&core_candidate_map, cpumask_of(cpu)); It is probably worth mentioning changes like this in the commit message, as they are slightly more than just a simple removal of the lock prefix. > } > else if ( core_weight =3D=3D core_tmp ) > - cpumask_set_cpu(cpu, &core_candidate_map); > + __cpumask_set_cpu(cpu, &core_candidate_map); > } > =20 > for_each_cpu(cpu, &core_candidate_map) > --- a/xen/common/sched_rt.c > +++ b/xen/common/sched_rt.c > @@ -663,7 +663,7 @@ burn_budget(const struct scheduler *ops, > * lock is grabbed before calling this function > */ > static struct rt_vcpu * > -__runq_pick(const struct scheduler *ops, cpumask_t *mask) > +__runq_pick(const struct scheduler *ops, const cpumask_t *mask) > { > struct list_head *runq =3D rt_runq(ops); > struct list_head *iter; > @@ -780,10 +780,7 @@ rt_schedule(const struct scheduler *ops, > } > else > { > - cpumask_t cur_cpu; > - cpumask_clear(&cur_cpu); > - cpumask_set_cpu(cpu, &cur_cpu); > - snext =3D __runq_pick(ops, &cur_cpu); > + snext =3D __runq_pick(ops, cpumask_of(cpu)); > if ( snext =3D=3D NULL ) > snext =3D rt_vcpu(idle_vcpu[cpu]); > =20 > --- a/xen/common/softirq.c > +++ b/xen/common/softirq.c > @@ -88,7 +88,7 @@ void cpumask_raise_softirq(const cpumask > if ( !test_and_set_bit(nr, &softirq_pending(cpu)) && > cpu !=3D this_cpu && > !arch_skip_send_event_check(cpu) ) > - cpumask_set_cpu(cpu, raise_mask); > + __cpumask_set_cpu(cpu, raise_mask); > =20 > if ( raise_mask =3D=3D &send_mask ) > smp_send_event_check_mask(raise_mask); > @@ -106,7 +106,7 @@ void cpu_raise_softirq(unsigned int cpu, > if ( !per_cpu(batching, this_cpu) || in_irq() ) > smp_send_event_check_cpu(cpu); > else > - set_bit(nr, &per_cpu(batch_mask, this_cpu)); > + __cpumask_set_cpu(nr, &per_cpu(batch_mask, this_cpu)); > } > =20 > void cpu_raise_softirq_batch_begin(void) > @@ -122,7 +122,7 @@ void cpu_raise_softirq_batch_finish(void > ASSERT(per_cpu(batching, this_cpu)); > for_each_cpu ( cpu, mask ) > if ( !softirq_pending(cpu) ) > - cpumask_clear_cpu(cpu, mask); > + __cpumask_clr_cpu(cpu, mask); > smp_send_event_check_mask(mask); > cpumask_clear(mask); > --per_cpu(batching, this_cpu); > --- a/xen/include/xen/cpumask.h > +++ b/xen/include/xen/cpumask.h > @@ -103,11 +103,21 @@ static inline void cpumask_set_cpu(int c > set_bit(cpumask_check(cpu), dstp->bits); > } > =20 > +static inline void __cpumask_set_cpu(int cpu, cpumask_t *dstp) > +{ > + __set_bit(cpumask_check(cpu), dstp->bits); > +} > + > static inline void cpumask_clear_cpu(int cpu, volatile cpumask_t *dstp= ) > { > clear_bit(cpumask_check(cpu), dstp->bits); > } > =20 > +static inline void __cpumask_clr_cpu(int cpu, cpumask_t *dstp) > +{ > + __clear_bit(cpumask_check(cpu), dstp->bits); > +} > + While I can appreciate the want for a shorter function name, I feel that consistency with its locked alternative is more important. > static inline void cpumask_setall(cpumask_t *dstp) > { > bitmap_fill(dstp->bits, nr_cpumask_bits); > @@ -122,16 +132,26 @@ static inline void cpumask_clear(cpumask > #define cpumask_test_cpu(cpu, cpumask) \ > test_bit(cpumask_check(cpu), (cpumask)->bits) > =20 > -static inline int cpumask_test_and_set_cpu(int cpu, cpumask_t *addr) > +static inline int cpumask_test_and_set_cpu(int cpu, volatile cpumask_t= *addr) > { > return test_and_set_bit(cpumask_check(cpu), addr->bits); > } > =20 > -static inline int cpumask_test_and_clear_cpu(int cpu, cpumask_t *addr)= > +static inline int __cpumask_tst_set_cpu(int cpu, cpumask_t *addr) > +{ > + return __test_and_set_bit(cpumask_check(cpu), addr->bits); > +} > + > +static inline int cpumask_test_and_clear_cpu(int cpu, volatile cpumask= _t *addr) This introduction of volatile also need mentioning in the commit message, but I would agree that it should be here. ~Andrew > { > return test_and_clear_bit(cpumask_check(cpu), addr->bits); > } > =20 > +static inline int __cpumask_tst_clr_cpu(int cpu, cpumask_t *addr) > +{ > + return __test_and_clear_bit(cpumask_check(cpu), addr->bits); > +} > + > static inline void cpumask_and(cpumask_t *dstp, const cpumask_t *src1p= , > const cpumask_t *src2p) > { > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel --------------060506060809080303070107 Content-Type: text/html; charset="windows-1252" Content-Transfer-Encoding: 8bit
On 19/01/15 15:58, Jan Beulich wrote:
--- a/xen/common/core_parking.c
+++ b/xen/common/core_parking.c
@@ -75,11 +75,10 @@ static unsigned int core_parking_perform
             if ( core_weight < core_tmp )
             {
                 core_weight = core_tmp;
-                cpumask_clear(&core_candidate_map);
-                cpumask_set_cpu(cpu, &core_candidate_map);
+                cpumask_copy(&core_candidate_map, cpumask_of(cpu));

It is probably worth mentioning changes like this in the commit message, as they are slightly more than just a simple removal of the lock prefix.

             }
             else if ( core_weight == core_tmp )
-                cpumask_set_cpu(cpu, &core_candidate_map);
+                __cpumask_set_cpu(cpu, &core_candidate_map);
         }
 
         for_each_cpu(cpu, &core_candidate_map)
--- a/xen/common/sched_rt.c
+++ b/xen/common/sched_rt.c
@@ -663,7 +663,7 @@ burn_budget(const struct scheduler *ops,
  * lock is grabbed before calling this function
  */
 static struct rt_vcpu *
-__runq_pick(const struct scheduler *ops, cpumask_t *mask)
+__runq_pick(const struct scheduler *ops, const cpumask_t *mask)
 {
     struct list_head *runq = rt_runq(ops);
     struct list_head *iter;
@@ -780,10 +780,7 @@ rt_schedule(const struct scheduler *ops,
     }
     else
     {
-        cpumask_t cur_cpu;
-        cpumask_clear(&cur_cpu);
-        cpumask_set_cpu(cpu, &cur_cpu);
-        snext = __runq_pick(ops, &cur_cpu);
+        snext = __runq_pick(ops, cpumask_of(cpu));
         if ( snext == NULL )
             snext = rt_vcpu(idle_vcpu[cpu]);
 
--- a/xen/common/softirq.c
+++ b/xen/common/softirq.c
@@ -88,7 +88,7 @@ void cpumask_raise_softirq(const cpumask
         if ( !test_and_set_bit(nr, &softirq_pending(cpu)) &&
              cpu != this_cpu &&
              !arch_skip_send_event_check(cpu) )
-            cpumask_set_cpu(cpu, raise_mask);
+            __cpumask_set_cpu(cpu, raise_mask);
 
     if ( raise_mask == &send_mask )
         smp_send_event_check_mask(raise_mask);
@@ -106,7 +106,7 @@ void cpu_raise_softirq(unsigned int cpu,
     if ( !per_cpu(batching, this_cpu) || in_irq() )
         smp_send_event_check_cpu(cpu);
     else
-        set_bit(nr, &per_cpu(batch_mask, this_cpu));
+        __cpumask_set_cpu(nr, &per_cpu(batch_mask, this_cpu));
 }
 
 void cpu_raise_softirq_batch_begin(void)
@@ -122,7 +122,7 @@ void cpu_raise_softirq_batch_finish(void
     ASSERT(per_cpu(batching, this_cpu));
     for_each_cpu ( cpu, mask )
         if ( !softirq_pending(cpu) )
-            cpumask_clear_cpu(cpu, mask);
+            __cpumask_clr_cpu(cpu, mask);
     smp_send_event_check_mask(mask);
     cpumask_clear(mask);
     --per_cpu(batching, this_cpu);
--- a/xen/include/xen/cpumask.h
+++ b/xen/include/xen/cpumask.h
@@ -103,11 +103,21 @@ static inline void cpumask_set_cpu(int c
 	set_bit(cpumask_check(cpu), dstp->bits);
 }
 
+static inline void __cpumask_set_cpu(int cpu, cpumask_t *dstp)
+{
+	__set_bit(cpumask_check(cpu), dstp->bits);
+}
+
 static inline void cpumask_clear_cpu(int cpu, volatile cpumask_t *dstp)
 {
 	clear_bit(cpumask_check(cpu), dstp->bits);
 }
 
+static inline void __cpumask_clr_cpu(int cpu, cpumask_t *dstp)
+{
+	__clear_bit(cpumask_check(cpu), dstp->bits);
+}
+

While I can appreciate the want for a shorter function name, I feel that consistency with its locked alternative is more important.

 static inline void cpumask_setall(cpumask_t *dstp)
 {
 	bitmap_fill(dstp->bits, nr_cpumask_bits);
@@ -122,16 +132,26 @@ static inline void cpumask_clear(cpumask
 #define cpumask_test_cpu(cpu, cpumask) \
 	test_bit(cpumask_check(cpu), (cpumask)->bits)
 
-static inline int cpumask_test_and_set_cpu(int cpu, cpumask_t *addr)
+static inline int cpumask_test_and_set_cpu(int cpu, volatile cpumask_t *addr)
 {
 	return test_and_set_bit(cpumask_check(cpu), addr->bits);
 }
 
-static inline int cpumask_test_and_clear_cpu(int cpu, cpumask_t *addr)
+static inline int __cpumask_tst_set_cpu(int cpu, cpumask_t *addr)
+{
+	return __test_and_set_bit(cpumask_check(cpu), addr->bits);
+}
+
+static inline int cpumask_test_and_clear_cpu(int cpu, volatile cpumask_t *addr)

This introduction of volatile also need mentioning in the commit message, but I would agree that it should be here.

~Andrew

 {
 	return test_and_clear_bit(cpumask_check(cpu), addr->bits);
 }
 
+static inline int __cpumask_tst_clr_cpu(int cpu, cpumask_t *addr)
+{
+	return __test_and_clear_bit(cpumask_check(cpu), addr->bits);
+}
+
 static inline void cpumask_and(cpumask_t *dstp, const cpumask_t *src1p,
 			       const cpumask_t *src2p)
 {





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

--------------060506060809080303070107-- --===============6767245141047122787== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============6767245141047122787==--