[PATCH v2 0/5] xen: Use %*pb[l] for printing bitmaps

[PATCH v2 0/5] xen: Use %*pb[l] for printing bitmaps

[Andrew Cooper]

I was trying to debug a cpumask problem, and got irritated with how awkard it
was to render and print the masks.  Luckily, the fix is quite simple and far
nicer to use.

The overall diffstat is to patch 4 is:

  add/remove: 0/4 grow/shrink: 2/11 up/down: 603/-1191 (-588)
  Function                                     old     new   delta
  vsnprintf                                   2775    3348    +573
  dump_runq                                    279     309     +30
  null_dump                                    675     659     -16
  dump_irqs                                    718     702     -16
  machine_crash_shutdown                       594     570     -24
  csched_dump                                  527     503     -24
  rt_dump_vcpu.isra                            254     222     -32
  dump_evtchn_info                             657     625     -32
  print_cpumap                                  55       -     -55
  cpuset_print.constprop                        61       -     -61
  csched_dump_pcpu                             473     401     -72
  mcheck_cmn_handler                          1224    1146     -78
  null_dump_pcpu                               493     413     -80
  dump_domains                                1160    1064     -96
  csched2_dump                                1549    1389    -160
  bitmap_scnprintf                             193       -    -193
  bitmap_scnlistprintf                         252       -    -252
  Total: Before=3295448, After=3294860, chg -0.02%

Only minor changes between this and v1.  See individual patches for details.

Andrew Cooper (5):
  xen/sched: Use %*pb[l] instead of cpumask_scn{,list}printf()
  xen/common: Use %*pb[l] instead of {cpu,node}mask_scn{,list}printf()
  xen/x86: Use %*pb[l] instead of cpumask_scn{,list}printf()
  xen/bitmap: Drop all bitmap_scn{,list}printf() infrastructure
  xen/keyhandler: Drop keyhandler_scratch

 xen/arch/x86/cpu/mcheck/mce.c |   9 ++--
 xen/arch/x86/crash.c          |   7 +--
 xen/arch/x86/irq.c            |   7 +--
 xen/arch/x86/numa.c           |  11 ++---
 xen/common/bitmap.c           | 105 ------------------------------------------
 xen/common/cpupool.c          |  12 ++---
 xen/common/efi/boot.c         |   4 +-
 xen/common/event_channel.c    |   6 +--
 xen/common/keyhandler.c       |  61 +++++++-----------------
 xen/common/sched_credit.c     |  17 ++-----
 xen/common/sched_credit2.c    |  27 ++++-------
 xen/common/sched_null.c       |  15 ++----
 xen/common/sched_rt.c         |   5 +-
 xen/include/xen/bitmap.h      |   6 ---
 xen/include/xen/cpumask.h     |  18 --------
 xen/include/xen/keyhandler.h  |   3 --
 xen/include/xen/nodemask.h    |  34 --------------
 17 files changed, 59 insertions(+), 288 deletions(-)

-- 
2.1.4


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

[PATCH v2 1/5] xen/sched: Use %*pb[l] instead of cpumask_scn{, list}printf()

[Andrew Cooper]

This removes all use of keyhandler_scratch as a bounce-buffer for the rendered
string.  In some cases, collapse combine adjacent printk()'s which are writing
parts of the same line.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: George Dunlap <george.dunlap@citrix.com>
Acked-by: Dario Faggioli <dfaggioli@suse.com>
---
CC: Josh Whitehead <josh.whitehead@dornerworks.com>
CC: Robert VanVossen <robert.vanvossen@dornerworks.com>
CC: Meng Xu <mengxu@cis.upenn.edu>

v2:
 * Use ->bits for cpumasks
---
 xen/common/sched_credit.c  | 17 +++++------------
 xen/common/sched_credit2.c | 27 ++++++++++-----------------
 xen/common/sched_null.c    | 15 +++++----------
 xen/common/sched_rt.c      |  5 ++---
 4 files changed, 22 insertions(+), 42 deletions(-)

diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
index 84e744b..4560ab6 100644
--- a/xen/common/sched_credit.c
+++ b/xen/common/sched_credit.c
@@ -2044,7 +2044,6 @@ csched_dump_pcpu(const struct scheduler *ops, int cpu)
     spinlock_t *lock;
     unsigned long flags;
     int loop;
-#define cpustr keyhandler_scratch
 
     /*
      * We need both locks:
@@ -2059,11 +2058,10 @@ csched_dump_pcpu(const struct scheduler *ops, int cpu)
     spc = CSCHED_PCPU(cpu);
     runq = &spc->runq;
 
-    cpumask_scnprintf(cpustr, sizeof(cpustr), per_cpu(cpu_sibling_mask, cpu));
-    printk("CPU[%02d] nr_run=%d, sort=%d, sibling=%s, ",
-           cpu, spc->nr_runnable, spc->runq_sort_last, cpustr);
-    cpumask_scnprintf(cpustr, sizeof(cpustr), per_cpu(cpu_core_mask, cpu));
-    printk("core=%s\n", cpustr);
+    printk("CPU[%02d] nr_run=%d, sort=%d, sibling=%*pb, core=%*pb\n",
+           cpu, spc->nr_runnable, spc->runq_sort_last,
+           nr_cpu_ids, per_cpu(cpu_sibling_mask, cpu)->bits,
+           nr_cpu_ids, per_cpu(cpu_core_mask, cpu)->bits);
 
     /* current VCPU (nothing to say if that's the idle vcpu). */
     svc = CSCHED_VCPU(curr_on_cpu(cpu));
@@ -2086,7 +2084,6 @@ csched_dump_pcpu(const struct scheduler *ops, int cpu)
 
     pcpu_schedule_unlock(lock, cpu);
     spin_unlock_irqrestore(&prv->lock, flags);
-#undef cpustr
 }
 
 static void
@@ -2099,8 +2096,6 @@ csched_dump(const struct scheduler *ops)
 
     spin_lock_irqsave(&prv->lock, flags);
 
-#define idlers_buf keyhandler_scratch
-
     printk("info:\n"
            "\tncpus              = %u\n"
            "\tmaster             = %u\n"
@@ -2127,8 +2122,7 @@ csched_dump(const struct scheduler *ops)
            prv->ticks_per_tslice,
            prv->vcpu_migr_delay/ MICROSECS(1));
 
-    cpumask_scnprintf(idlers_buf, sizeof(idlers_buf), prv->idlers);
-    printk("idlers: %s\n", idlers_buf);
+    printk("idlers: %*pb\n", nr_cpu_ids, prv->idlers->bits);
 
     printk("active vcpus:\n");
     loop = 0;
@@ -2151,7 +2145,6 @@ csched_dump(const struct scheduler *ops)
             vcpu_schedule_unlock(lock, svc->vcpu);
         }
     }
-#undef idlers_buf
 
     spin_unlock_irqrestore(&prv->lock, flags);
 }
diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index 2b16bce..4adb6fc 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -3654,12 +3654,11 @@ dump_pcpu(const struct scheduler *ops, int cpu)
 {
     struct csched2_private *prv = csched2_priv(ops);
     struct csched2_vcpu *svc;
-#define cpustr keyhandler_scratch
 
-    cpumask_scnprintf(cpustr, sizeof(cpustr), per_cpu(cpu_sibling_mask, cpu));
-    printk("CPU[%02d] runq=%d, sibling=%s, ", cpu, c2r(cpu), cpustr);
-    cpumask_scnprintf(cpustr, sizeof(cpustr), per_cpu(cpu_core_mask, cpu));
-    printk("core=%s\n", cpustr);
+    printk("CPU[%02d] runq=%d, sibling=%*pb, core=%*pb\n",
+           cpu, c2r(cpu),
+           nr_cpu_ids, per_cpu(cpu_sibling_mask, cpu)->bits,
+           nr_cpu_ids, per_cpu(cpu_core_mask, cpu)->bits);
 
     /* current VCPU (nothing to say if that's the idle vcpu) */
     svc = csched2_vcpu(curr_on_cpu(cpu));
@@ -3668,7 +3667,6 @@ dump_pcpu(const struct scheduler *ops, int cpu)
         printk("\trun: ");
         csched2_dump_vcpu(prv, svc);
     }
-#undef cpustr
 }
 
 static void
@@ -3678,7 +3676,6 @@ csched2_dump(const struct scheduler *ops)
     struct csched2_private *prv = csched2_priv(ops);
     unsigned long flags;
     unsigned int i, j, loop;
-#define cpustr keyhandler_scratch
 
     /*
      * We need the private scheduler lock as we access global
@@ -3696,29 +3693,26 @@ csched2_dump(const struct scheduler *ops)
 
         fraction = (prv->rqd[i].avgload * 100) >> prv->load_precision_shift;
 
-        cpulist_scnprintf(cpustr, sizeof(cpustr), &prv->rqd[i].active);
         printk("Runqueue %d:\n"
                "\tncpus              = %u\n"
-               "\tcpus               = %s\n"
+               "\tcpus               = %*pbl\n"
                "\tmax_weight         = %u\n"
                "\tpick_bias          = %u\n"
                "\tinstload           = %d\n"
                "\taveload            = %"PRI_stime" (~%"PRI_stime"%%)\n",
                i,
                cpumask_weight(&prv->rqd[i].active),
-               cpustr,
+               nr_cpu_ids, prv->rqd[i].active.bits,
                prv->rqd[i].max_weight,
                prv->rqd[i].pick_bias,
                prv->rqd[i].load,
                prv->rqd[i].avgload,
                fraction);
 
-        cpumask_scnprintf(cpustr, sizeof(cpustr), &prv->rqd[i].idle);
-        printk("\tidlers: %s\n", cpustr);
-        cpumask_scnprintf(cpustr, sizeof(cpustr), &prv->rqd[i].tickled);
-        printk("\ttickled: %s\n", cpustr);
-        cpumask_scnprintf(cpustr, sizeof(cpustr), &prv->rqd[i].smt_idle);
-        printk("\tfully idle cores: %s\n", cpustr);
+        printk("\tidlers: %*pb\n", nr_cpu_ids, prv->rqd[i].idle.bits);
+        printk("\ttickled: %*pb\n", nr_cpu_ids, prv->rqd[i].tickled.bits);
+        printk("\tfully idle cores: %*pb\n",
+               nr_cpu_ids, prv->rqd[i].smt_idle.bits);
     }
 
     printk("Domain info:\n");
@@ -3779,7 +3773,6 @@ csched2_dump(const struct scheduler *ops)
     }
 
     read_unlock_irqrestore(&prv->lock, flags);
-#undef cpustr
 }
 
 static void *
diff --git a/xen/common/sched_null.c b/xen/common/sched_null.c
index 7b039b7..fdaeab8 100644
--- a/xen/common/sched_null.c
+++ b/xen/common/sched_null.c
@@ -793,14 +793,13 @@ static void null_dump_pcpu(const struct scheduler *ops, int cpu)
     struct null_vcpu *nvc;
     spinlock_t *lock;
     unsigned long flags;
-#define cpustr keyhandler_scratch
 
     lock = pcpu_schedule_lock_irqsave(cpu, &flags);
 
-    cpumask_scnprintf(cpustr, sizeof(cpustr), per_cpu(cpu_sibling_mask, cpu));
-    printk("CPU[%02d] sibling=%s, ", cpu, cpustr);
-    cpumask_scnprintf(cpustr, sizeof(cpustr), per_cpu(cpu_core_mask, cpu));
-    printk("core=%s", cpustr);
+    printk("CPU[%02d] sibling=%*pb, core=%*pb",
+           cpu,
+           nr_cpu_ids, per_cpu(cpu_sibling_mask, cpu)->bits,
+           nr_cpu_ids, per_cpu(cpu_core_mask, cpu)->bits);
     if ( per_cpu(npc, cpu).vcpu != NULL )
         printk(", vcpu=%pv", per_cpu(npc, cpu).vcpu);
     printk("\n");
@@ -815,7 +814,6 @@ static void null_dump_pcpu(const struct scheduler *ops, int cpu)
     }
 
     pcpu_schedule_unlock_irqrestore(lock, flags, cpu);
-#undef cpustr
 }
 
 static void null_dump(const struct scheduler *ops)
@@ -824,12 +822,10 @@ static void null_dump(const struct scheduler *ops)
     struct list_head *iter;
     unsigned long flags;
     unsigned int loop;
-#define cpustr keyhandler_scratch
 
     spin_lock_irqsave(&prv->lock, flags);
 
-    cpulist_scnprintf(cpustr, sizeof(cpustr), &prv->cpus_free);
-    printk("\tcpus_free = %s\n", cpustr);
+    printk("\tcpus_free = %*pbl\n", nr_cpu_ids, prv->cpus_free.bits);
 
     printk("Domain info:\n");
     loop = 0;
@@ -873,7 +869,6 @@ static void null_dump(const struct scheduler *ops)
     spin_unlock(&prv->waitq_lock);
 
     spin_unlock_irqrestore(&prv->lock, flags);
-#undef cpustr
 }
 
 const struct scheduler sched_null_def = {
diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
index 59fbfa6..fffbeab 100644
--- a/xen/common/sched_rt.c
+++ b/xen/common/sched_rt.c
@@ -328,11 +328,10 @@ rt_dump_vcpu(const struct scheduler *ops, const struct rt_vcpu *svc)
 
     cpupool_mask = cpupool_domain_cpumask(svc->vcpu->domain);
     cpumask_and(mask, cpupool_mask, svc->vcpu->cpu_hard_affinity);
-    cpulist_scnprintf(keyhandler_scratch, sizeof(keyhandler_scratch), mask);
     printk("[%5d.%-2u] cpu %u, (%"PRI_stime", %"PRI_stime"),"
            " cur_b=%"PRI_stime" cur_d=%"PRI_stime" last_start=%"PRI_stime"\n"
            " \t\t priority_level=%d has_extratime=%d\n"
-           " \t\t onQ=%d runnable=%d flags=%x effective hard_affinity=%s\n",
+           " \t\t onQ=%d runnable=%d flags=%x effective hard_affinity=%*pbl\n",
             svc->vcpu->domain->domain_id,
             svc->vcpu->vcpu_id,
             svc->vcpu->processor,
@@ -346,7 +345,7 @@ rt_dump_vcpu(const struct scheduler *ops, const struct rt_vcpu *svc)
             vcpu_on_q(svc),
             vcpu_runnable(svc->vcpu),
             svc->flags,
-            keyhandler_scratch);
+            nr_cpu_ids, mask->bits);
 }
 
 static void
-- 
2.1.4


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

[PATCH v2 2/5] xen/common: Use %*pb[l] instead of {cpu, node}mask_scn{, list}printf()

[Andrew Cooper]

This removes all use of keyhandler_scratch as a bounce-buffer for the rendered
string.  In some cases, collapse combine adjacent printk()'s which are writing
parts of the same line.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Juergen Gross <jgross@suse.com>
---
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>

v2:
 * Use ->bits for cpumasks
---
 xen/common/cpupool.c       | 12 +++---------
 xen/common/event_channel.c |  6 ++----
 xen/common/keyhandler.c    | 35 +++++++++--------------------------
 3 files changed, 14 insertions(+), 39 deletions(-)

diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
index 1e8edcb..121fcfc 100644
--- a/xen/common/cpupool.c
+++ b/xen/common/cpupool.c
@@ -732,12 +732,6 @@ int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op)
     return ret;
 }
 
-static void print_cpumap(const char *str, const cpumask_t *map)
-{
-    cpulist_scnprintf(keyhandler_scratch, sizeof(keyhandler_scratch), map);
-    printk("%s: %s\n", str, keyhandler_scratch);
-}
-
 void dump_runq(unsigned char key)
 {
     unsigned long    flags;
@@ -751,17 +745,17 @@ void dump_runq(unsigned char key)
             sched_smt_power_savings? "enabled":"disabled");
     printk("NOW=%"PRI_stime"\n", now);
 
-    print_cpumap("Online Cpus", &cpu_online_map);
+    printk("Online Cpus: %*pbl\n", nr_cpu_ids, cpu_online_map.bits);
     if ( !cpumask_empty(&cpupool_free_cpus) )
     {
-        print_cpumap("Free Cpus", &cpupool_free_cpus);
+        printk("Free Cpus: %*pbl\n", nr_cpu_ids, cpupool_free_cpus.bits);
         schedule_dump(NULL);
     }
 
     for_each_cpupool(c)
     {
         printk("Cpupool %d:\n", (*c)->cpupool_id);
-        print_cpumap("Cpus", (*c)->cpu_valid);
+        printk("Cpus: %*pbl\n", nr_cpu_ids, (*c)->cpu_valid->bits);
         schedule_dump(*c);
     }
 
diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index 381f30e..f34d4f0 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -1377,11 +1377,9 @@ static void domain_dump_evtchn_info(struct domain *d)
     unsigned int port;
     int irq;
 
-    bitmap_scnlistprintf(keyhandler_scratch, sizeof(keyhandler_scratch),
-                         d->poll_mask, d->max_vcpus);
     printk("Event channel information for domain %d:\n"
-           "Polling vCPUs: {%s}\n"
-           "    port [p/m/s]\n", d->domain_id, keyhandler_scratch);
+           "Polling vCPUs: {%*pbl}\n"
+           "    port [p/m/s]\n", d->domain_id, d->max_vcpus, d->poll_mask);
 
     spin_lock(&d->event_lock);
 
diff --git a/xen/common/keyhandler.c b/xen/common/keyhandler.c
index 60bbeeb..4bb2643 100644
--- a/xen/common/keyhandler.c
+++ b/xen/common/keyhandler.c
@@ -250,22 +250,6 @@ static void reboot_machine(unsigned char key, struct cpu_user_regs *regs)
     machine_restart(0);
 }
 
-static void cpuset_print(char *set, int size, const cpumask_t *mask)
-{
-    *set++ = '{';
-    set += cpulist_scnprintf(set, size-2, mask);
-    *set++ = '}';
-    *set++ = '\0';
-}
-
-static void nodeset_print(char *set, int size, const nodemask_t *mask)
-{
-    *set++ = '[';
-    set += nodelist_scnprintf(set, size-2, mask);
-    *set++ = ']';
-    *set++ = '\0';
-}
-
 static void periodic_timer_print(char *str, int size, uint64_t period)
 {
     if ( period == 0 )
@@ -298,14 +282,14 @@ static void dump_domains(unsigned char key)
         process_pending_softirqs();
 
         printk("General information for domain %u:\n", d->domain_id);
-        cpuset_print(tmpstr, sizeof(tmpstr), d->dirty_cpumask);
         printk("    refcnt=%d dying=%d pause_count=%d\n",
                atomic_read(&d->refcnt), d->is_dying,
                atomic_read(&d->pause_count));
         printk("    nr_pages=%d xenheap_pages=%d shared_pages=%u paged_pages=%u "
-               "dirty_cpus=%s max_pages=%u\n", d->tot_pages, d->xenheap_pages,
-                atomic_read(&d->shr_pages), atomic_read(&d->paged_pages),
-                tmpstr, d->max_pages);
+               "dirty_cpus={%*pbl} max_pages=%u\n",
+               d->tot_pages, d->xenheap_pages, atomic_read(&d->shr_pages),
+               atomic_read(&d->paged_pages), nr_cpu_ids, d->dirty_cpumask->bits,
+               d->max_pages);
         printk("    handle=%02x%02x%02x%02x-%02x%02x-%02x%02x-"
                "%02x%02x-%02x%02x%02x%02x%02x%02x vm_assist=%08lx\n",
                d->handle[ 0], d->handle[ 1], d->handle[ 2], d->handle[ 3],
@@ -324,8 +308,8 @@ static void dump_domains(unsigned char key)
 
         dump_pageframe_info(d);
 
-        nodeset_print(tmpstr, sizeof(tmpstr), &d->node_affinity);
-        printk("NODE affinity for domain %d: %s\n", d->domain_id, tmpstr);
+        printk("NODE affinity for domain %d: [%*pbl]\n",
+               d->domain_id, MAX_NUMNODES, d->node_affinity.bits);
 
         printk("VCPU information and callbacks for domain %u:\n",
                d->domain_id);
@@ -343,10 +327,9 @@ static void dump_domains(unsigned char key)
             if ( vcpu_cpu_dirty(v) )
                 printk("dirty_cpu=%u", v->dirty_cpu);
             printk("\n");
-            cpuset_print(tmpstr, sizeof(tmpstr), v->cpu_hard_affinity);
-            printk("    cpu_hard_affinity=%s ", tmpstr);
-            cpuset_print(tmpstr, sizeof(tmpstr), v->cpu_soft_affinity);
-            printk("cpu_soft_affinity=%s\n", tmpstr);
+            printk("    cpu_hard_affinity={%*pbl} cpu_soft_affinity={%*pbl}\n",
+                   nr_cpu_ids, v->cpu_hard_affinity->bits,
+                   nr_cpu_ids, v->cpu_soft_affinity->bits);
             printk("    pause_count=%d pause_flags=%lx\n",
                    atomic_read(&v->pause_count), v->pause_flags);
             arch_dump_vcpu_info(v);
-- 
2.1.4


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

[PATCH v2 3/5] xen/x86: Use %*pb[l] instead of cpumask_scn{, list}printf()

[Andrew Cooper]

This removes all use of keyhandler_scratch as a bounce-buffer for the rendered
string.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
v2:
 * Fix %pd typo
 * Use ->bits for cpumasks
---
 xen/arch/x86/cpu/mcheck/mce.c | 9 ++++++---
 xen/arch/x86/crash.c          | 7 ++-----
 xen/arch/x86/irq.c            | 7 ++-----
 3 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c
index 1eec631..c4835be 100644
--- a/xen/arch/x86/cpu/mcheck/mce.c
+++ b/xen/arch/x86/cpu/mcheck/mce.c
@@ -535,9 +535,12 @@ void mcheck_cmn_handler(const struct cpu_user_regs *regs)
             mc_panic("MCE: No CPU found valid MCE, need reset");
         if ( !cpumask_empty(&mce_fatal_cpus) )
         {
-            char *ebufp, ebuf[96] = "MCE: Fatal error happened on CPUs ";
-            ebufp = ebuf + strlen(ebuf);
-            cpumask_scnprintf(ebufp, 95 - strlen(ebuf), &mce_fatal_cpus);
+            char ebuf[96];
+
+            snprintf(ebuf, sizeof(ebuf),
+                     "MCE: Fatal error happened on CPUs %*pb",
+                     nr_cpu_ids, mce_fatal_cpus.bits);
+
             mc_panic(ebuf);
         }
         atomic_set(&found_error, 0);
diff --git a/xen/arch/x86/crash.c b/xen/arch/x86/crash.c
index 8d74258..81bb296 100644
--- a/xen/arch/x86/crash.c
+++ b/xen/arch/x86/crash.c
@@ -159,11 +159,8 @@ static void nmi_shootdown_cpus(void)
     if ( cpumask_empty(&waiting_to_crash) )
         printk("Shot down all CPUs\n");
     else
-    {
-        cpulist_scnprintf(keyhandler_scratch, sizeof keyhandler_scratch,
-                          &waiting_to_crash);
-        printk("Failed to shoot down CPUs {%s}\n", keyhandler_scratch);
-    }
+        printk("Failed to shoot down CPUs {%*pbl}\n",
+               nr_cpu_ids, waiting_to_crash.bits);
 
     /* Crash shutdown any IOMMU functionality as the crashdump kernel is not
      * happy when booting if interrupt/dma remapping is still enabled */
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index 35e7de5..006bdc7 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -2302,11 +2302,8 @@ static void dump_irqs(unsigned char key)
 
         spin_lock_irqsave(&desc->lock, flags);
 
-        cpumask_scnprintf(keyhandler_scratch, sizeof(keyhandler_scratch),
-                          desc->affinity);
-        printk("   IRQ:%4d affinity:%s vec:%02x type=%-15s"
-               " status=%08x ",
-               irq, keyhandler_scratch, desc->arch.vector,
+        printk("   IRQ:%4d affinity:%*pb vec:%02x type=%-15s status=%08x ",
+               irq, nr_cpu_ids, desc->affinity->bits, desc->arch.vector,
                desc->handler->typename, desc->status);
 
         if ( ssid )
-- 
2.1.4


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

[PATCH v2 4/5] xen/bitmap: Drop all bitmap_scn{, list}printf() infrastructure

[Andrew Cooper]

All callers have been convered to using %*pb[l].  In the unlikely case that
future code wants to retain this functionaly, it can be replicated in a more
convenient fashon with snprintf().

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
Reviewed-by: Dario Faggioli <dfaggioli@suse.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
---
 xen/common/bitmap.c        | 105 ---------------------------------------------
 xen/include/xen/bitmap.h   |   6 ---
 xen/include/xen/cpumask.h  |  18 --------
 xen/include/xen/nodemask.h |  34 ---------------
 4 files changed, 163 deletions(-)

diff --git a/xen/common/bitmap.c b/xen/common/bitmap.c
index f498ee6..34de387 100644
--- a/xen/common/bitmap.c
+++ b/xen/common/bitmap.c
@@ -300,111 +300,6 @@ int __bitmap_weight(const unsigned long *bitmap, int bits)
 #endif
 EXPORT_SYMBOL(__bitmap_weight);
 
-/*
- * Bitmap printing & parsing functions: first version by Bill Irwin,
- * second version by Paul Jackson, third by Joe Korty.
- */
-
-#define CHUNKSZ				32
-#define nbits_to_hold_value(val)	fls(val)
-#define roundup_power2(val,modulus)	(((val) + (modulus) - 1) & ~((modulus) - 1))
-#define unhex(c)			(isdigit(c) ? (c - '0') : (toupper(c) - 'A' + 10))
-#define BASEDEC 10		/* fancier cpuset lists input in decimal */
-
-/**
- * bitmap_scnprintf - convert bitmap to an ASCII hex string.
- * @buf: byte buffer into which string is placed
- * @buflen: reserved size of @buf, in bytes
- * @maskp: pointer to bitmap to convert
- * @nmaskbits: size of bitmap, in bits
- *
- * Exactly @nmaskbits bits are displayed.  Hex digits are grouped into
- * comma-separated sets of eight digits per set.
- */
-int bitmap_scnprintf(char *buf, unsigned int buflen,
-	const unsigned long *maskp, int nmaskbits)
-{
-	int i, word, bit, len = 0;
-	unsigned long val;
-	const char *sep = "";
-	int chunksz;
-	u32 chunkmask;
-
-	chunksz = nmaskbits & (CHUNKSZ - 1);
-	if (chunksz == 0)
-		chunksz = CHUNKSZ;
-
-	i = roundup_power2(nmaskbits, CHUNKSZ) - CHUNKSZ;
-	for (; i >= 0; i -= CHUNKSZ) {
-		chunkmask = ((1ULL << chunksz) - 1);
-		word = i / BITS_PER_LONG;
-		bit = i % BITS_PER_LONG;
-		val = (maskp[word] >> bit) & chunkmask;
-		len += scnprintf(buf+len, buflen-len, "%s%0*lx", sep,
-			(chunksz+3)/4, val);
-		chunksz = CHUNKSZ;
-		sep = ",";
-	}
-	return len;
-}
-EXPORT_SYMBOL(bitmap_scnprintf);
-
-/*
- * bscnl_emit(buf, buflen, rbot, rtop, bp)
- *
- * Helper routine for bitmap_scnlistprintf().  Write decimal number
- * or range to buf, suppressing output past buf+buflen, with optional
- * comma-prefix.  Return len of what would be written to buf, if it
- * all fit.
- */
-static inline int bscnl_emit(char *buf, int buflen, int rbot, int rtop, int len)
-{
-	if (len > 0)
-		len += scnprintf(buf + len, buflen - len, ",");
-	if (rbot == rtop)
-		len += scnprintf(buf + len, buflen - len, "%d", rbot);
-	else
-		len += scnprintf(buf + len, buflen - len, "%d-%d", rbot, rtop);
-	return len;
-}
-
-/**
- * bitmap_scnlistprintf - convert bitmap to list format ASCII string
- * @buf: byte buffer into which string is placed
- * @buflen: reserved size of @buf, in bytes
- * @maskp: pointer to bitmap to convert
- * @nmaskbits: size of bitmap, in bits
- *
- * Output format is a comma-separated list of decimal numbers and
- * ranges.  Consecutively set bits are shown as two hyphen-separated
- * decimal numbers, the smallest and largest bit numbers set in
- * the range.  Output format is compatible with the format
- * accepted as input by bitmap_parselist().
- *
- * The return value is the number of characters which were output,
- * excluding the trailing '\0'.
- */
-int bitmap_scnlistprintf(char *buf, unsigned int buflen,
-	const unsigned long *maskp, int nmaskbits)
-{
-	int len = 0;
-	/* current bit is 'cur', most recently seen range is [rbot, rtop] */
-	int cur, rbot, rtop;
-
-	rbot = cur = find_first_bit(maskp, nmaskbits);
-	while (cur < nmaskbits) {
-		rtop = cur;
-		cur = find_next_bit(maskp, nmaskbits, cur+1);
-		if (cur >= nmaskbits || cur > rtop + 1) {
-			len = bscnl_emit(buf, buflen, rbot, rtop, len);
-			rbot = cur;
-		}
-	}
-	if (!len && buflen)
-		*buf = 0;
-	return len;
-}
-EXPORT_SYMBOL(bitmap_scnlistprintf);
 
 /**
  *	bitmap_find_free_region - find a contiguous aligned mem region
diff --git a/xen/include/xen/bitmap.h b/xen/include/xen/bitmap.h
index e2a3686..fe3c720 100644
--- a/xen/include/xen/bitmap.h
+++ b/xen/include/xen/bitmap.h
@@ -40,8 +40,6 @@
  * bitmap_weight(src, nbits)			Hamming Weight: number set bits
  * bitmap_shift_right(dst, src, n, nbits)	*dst = *src >> n
  * bitmap_shift_left(dst, src, n, nbits)	*dst = *src << n
- * bitmap_scnprintf(buf, len, src, nbits)	Print bitmap src to buf
- * bitmap_scnlistprintf(buf, len, src, nbits)	Print bitmap src as list to buf
  */
 
 /*
@@ -94,10 +92,6 @@ extern int __bitmap_subset(const unsigned long *bitmap1,
 			const unsigned long *bitmap2, int bits);
 extern int __bitmap_weight(const unsigned long *bitmap, int bits);
 
-extern int bitmap_scnprintf(char *buf, unsigned int len,
-			const unsigned long *src, int nbits);
-extern int bitmap_scnlistprintf(char *buf, unsigned int len,
-			const unsigned long *src, int nbits);
 extern int bitmap_find_free_region(unsigned long *bitmap, int bits, int order);
 extern void bitmap_release_region(unsigned long *bitmap, int pos, int order);
 extern int bitmap_allocate_region(unsigned long *bitmap, int pos, int order);
diff --git a/xen/include/xen/cpumask.h b/xen/include/xen/cpumask.h
index 4731a63..b4cc92a 100644
--- a/xen/include/xen/cpumask.h
+++ b/xen/include/xen/cpumask.h
@@ -8,9 +8,6 @@
  * See detailed comments in the file xen/bitmap.h describing the
  * data type on which these cpumasks are based.
  *
- * For details of cpumask_scnprintf() and cpulist_scnprintf(),
- * see bitmap_scnprintf() and bitmap_scnlistprintf() in lib/bitmap.c.
- *
  * The available cpumask operations are:
  *
  * void cpumask_set_cpu(cpu, mask)	turn on bit 'cpu' in mask
@@ -46,9 +43,6 @@
  * const cpumask_t *cpumask_of(cpu)	Return cpumask with bit 'cpu' set
  * unsigned long *cpumask_bits(mask)	Array of unsigned long's in mask
  *
- * int cpumask_scnprintf(buf, len, mask) Format cpumask for printing
- * int cpulist_scnprintf(buf, len, mask) Format cpumask as list for printing
- *
  * for_each_cpu(cpu, mask)		for-loop cpu over mask
  *
  * int num_online_cpus()		Number of online CPUs
@@ -312,18 +306,6 @@ static inline const cpumask_t *cpumask_of(unsigned int cpu)
 
 #define cpumask_bits(maskp) ((maskp)->bits)
 
-static inline int cpumask_scnprintf(char *buf, int len,
-				    const cpumask_t *srcp)
-{
-	return bitmap_scnprintf(buf, len, srcp->bits, nr_cpu_ids);
-}
-
-static inline int cpulist_scnprintf(char *buf, int len,
-				    const cpumask_t *srcp)
-{
-	return bitmap_scnlistprintf(buf, len, srcp->bits, nr_cpu_ids);
-}
-
 /*
  * cpumask_var_t: struct cpumask for stack usage.
  *
diff --git a/xen/include/xen/nodemask.h b/xen/include/xen/nodemask.h
index 2a90dc1..e287399 100644
--- a/xen/include/xen/nodemask.h
+++ b/xen/include/xen/nodemask.h
@@ -8,10 +8,6 @@
  * See detailed comments in the file linux/bitmap.h describing the
  * data type on which these nodemasks are based.
  *
- * For details of nodemask_scnprintf(), nodelist_scnpintf() and
- * nodemask_parse(), see bitmap_scnprintf() and bitmap_parse()
- * in lib/bitmap.c.
- *
  * The available nodemask operations are:
  *
  * void node_set(node, mask)		turn on bit 'node' in mask
@@ -50,10 +46,6 @@
  * NODE_MASK_NONE			Initializer - no bits set
  * unsigned long *nodes_addr(mask)	Array of unsigned long's in mask
  *
- * int nodemask_scnprintf(buf, len, mask) Format nodemask for printing
- * int nodelist_scnprintf(buf, len, mask) Format nodemask as a list for printing
- * int nodemask_parse(ubuf, ulen, mask)	Parse ascii string as nodemask
- *
  * for_each_node_mask(node, mask)	for-loop node over mask
  *
  * int num_online_nodes()		Number of online Nodes
@@ -294,32 +286,6 @@ static inline int __cycle_node(int n, const nodemask_t *maskp, int nbits)
 
 #define nodes_addr(src) ((src).bits)
 
-#define nodelist_scnprintf(buf, len, src) \
-			__nodelist_scnprintf((buf), (len), (src), MAX_NUMNODES)
-static inline int __nodelist_scnprintf(char *buf, int len,
-					const nodemask_t *srcp, int nbits)
-{
-	return bitmap_scnlistprintf(buf, len, srcp->bits, nbits);
-}
-
-#if 0
-#define nodemask_scnprintf(buf, len, src) \
-			__nodemask_scnprintf((buf), (len), &(src), MAX_NUMNODES)
-static inline int __nodemask_scnprintf(char *buf, int len,
-					const nodemask_t *srcp, int nbits)
-{
-	return bitmap_scnprintf(buf, len, srcp->bits, nbits);
-}
-
-#define nodemask_parse(ubuf, ulen, dst) \
-			__nodemask_parse((ubuf), (ulen), &(dst), MAX_NUMNODES)
-static inline int __nodemask_parse(const char __user *buf, int len,
-					nodemask_t *dstp, int nbits)
-{
-	return bitmap_parse(buf, len, dstp->bits, nbits);
-}
-#endif
-
 #if MAX_NUMNODES > 1
 #define for_each_node_mask(node, mask)			\
 	for ((node) = first_node(mask);			\
-- 
2.1.4


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

[PATCH v2 5/5] xen/keyhandler: Drop keyhandler_scratch

[Andrew Cooper]

With almost all users of keyhandler_scratch gone, clean up the 3 remaining
users and drop the buffer.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>

v2:
 * Use a static __initdata buffer for EFI, rather than a stack variable.
 * Drop (int) casts for periodic timer printing.
---
 xen/arch/x86/numa.c          | 11 ++++-------
 xen/common/efi/boot.c        |  4 ++--
 xen/common/keyhandler.c      | 26 +++++++-------------------
 xen/include/xen/keyhandler.h |  3 ---
 4 files changed, 13 insertions(+), 31 deletions(-)

diff --git a/xen/arch/x86/numa.c b/xen/arch/x86/numa.c
index 8e08173..b3c9c12 100644
--- a/xen/arch/x86/numa.c
+++ b/xen/arch/x86/numa.c
@@ -372,7 +372,6 @@ static void dump_numa(unsigned char key)
 {
     s_time_t now = NOW();
     unsigned int i, j, n;
-    int err;
     struct domain *d;
     struct page_info *page;
     unsigned int page_num_node[MAX_NUMNODES];
@@ -454,12 +453,10 @@ static void dump_numa(unsigned char key)
         {
             unsigned int start_cpu = ~0U;
 
-            err = snprintf(keyhandler_scratch, 12, "%3u",
-                    vnuma->vnode_to_pnode[i]);
-            if ( err < 0 || vnuma->vnode_to_pnode[i] == NUMA_NO_NODE )
-                strlcpy(keyhandler_scratch, "???", sizeof(keyhandler_scratch));
-
-            printk("       %3u: pnode %s,", i, keyhandler_scratch);
+            if ( vnuma->vnode_to_pnode[i] == NUMA_NO_NODE )
+                printk("       %3u: pnode ???,", i);
+            else
+                printk("       %3u: pnode %3u,", i, vnuma->vnode_to_pnode[i]);
 
             printk(" vcpus ");
 
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 2f49731..ef8c77c 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -487,6 +487,7 @@ static EFI_FILE_HANDLE __init get_parent_handle(EFI_LOADED_IMAGE *loaded_image,
                                                 CHAR16 **leaf)
 {
     static EFI_GUID __initdata fs_protocol = SIMPLE_FILE_SYSTEM_PROTOCOL;
+    static CHAR16 __initdata buffer[256];
     EFI_FILE_HANDLE dir_handle;
     EFI_DEVICE_PATH *dp;
     CHAR16 *pathend, *ptr;
@@ -506,8 +507,7 @@ static EFI_FILE_HANDLE __init get_parent_handle(EFI_LOADED_IMAGE *loaded_image,
     if ( ret != EFI_SUCCESS )
         PrintErrMesg(L"OpenVolume failure", ret);
 
-#define buffer ((CHAR16 *)keyhandler_scratch)
-#define BUFFERSIZE sizeof(keyhandler_scratch)
+#define BUFFERSIZE sizeof(buffer)
     for ( dp = loaded_image->FilePath, *buffer = 0;
           DevicePathType(dp) != END_DEVICE_PATH_TYPE;
           dp = (void *)dp + DevicePathNodeLength(dp) )
diff --git a/xen/common/keyhandler.c b/xen/common/keyhandler.c
index 4bb2643..7333f55 100644
--- a/xen/common/keyhandler.c
+++ b/xen/common/keyhandler.c
@@ -29,8 +29,6 @@ static keyhandler_fn_t show_handlers, dump_hwdom_registers,
 static irq_keyhandler_fn_t do_toggle_alt_key, dump_registers,
     reboot_machine, run_all_keyhandlers, do_debug_key;
 
-char keyhandler_scratch[1024];
-
 static struct keyhandler {
     union {
         keyhandler_fn_t *fn;
@@ -250,25 +248,11 @@ static void reboot_machine(unsigned char key, struct cpu_user_regs *regs)
     machine_restart(0);
 }
 
-static void periodic_timer_print(char *str, int size, uint64_t period)
-{
-    if ( period == 0 )
-    {
-        strlcpy(str, "No periodic timer", size);
-        return;
-    }
-
-    snprintf(str, size,
-             "%u Hz periodic timer (period %u ms)",
-             1000000000/(int)period, (int)period/1000000);
-}
-
 static void dump_domains(unsigned char key)
 {
     struct domain *d;
     struct vcpu   *v;
     s_time_t       now = NOW();
-#define tmpstr keyhandler_scratch
 
     printk("'%c' pressed -> dumping domain info (now = %"PRI_stime"\n", key,
            now);
@@ -333,8 +317,13 @@ static void dump_domains(unsigned char key)
             printk("    pause_count=%d pause_flags=%lx\n",
                    atomic_read(&v->pause_count), v->pause_flags);
             arch_dump_vcpu_info(v);
-            periodic_timer_print(tmpstr, sizeof(tmpstr), v->periodic_period);
-            printk("    %s\n", tmpstr);
+
+            if ( v->periodic_period == 0 )
+                printk("No periodic timer\n");
+            else
+                printk("%"PRI_stime" Hz periodic timer (period %"PRI_stime" ms)\n",
+                       1000000000 / v->periodic_period,
+                       v->periodic_period / 1000000);
         }
     }
 
@@ -355,7 +344,6 @@ static void dump_domains(unsigned char key)
     arch_dump_shared_mem_info();
 
     rcu_read_unlock(&domlist_read_lock);
-#undef tmpstr
 }
 
 static cpumask_t read_clocks_cpumask;
diff --git a/xen/include/xen/keyhandler.h b/xen/include/xen/keyhandler.h
index 06c05c8..5131e86 100644
--- a/xen/include/xen/keyhandler.h
+++ b/xen/include/xen/keyhandler.h
@@ -48,7 +48,4 @@ void register_irq_keyhandler(unsigned char key,
 /* Inject a keypress into the key-handling subsystem. */
 extern void handle_keypress(unsigned char key, struct cpu_user_regs *regs);
 
-/* Scratch space is available for use of any keyhandler. */
-extern char keyhandler_scratch[1024];
-
 #endif /* __XEN_KEYHANDLER_H__ */
-- 
2.1.4


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

Re: [PATCH v2 2/5] xen/common: Use %*pb[l] instead of {cpu, node}mask_scn{, list}printf()

[Jan Beulich]

>>> On 22.10.18 at 14:57, <andrew.cooper3@citrix.com> wrote:
> This removes all use of keyhandler_scratch as a bounce-buffer for the rendered
> string.  In some cases, collapse combine adjacent printk()'s which are writing
> parts of the same line.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Wei Liu <wei.liu2@citrix.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>
> Acked-by: Juergen Gross <jgross@suse.com>
> ---
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> 
> v2:
>  * Use ->bits for cpumasks

No, I'm sorry - this is not what I gave my ack for. I specifically
said that ->bits may only be used by the cpumask implementation.
Everything else should use the cpumask_bits() wrapper.

> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -1377,11 +1377,9 @@ static void domain_dump_evtchn_info(struct domain *d)
>      unsigned int port;
>      int irq;
>  
> -    bitmap_scnlistprintf(keyhandler_scratch, sizeof(keyhandler_scratch),
> -                         d->poll_mask, d->max_vcpus);
>      printk("Event channel information for domain %d:\n"
> -           "Polling vCPUs: {%s}\n"
> -           "    port [p/m/s]\n", d->domain_id, keyhandler_scratch);
> +           "Polling vCPUs: {%*pbl}\n"
> +           "    port [p/m/s]\n", d->domain_id, d->max_vcpus, d->poll_mask);

Neither cpumask_bits() nor its open coding here at all?

Jan


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

Re: [PATCH v2 3/5] xen/x86: Use %*pb[l] instead of cpumask_scn{, list}printf()

[Jan Beulich]

>>> On 22.10.18 at 14:58, <andrew.cooper3@citrix.com> wrote:
> This removes all use of keyhandler_scratch as a bounce-buffer for the 
> rendered
> string.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Wei Liu <wei.liu2@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2:
>  * Fix %pd typo
>  * Use ->bits for cpumasks

Same here as for patch 2.

Jan



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

Re: [PATCH v2 5/5] xen/keyhandler: Drop keyhandler_scratch

[Jan Beulich]

>>> On 22.10.18 at 14:58, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -487,6 +487,7 @@ static EFI_FILE_HANDLE __init get_parent_handle(EFI_LOADED_IMAGE *loaded_image,
>                                                  CHAR16 **leaf)
>  {
>      static EFI_GUID __initdata fs_protocol = SIMPLE_FILE_SYSTEM_PROTOCOL;
> +    static CHAR16 __initdata buffer[256];

Did you intentionally halve the size of the buffer?

Jan



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

Re: [PATCH v2 5/5] xen/keyhandler: Drop keyhandler_scratch

[Wei Liu]

On Fri, Oct 26, 2018 at 09:21:03AM -0600, Jan Beulich wrote:
> >>> On 22.10.18 at 14:58, <andrew.cooper3@citrix.com> wrote:
> > --- a/xen/common/efi/boot.c
> > +++ b/xen/common/efi/boot.c
> > @@ -487,6 +487,7 @@ static EFI_FILE_HANDLE __init get_parent_handle(EFI_LOADED_IMAGE *loaded_image,
> >                                                  CHAR16 **leaf)
> >  {
> >      static EFI_GUID __initdata fs_protocol = SIMPLE_FILE_SYSTEM_PROTOCOL;
> > +    static CHAR16 __initdata buffer[256];
> 
> Did you intentionally halve the size of the buffer?


Seeing the length field consists of two uint8, the maximum length of the
path is 256.

Wei.

> 
> Jan
> 
> 

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

Re: [PATCH v2 5/5] xen/keyhandler: Drop keyhandler_scratch

[Wei Liu]

On Mon, Oct 22, 2018 at 01:58:02PM +0100, Andrew Cooper wrote:
> With almost all users of keyhandler_scratch gone, clean up the 3 remaining
> users and drop the buffer.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH v2 5/5] xen/keyhandler: Drop keyhandler_scratch

[Jan Beulich]

>>> On 30.10.18 at 16:32, <wei.liu2@citrix.com> wrote:
> On Fri, Oct 26, 2018 at 09:21:03AM -0600, Jan Beulich wrote:
>> >>> On 22.10.18 at 14:58, <andrew.cooper3@citrix.com> wrote:
>> > --- a/xen/common/efi/boot.c
>> > +++ b/xen/common/efi/boot.c
>> > @@ -487,6 +487,7 @@ static EFI_FILE_HANDLE __init get_parent_handle(EFI_LOADED_IMAGE *loaded_image,
>> >                                                  CHAR16 **leaf)
>> >  {
>> >      static EFI_GUID __initdata fs_protocol = SIMPLE_FILE_SYSTEM_PROTOCOL;
>> > +    static CHAR16 __initdata buffer[256];
>> 
>> Did you intentionally halve the size of the buffer?
> 
> 
> Seeing the length field consists of two uint8, the maximum length of the
> path is 256.

With

#define DevicePathNodeLength(a)     ( ((a)->Length[0]) | ((a)->Length[1] << 8) )

I'd rather say 65536.

Jan



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

Re: [PATCH v2 5/5] xen/keyhandler: Drop keyhandler_scratch

[Wei Liu]

On Tue, Oct 30, 2018 at 09:40:17AM -0600, Jan Beulich wrote:
> >>> On 30.10.18 at 16:32, <wei.liu2@citrix.com> wrote:
> > On Fri, Oct 26, 2018 at 09:21:03AM -0600, Jan Beulich wrote:
> >> >>> On 22.10.18 at 14:58, <andrew.cooper3@citrix.com> wrote:
> >> > --- a/xen/common/efi/boot.c
> >> > +++ b/xen/common/efi/boot.c
> >> > @@ -487,6 +487,7 @@ static EFI_FILE_HANDLE __init get_parent_handle(EFI_LOADED_IMAGE *loaded_image,
> >> >                                                  CHAR16 **leaf)
> >> >  {
> >> >      static EFI_GUID __initdata fs_protocol = SIMPLE_FILE_SYSTEM_PROTOCOL;
> >> > +    static CHAR16 __initdata buffer[256];
> >> 
> >> Did you intentionally halve the size of the buffer?
> > 
> > 
> > Seeing the length field consists of two uint8, the maximum length of the
> > path is 256.
> 
> With
> 
> #define DevicePathNodeLength(a)     ( ((a)->Length[0]) | ((a)->Length[1] << 8) )
> 
> I'd rather say 65536.

Yes you're right. Can't do calculation when I'm having a cold. :-(
But having 65536 is unrealistic.

Wei.

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

Re: [PATCH v2 5/5] xen/keyhandler: Drop keyhandler_scratch

[Jan Beulich]

>>> On 30.10.18 at 16:46, <wei.liu2@citrix.com> wrote:
> On Tue, Oct 30, 2018 at 09:40:17AM -0600, Jan Beulich wrote:
>> >>> On 30.10.18 at 16:32, <wei.liu2@citrix.com> wrote:
>> > On Fri, Oct 26, 2018 at 09:21:03AM -0600, Jan Beulich wrote:
>> >> >>> On 22.10.18 at 14:58, <andrew.cooper3@citrix.com> wrote:
>> >> > --- a/xen/common/efi/boot.c
>> >> > +++ b/xen/common/efi/boot.c
>> >> > @@ -487,6 +487,7 @@ static EFI_FILE_HANDLE __init 
> get_parent_handle(EFI_LOADED_IMAGE *loaded_image,
>> >> >                                                  CHAR16 **leaf)
>> >> >  {
>> >> >      static EFI_GUID __initdata fs_protocol = SIMPLE_FILE_SYSTEM_PROTOCOL;
>> >> > +    static CHAR16 __initdata buffer[256];
>> >> 
>> >> Did you intentionally halve the size of the buffer?
>> > 
>> > 
>> > Seeing the length field consists of two uint8, the maximum length of the
>> > path is 256.
>> 
>> With
>> 
>> #define DevicePathNodeLength(a)     ( ((a)->Length[0]) | ((a)->Length[1] << 8) )
>> 
>> I'd rather say 65536.
> 
> Yes you're right. Can't do calculation when I'm having a cold. :-(
> But having 65536 is unrealistic.

Sure, I certainly don't mean the buffer to grow that large, but I'd
prefer if it didn't shrink compared to the original.

Jan



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

Re: [PATCH v2 2/5] xen/common: Use %*pb[l] instead of {cpu, node}mask_scn{, list}printf()

[Andrew Cooper]

On 25/10/18 11:56, Jan Beulich wrote:
>>>> On 22.10.18 at 14:57, <andrew.cooper3@citrix.com> wrote:
>> This removes all use of keyhandler_scratch as a bounce-buffer for the rendered
>> string.  In some cases, collapse combine adjacent printk()'s which are writing
>> parts of the same line.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Reviewed-by: Wei Liu <wei.liu2@citrix.com>
>> Acked-by: Jan Beulich <jbeulich@suse.com>
>> Acked-by: Juergen Gross <jgross@suse.com>
>> ---
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Julien Grall <julien.grall@arm.com>
>>
>> v2:
>>  * Use ->bits for cpumasks
> No, I'm sorry - this is not what I gave my ack for. I specifically
> said that ->bits may only be used by the cpumask implementation.
> Everything else should use the cpumask_bits() wrapper.

cpumask_bits() is worse than useless, because it is longer than the
alternative, and results in harder to read code.

I will not be adding to its (mis)use.

>
>> --- a/xen/common/event_channel.c
>> +++ b/xen/common/event_channel.c
>> @@ -1377,11 +1377,9 @@ static void domain_dump_evtchn_info(struct domain *d)
>>      unsigned int port;
>>      int irq;
>>  
>> -    bitmap_scnlistprintf(keyhandler_scratch, sizeof(keyhandler_scratch),
>> -                         d->poll_mask, d->max_vcpus);
>>      printk("Event channel information for domain %d:\n"
>> -           "Polling vCPUs: {%s}\n"
>> -           "    port [p/m/s]\n", d->domain_id, keyhandler_scratch);
>> +           "Polling vCPUs: {%*pbl}\n"
>> +           "    port [p/m/s]\n", d->domain_id, d->max_vcpus, d->poll_mask);
> Neither cpumask_bits() nor its open coding here at all?

No.  Why do you think poll_mask is a cpumask?  Its a straight bitmap.

~Andrew

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

Re: [PATCH v2 2/5] xen/common: Use %*pb[l] instead of {cpu, node}mask_scn{, list}printf()

[Jan Beulich]

>>> On 09.11.18 at 19:01, <andrew.cooper3@citrix.com> wrote:
> On 25/10/18 11:56, Jan Beulich wrote:
>>>>> On 22.10.18 at 14:57, <andrew.cooper3@citrix.com> wrote:
>>> This removes all use of keyhandler_scratch as a bounce-buffer for the 
> rendered
>>> string.  In some cases, collapse combine adjacent printk()'s which are 
> writing
>>> parts of the same line.
>>>
>>> No functional change.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Reviewed-by: Wei Liu <wei.liu2@citrix.com>
>>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>> Acked-by: Juergen Gross <jgross@suse.com>
>>> ---
>>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>> CC: Stefano Stabellini <sstabellini@kernel.org>
>>> CC: Julien Grall <julien.grall@arm.com>
>>>
>>> v2:
>>>  * Use ->bits for cpumasks
>> No, I'm sorry - this is not what I gave my ack for. I specifically
>> said that ->bits may only be used by the cpumask implementation.
>> Everything else should use the cpumask_bits() wrapper.
> 
> cpumask_bits() is worse than useless, because it is longer than the
> alternative, and results in harder to read code.
> 
> I will not be adding to its (mis)use.

Whether that's proper use or misuse would need to be decided. If
it's misuse, then the construct should go away. Despite being longer
than the open coding I do think though that it's better to have (and
use consistently).

>>> --- a/xen/common/event_channel.c
>>> +++ b/xen/common/event_channel.c
>>> @@ -1377,11 +1377,9 @@ static void domain_dump_evtchn_info(struct domain *d)
>>>      unsigned int port;
>>>      int irq;
>>>  
>>> -    bitmap_scnlistprintf(keyhandler_scratch, sizeof(keyhandler_scratch),
>>> -                         d->poll_mask, d->max_vcpus);
>>>      printk("Event channel information for domain %d:\n"
>>> -           "Polling vCPUs: {%s}\n"
>>> -           "    port [p/m/s]\n", d->domain_id, keyhandler_scratch);
>>> +           "Polling vCPUs: {%*pbl}\n"
>>> +           "    port [p/m/s]\n", d->domain_id, d->max_vcpus, d->poll_mask);
>> Neither cpumask_bits() nor its open coding here at all?
> 
> No.  Why do you think poll_mask is a cpumask?  Its a straight bitmap.

Oh, sorry.

Jan


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

[PATCH v3 5/5] xen/keyhandler: Drop keyhandler_scratch

[Andrew Cooper]

With almost all users of keyhandler_scratch gone, clean up the 3 remaining
users and drop the buffer.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>

v2:
 * Use a static __initdata buffer for EFI, rather than a stack variable.
 * Drop (int) casts for periodic timer printing.
v3:
 * Extend the EFI buffer to remain being 1024 bytes long.
---
 xen/arch/x86/numa.c          | 11 ++++-------
 xen/common/efi/boot.c        |  4 ++--
 xen/common/keyhandler.c      | 26 +++++++-------------------
 xen/include/xen/keyhandler.h |  3 ---
 4 files changed, 13 insertions(+), 31 deletions(-)

diff --git a/xen/arch/x86/numa.c b/xen/arch/x86/numa.c
index 8e08173..b3c9c12 100644
--- a/xen/arch/x86/numa.c
+++ b/xen/arch/x86/numa.c
@@ -372,7 +372,6 @@ static void dump_numa(unsigned char key)
 {
     s_time_t now = NOW();
     unsigned int i, j, n;
-    int err;
     struct domain *d;
     struct page_info *page;
     unsigned int page_num_node[MAX_NUMNODES];
@@ -454,12 +453,10 @@ static void dump_numa(unsigned char key)
         {
             unsigned int start_cpu = ~0U;
 
-            err = snprintf(keyhandler_scratch, 12, "%3u",
-                    vnuma->vnode_to_pnode[i]);
-            if ( err < 0 || vnuma->vnode_to_pnode[i] == NUMA_NO_NODE )
-                strlcpy(keyhandler_scratch, "???", sizeof(keyhandler_scratch));
-
-            printk("       %3u: pnode %s,", i, keyhandler_scratch);
+            if ( vnuma->vnode_to_pnode[i] == NUMA_NO_NODE )
+                printk("       %3u: pnode ???,", i);
+            else
+                printk("       %3u: pnode %3u,", i, vnuma->vnode_to_pnode[i]);
 
             printk(" vcpus ");
 
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 2f49731..2ed5403 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -487,6 +487,7 @@ static EFI_FILE_HANDLE __init get_parent_handle(EFI_LOADED_IMAGE *loaded_image,
                                                 CHAR16 **leaf)
 {
     static EFI_GUID __initdata fs_protocol = SIMPLE_FILE_SYSTEM_PROTOCOL;
+    static CHAR16 __initdata buffer[512];
     EFI_FILE_HANDLE dir_handle;
     EFI_DEVICE_PATH *dp;
     CHAR16 *pathend, *ptr;
@@ -506,8 +507,7 @@ static EFI_FILE_HANDLE __init get_parent_handle(EFI_LOADED_IMAGE *loaded_image,
     if ( ret != EFI_SUCCESS )
         PrintErrMesg(L"OpenVolume failure", ret);
 
-#define buffer ((CHAR16 *)keyhandler_scratch)
-#define BUFFERSIZE sizeof(keyhandler_scratch)
+#define BUFFERSIZE sizeof(buffer)
     for ( dp = loaded_image->FilePath, *buffer = 0;
           DevicePathType(dp) != END_DEVICE_PATH_TYPE;
           dp = (void *)dp + DevicePathNodeLength(dp) )
diff --git a/xen/common/keyhandler.c b/xen/common/keyhandler.c
index 27c8269..c25a30e 100644
--- a/xen/common/keyhandler.c
+++ b/xen/common/keyhandler.c
@@ -29,8 +29,6 @@ static keyhandler_fn_t show_handlers, dump_hwdom_registers,
 static irq_keyhandler_fn_t do_toggle_alt_key, dump_registers,
     reboot_machine, run_all_keyhandlers, do_debug_key;
 
-char keyhandler_scratch[1024];
-
 static struct keyhandler {
     union {
         keyhandler_fn_t *fn;
@@ -250,25 +248,11 @@ static void reboot_machine(unsigned char key, struct cpu_user_regs *regs)
     machine_restart(0);
 }
 
-static void periodic_timer_print(char *str, int size, uint64_t period)
-{
-    if ( period == 0 )
-    {
-        strlcpy(str, "No periodic timer", size);
-        return;
-    }
-
-    snprintf(str, size,
-             "%u Hz periodic timer (period %u ms)",
-             1000000000/(int)period, (int)period/1000000);
-}
-
 static void dump_domains(unsigned char key)
 {
     struct domain *d;
     struct vcpu   *v;
     s_time_t       now = NOW();
-#define tmpstr keyhandler_scratch
 
     printk("'%c' pressed -> dumping domain info (now = %"PRI_stime")\n",
            key, now);
@@ -333,8 +317,13 @@ static void dump_domains(unsigned char key)
             printk("    pause_count=%d pause_flags=%lx\n",
                    atomic_read(&v->pause_count), v->pause_flags);
             arch_dump_vcpu_info(v);
-            periodic_timer_print(tmpstr, sizeof(tmpstr), v->periodic_period);
-            printk("    %s\n", tmpstr);
+
+            if ( v->periodic_period == 0 )
+                printk("No periodic timer\n");
+            else
+                printk("%"PRI_stime" Hz periodic timer (period %"PRI_stime" ms)\n",
+                       1000000000 / v->periodic_period,
+                       v->periodic_period / 1000000);
         }
     }
 
@@ -355,7 +344,6 @@ static void dump_domains(unsigned char key)
     arch_dump_shared_mem_info();
 
     rcu_read_unlock(&domlist_read_lock);
-#undef tmpstr
 }
 
 static cpumask_t read_clocks_cpumask;
diff --git a/xen/include/xen/keyhandler.h b/xen/include/xen/keyhandler.h
index 06c05c8..5131e86 100644
--- a/xen/include/xen/keyhandler.h
+++ b/xen/include/xen/keyhandler.h
@@ -48,7 +48,4 @@ void register_irq_keyhandler(unsigned char key,
 /* Inject a keypress into the key-handling subsystem. */
 extern void handle_keypress(unsigned char key, struct cpu_user_regs *regs);
 
-/* Scratch space is available for use of any keyhandler. */
-extern char keyhandler_scratch[1024];
-
 #endif /* __XEN_KEYHANDLER_H__ */
-- 
2.1.4


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

Re: [PATCH v3 5/5] xen/keyhandler: Drop keyhandler_scratch

[Jan Beulich]

>>> On 16.11.18 at 18:45, <andrew.cooper3@citrix.com> wrote:
> With almost all users of keyhandler_scratch gone, clean up the 3 remaining
> users and drop the buffer.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Wei Liu <wei.liu2@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>



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