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

[PATCH 0/6] 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 5 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%

Andrew Cooper (6):
  xen/vsprintf: Introduce %*pb[l] for printing bitmaps
  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

 docs/misc/printk-formats.txt  |   8 ++++
 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         |   5 +-
 xen/common/event_channel.c    |   6 +--
 xen/common/keyhandler.c       |  61 +++++++-----------------
 xen/common/sched_credit.c     |  17 ++-----
 xen/common/sched_credit2.c    |  26 ++++-------
 xen/common/sched_null.c       |  15 ++----
 xen/common/sched_rt.c         |   5 +-
 xen/common/vsprintf.c         |  97 ++++++++++++++++++++++++++++++++++++++
 xen/include/xen/bitmap.h      |   6 ---
 xen/include/xen/cpumask.h     |  18 --------
 xen/include/xen/keyhandler.h  |   3 --
 xen/include/xen/nodemask.h    |  34 --------------
 19 files changed, 163 insertions(+), 289 deletions(-)

-- 
2.1.4


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

[PATCH 1/6] xen/vsprintf: Introduce %*pb[l] for printing bitmaps

[Andrew Cooper]

The format identifier is consistent with Linux.  The code is adapted from
bitmap_scn{,list}printf() but cleaned up.

This change allows all callers to avoid needing a secondary buffer to render a
cpumask/nodemask into.

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>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Dario Faggioli <dfaggioli@suse.com>
---
 docs/misc/printk-formats.txt |  8 ++++
 xen/common/vsprintf.c        | 97 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 105 insertions(+)

diff --git a/docs/misc/printk-formats.txt b/docs/misc/printk-formats.txt
index 525108f..d0e1537 100644
--- a/docs/misc/printk-formats.txt
+++ b/docs/misc/printk-formats.txt
@@ -13,6 +13,14 @@ Raw buffer as hex string:
        Up to 64 characters.  Buffer length expected via the field_width
        paramter. i.e. printk("%*ph", 8, buffer);
 
+Bitmaps (e.g. cpumask/nodemask):
+
+       %*pb    4321
+       %*pbl   0,5,8-9,14
+
+       Print a bitmap as either a hex string, or a range list.  Bitmap length
+       (in bits) expected via the field_width parameter.
+
 Symbol/Function pointers:
 
        %ps     Symbol name with condition offset and size (iff offset != 0)
diff --git a/xen/common/vsprintf.c b/xen/common/vsprintf.c
index f92fb67..1750e5e 100644
--- a/xen/common/vsprintf.c
+++ b/xen/common/vsprintf.c
@@ -264,6 +264,88 @@ static char *string(char *str, char *end, const char *s,
     return str;
 }
 
+/* Print a bitmap as '0-3,6-15' */
+static char *print_bitmap_list(char *str, char *end,
+                               const unsigned long *bitmap, int nr_bits)
+{
+    /* current bit is 'cur', most recently seen range is [rbot, rtop] */
+    int cur, rbot, rtop;
+    bool first = true;
+
+    rbot = cur = find_first_bit(bitmap, nr_bits);
+    while ( cur < nr_bits )
+    {
+        rtop = cur;
+        cur = find_next_bit(bitmap, nr_bits, cur + 1);
+
+        if ( cur < nr_bits && cur <= rtop + 1 )
+            continue;
+
+        if ( !first )
+        {
+            if ( str < end )
+                *str = ',';
+            str++;
+        }
+        first = false;
+
+        str = number(str, end, rbot, 10, -1, -1, 0);
+        if ( rbot < rtop )
+        {
+            if ( str < end )
+                *str = '-';
+            str++;
+
+            str = number(str, end, rtop, 10, -1, -1, 0);
+        }
+
+        rbot = cur;
+    }
+
+    return str;
+}
+
+/* Print a bitmap as a comma separated hex string. */
+static char *print_bitmap_string(char *str, char *end,
+                                 const unsigned long *bitmap, int nr_bits)
+{
+    const unsigned int CHUNKSZ = 32;
+    unsigned int chunksz;
+    int i;
+    bool first = true;
+
+    chunksz = nr_bits & (CHUNKSZ - 1);
+    if ( chunksz == 0 )
+        chunksz = CHUNKSZ;
+
+    /*
+     * First iteration copes with the trailing partial word if nr_bits isn't a
+     * round multiple of CHUNKSZ.  All subsequent iterations work on a
+     * complete CHUNKSZ block.
+     */
+    for ( i = ROUNDUP(nr_bits, CHUNKSZ) - CHUNKSZ; i >= 0; i -= CHUNKSZ )
+    {
+        unsigned int chunkmask = (1ull << chunksz) - 1;
+        unsigned int word   = i / BITS_PER_LONG;
+        unsigned int offset = i % BITS_PER_LONG;
+        unsigned long val = (bitmap[word] >> offset) & chunkmask;
+
+        if ( !first )
+        {
+            if ( str < end )
+                *str = ',';
+            str++;
+        }
+        first = false;
+
+        str = number(str, end, val, 16, DIV_ROUND_UP(chunksz, 4), -1, ZEROPAD);
+
+        chunksz = CHUNKSZ;
+    }
+
+    return str;
+}
+
 static char *pointer(char *str, char *end, const char **fmt_ptr,
                      const void *arg, int field_width, int precision,
                      int flags)
@@ -273,6 +355,21 @@ static char *pointer(char *str, char *end, const char **fmt_ptr,
     /* Custom %p suffixes. See XEN_ROOT/docs/misc/printk-formats.txt */
     switch ( fmt[1] )
     {
+    case 'b': /* Bitmap as hex, or list */
+        ++*fmt_ptr;
+
+        if ( field_width < 0 )
+            return str;
+
+        if ( fmt[2] == 'l' )
+        {
+            ++*fmt_ptr;
+
+            return print_bitmap_list(str, end, arg, field_width);
+        }
+
+        return print_bitmap_string(str, end, arg, field_width);
+
     case 'h': /* Raw buffer as hex string. */
     {
         const uint8_t *hex_buffer = arg;
-- 
2.1.4


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

[PATCH 2/6] 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>
---
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: 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>
---
 xen/common/sched_credit.c  | 17 +++++------------
 xen/common/sched_credit2.c | 26 +++++++++-----------------
 xen/common/sched_null.c    | 15 +++++----------
 xen/common/sched_rt.c      |  5 ++---
 4 files changed, 21 insertions(+), 42 deletions(-)

diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
index 84e744b..fe48e1b 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),
+           nr_cpu_ids, per_cpu(cpu_core_mask, cpu));
 
     /* 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);
 
     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 7438481..1df9d5f 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -3650,12 +3650,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),
+           nr_cpu_ids, per_cpu(cpu_core_mask, cpu));
 
     /* current VCPU (nothing to say if that's the idle vcpu) */
     svc = csched2_vcpu(curr_on_cpu(cpu));
@@ -3664,7 +3663,6 @@ dump_pcpu(const struct scheduler *ops, int cpu)
         printk("\trun: ");
         csched2_dump_vcpu(prv, svc);
     }
-#undef cpustr
 }
 
 static void
@@ -3674,7 +3672,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
@@ -3692,29 +3689,25 @@ 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,
                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);
+        printk("\ttickled: %*pb\n", nr_cpu_ids, &prv->rqd[i].tickled);
+        printk("\tfully idle cores: %*pb\n", nr_cpu_ids, &prv->rqd[i].smt_idle);
     }
 
     printk("Domain info:\n");
@@ -3775,7 +3768,6 @@ csched2_dump(const struct scheduler *ops)
     }
 
     read_unlock_irqrestore(&prv->lock, flags);
-#undef cpustr
 }
 
 /* Returns the ID of the runqueue the cpu is assigned to. */
diff --git a/xen/common/sched_null.c b/xen/common/sched_null.c
index 784db71..c2509ff 100644
--- a/xen/common/sched_null.c
+++ b/xen/common/sched_null.c
@@ -795,14 +795,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),
+           nr_cpu_ids, per_cpu(cpu_core_mask, cpu));
     if ( per_cpu(npc, cpu).vcpu != NULL )
         printk(", vcpu=d%dv%d", per_cpu(npc, cpu).vcpu->domain->domain_id,
                per_cpu(npc, cpu).vcpu->vcpu_id);
@@ -818,7 +817,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)
@@ -827,12 +825,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);
 
     printk("Domain info:\n");
     loop = 0;
@@ -876,7 +872,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 ac79f15..9784213 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);
 }
 
 static void
-- 
2.1.4


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

[PATCH 3/6] 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>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
CC: Juergen Gross <jgross@suse.com>
---
 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..16ca4c4 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);
     if ( !cpumask_empty(&cpupool_free_cpus) )
     {
-        print_cpumap("Free Cpus", &cpupool_free_cpus);
+        printk("Free Cpus: %*pbl\n", nr_cpu_ids, &cpupool_free_cpus);
         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);
         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 777c8e9..93ae738 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,
+               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);
 
         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,
+                   nr_cpu_ids, v->cpu_soft_affinity);
             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 4/6] 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>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 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 32273d9..a860693 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 %*pd",
+                     nr_cpu_ids,  &mce_fatal_cpus);
+
             mc_panic(ebuf);
         }
         atomic_set(&found_error, 0);
diff --git a/xen/arch/x86/crash.c b/xen/arch/x86/crash.c
index 8d74258..d4b83ad 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);
 
     /* 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 ec93ab6..ca51320 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, 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 5/6] 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>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.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 RFC 6/6] 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>

The NUMA and periodic timer adjustments are definitely improvements.  However,
this is RFC because of the EFI change, which might perhaps not wan to be done
this way.  Suggestions welcome.
---
 xen/arch/x86/numa.c          | 11 ++++-------
 xen/common/efi/boot.c        |  5 ++---
 xen/common/keyhandler.c      | 26 +++++++-------------------
 xen/include/xen/keyhandler.h |  3 ---
 4 files changed, 13 insertions(+), 32 deletions(-)

diff --git a/xen/arch/x86/numa.c b/xen/arch/x86/numa.c
index a87987d..de13aca 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..aa9a666 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -489,7 +489,7 @@ static EFI_FILE_HANDLE __init get_parent_handle(EFI_LOADED_IMAGE *loaded_image,
     static EFI_GUID __initdata fs_protocol = SIMPLE_FILE_SYSTEM_PROTOCOL;
     EFI_FILE_HANDLE dir_handle;
     EFI_DEVICE_PATH *dp;
-    CHAR16 *pathend, *ptr;
+    CHAR16 *pathend, *ptr, buffer[128];
     EFI_STATUS ret;
 
     do {
@@ -506,8 +506,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 93ae738..64e5783 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=0x%X:%08X)\n", key,
            (u32)(now>>32), (u32)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("%u Hz periodic timer (period %u ms)\n",
+                       1000000000/(int)v->periodic_period,
+                       (int)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 RFC 6/6] xen/keyhandler: Drop keyhandler_scratch

[Wei Liu]

On Thu, Sep 06, 2018 at 01:08:16PM +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>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> 
> The NUMA and periodic timer adjustments are definitely improvements.  However,
> this is RFC because of the EFI change, which might perhaps not wan to be done
> this way.  Suggestions welcome.

Why would the EFI change be a problem? Using keyhandler_scratch as a
generic buffer in EFI code seems wrong to me anyway.

Wei.

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

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

[Wei Liu]

On Thu, Sep 06, 2018 at 01:08:13PM +0100, Andrew Cooper 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>

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> CC: Juergen Gross <jgross@suse.com>
> ---
>  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..16ca4c4 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);
>      if ( !cpumask_empty(&cpupool_free_cpus) )
>      {
> -        print_cpumap("Free Cpus", &cpupool_free_cpus);
> +        printk("Free Cpus: %*pbl\n", nr_cpu_ids, &cpupool_free_cpus);
>          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);
>          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 777c8e9..93ae738 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,
> +               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);
>  
>          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,
> +                   nr_cpu_ids, v->cpu_soft_affinity);
>              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

Re: [PATCH 1/6] xen/vsprintf: Introduce %*pb[l] for printing bitmaps

[Wei Liu]

On Thu, Sep 06, 2018 at 01:08:11PM +0100, Andrew Cooper wrote:
> The format identifier is consistent with Linux.  The code is adapted from
> bitmap_scn{,list}printf() but cleaned up.
> 
> This change allows all callers to avoid needing a secondary buffer to render a
> cpumask/nodemask into.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

I only skim-read this patch, since this is mostly moved / copied code.

Wei.

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

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

[Wei Liu]

On Thu, Sep 06, 2018 at 01:08:14PM +0100, Andrew Cooper 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>

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

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

[Wei Liu]

On Thu, Sep 06, 2018 at 01:08:15PM +0100, Andrew Cooper wrote:
> 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>

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

Re: [PATCH 1/6] xen/vsprintf: Introduce %*pb[l] for printing bitmaps

[Jan Beulich]

>>> On 06.09.18 at 14:08, <andrew.cooper3@citrix.com> wrote:
> The format identifier is consistent with Linux.  The code is adapted from
> bitmap_scn{,list}printf() but cleaned up.

Irrespective of this I'm somewhat worried by ...

> --- a/docs/misc/printk-formats.txt
> +++ b/docs/misc/printk-formats.txt
> @@ -13,6 +13,14 @@ Raw buffer as hex string:
>         Up to 64 characters.  Buffer length expected via the field_width
>         paramter. i.e. printk("%*ph", 8, buffer);
>  
> +Bitmaps (e.g. cpumask/nodemask):
> +
> +       %*pb    4321
> +       %*pbl   0,5,8-9,14
> +
> +       Print a bitmap as either a hex string, or a range list.  Bitmap length
> +       (in bits) expected via the field_width parameter.

... the l suffix here. It's not very likely that someone might mean to
follow %pb by l, but it's syntactically ambiguous. Since the 'l' qualifier
is so far meaningless for %p, why can't we use that instead, making
usages look like %*lpb?

> --- a/xen/common/vsprintf.c
> +++ b/xen/common/vsprintf.c
> @@ -264,6 +264,88 @@ static char *string(char *str, char *end, const char 
> *s,
>      return str;
>  }
>  
> +/* Print a bitmap as '0-3,6-15' */
> +static char *print_bitmap_list(char *str, char *end,
> +                               const unsigned long *bitmap, int nr_bits)
> +{
> +    /* current bit is 'cur', most recently seen range is [rbot, rtop] */
> +    int cur, rbot, rtop;

Including the nr_bits parameter - which of these really have to be
plain (i.e. signed) int?

> +/* Print a bitmap as a comma separated hex string. */
> +static char *print_bitmap_string(char *str, char *end,
> +                                 const unsigned long *bitmap, int nr_bits)
> +{
> +    const unsigned int CHUNKSZ = 32;
> +    unsigned int chunksz;
> +    int i;

Same question here, despite ...

> +    bool first = true;
> +
> +    chunksz = nr_bits & (CHUNKSZ - 1);
> +    if ( chunksz == 0 )
> +        chunksz = CHUNKSZ;
> +
> +    /*
> +     * First iteration copes with the trailing partial word if nr_bits isn't a
> +     * round multiple of CHUNKSZ.  All subsequent iterations work on a
> +     * complete CHUNKSZ block.
> +     */
> +    for ( i = ROUNDUP(nr_bits, CHUNKSZ) - CHUNKSZ; i >= 0; i -= CHUNKSZ )

... this, which obviously would need adjustment if changed
(and where hence it is at least worthwhile to consider leaving
it the way it is).


> @@ -273,6 +355,21 @@ static char *pointer(char *str, char *end, const char **fmt_ptr,
>      /* Custom %p suffixes. See XEN_ROOT/docs/misc/printk-formats.txt */
>      switch ( fmt[1] )
>      {
> +    case 'b': /* Bitmap as hex, or list */
> +        ++*fmt_ptr;
> +
> +        if ( field_width < 0 )
> +            return str;
> +
> +        if ( fmt[2] == 'l' )
> +        {
> +            ++*fmt_ptr;

With the suffixing approach an anomaly will result here when the
"field_width < 0" path is taken above, leaving *fmt_ptr point at
the 'l'.

Jan



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

Re: [PATCH 2/6] xen/sched: Use %*pb[l] instead of cpumask_scn{, list}printf()

[Jan Beulich]

>>> On 06.09.18 at 14:08, <andrew.cooper3@citrix.com> wrote:
> @@ -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),
> +           nr_cpu_ids, per_cpu(cpu_core_mask, cpu));

Strictly speaking here and elsewhere you should wrap the CPU mask
accesses in cpumask_bits(). Then again I wonder whether a special
case for CPU masks wouldn't be warranted, making it unnecessary for
callers to pass in nr_cpu_ids explicitly.

Jan



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

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

[Jan Beulich]

>>> On 06.09.18 at 14:08, <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>

Subject to possible adjustments resulting from comments to earlier
patches
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan



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

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

[Jan Beulich]

>>> On 06.09.18 at 14:08, <andrew.cooper3@citrix.com> wrote:
> --- 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 %*pd",

DYM %*pb here? With this corrected
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan



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

Re: [PATCH RFC 6/6] xen/keyhandler: Drop keyhandler_scratch

[Jan Beulich]

>>> On 06.09.18 at 14:08, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -489,7 +489,7 @@ static EFI_FILE_HANDLE __init get_parent_handle(EFI_LOADED_IMAGE *loaded_image,
>      static EFI_GUID __initdata fs_protocol = SIMPLE_FILE_SYSTEM_PROTOCOL;
>      EFI_FILE_HANDLE dir_handle;
>      EFI_DEVICE_PATH *dp;
> -    CHAR16 *pathend, *ptr;
> +    CHAR16 *pathend, *ptr, buffer[128];
>      EFI_STATUS ret;
>  
>      do {
> @@ -506,8 +506,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) )

I don't mind a change like this at all - it just was convenient to use
an available static buffer. I'm not overly happy about the large stack
item - did you consider using a static __initdata object instead? We
could then also consider growing it further, as I'm not sure 128 is
sufficient in the general case (but then again I'm also not sure
whether FAT32 has an upper bound on file name length - it's been
too long ago that I last had to play with related code).

> @@ -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("%u Hz periodic timer (period %u ms)\n",
> +                       1000000000/(int)v->periodic_period,
> +                       (int)v->periodic_period/1000000);

Blanks around / please, and I can't see why the casts would be
needed either (in fact ).

Jan



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

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

[Juergen Gross]

On 06/09/18 14:08, Andrew Cooper 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>

Acked-by: Juergen Gross <jgross@suse.com>


Juergen

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

Re: [PATCH 1/6] xen/vsprintf: Introduce %*pb[l] for printing bitmaps

[Andrew Cooper]

On 07/09/18 08:41, Jan Beulich wrote:
>>>> On 06.09.18 at 14:08, <andrew.cooper3@citrix.com> wrote:
>> The format identifier is consistent with Linux.  The code is adapted from
>> bitmap_scn{,list}printf() but cleaned up.
> Irrespective of this I'm somewhat worried by ...
>
>> --- a/docs/misc/printk-formats.txt
>> +++ b/docs/misc/printk-formats.txt
>> @@ -13,6 +13,14 @@ Raw buffer as hex string:
>>         Up to 64 characters.  Buffer length expected via the field_width
>>         paramter. i.e. printk("%*ph", 8, buffer);
>>  
>> +Bitmaps (e.g. cpumask/nodemask):
>> +
>> +       %*pb    4321
>> +       %*pbl   0,5,8-9,14
>> +
>> +       Print a bitmap as either a hex string, or a range list.  Bitmap length
>> +       (in bits) expected via the field_width parameter.
> ... the l suffix here. It's not very likely that someone might mean to
> follow %pb by l, but it's syntactically ambiguous.

I don't see anything ambiguous here.  The l is for list, not for long,
and trailing modifiers are consistent with all the other %p infrastructure.

> Since the 'l' qualifier
> is so far meaningless for %p, why can't we use that instead, making
> usages look like %*lpb?

First and foremost, diverging from Linux's well-documented and well-used
API not something we should do without a very very good reason. 
Irrespective of whether you think it is ambiguous or not, I don't view
this as a good enough (potential) issue to diverge.

Furthermore, (and more likely to sway your opinion), N1570 indicates
that the 'l' length modifier is only applicable for the diouxXcs
conversion specifiers, and both Clang and GCC enforce this with -Wformat.

andrewcoop@andrewcoop:/local/xen.git/xen$ clang-6.0 -Wall -Werror -Wextra foo.c -o foo.o
foo.c:7:22: error: length modifier 'l' results in undefined behavior or no effect with 'p' conversion specifier [-Werror,-Wformat]
    printf("Testing %lpd\n", ptr);
                    ~^~
1 error generated.

andrewcoop@andrewcoop:/local/xen.git/xen$ gcc -Wall -Werror -Wextra foo.c -o foo.o
foo.c: In function ‘bar’:
foo.c:7:5: error: use of ‘l’ length modifier with ‘p’ type character [-Werror=format=]
     printf("Testing %lpd\n", ptr);
     ^
cc1: all warnings being treated as errors



>
>> --- a/xen/common/vsprintf.c
>> +++ b/xen/common/vsprintf.c
>> @@ -264,6 +264,88 @@ static char *string(char *str, char *end, const char 
>> *s,
>>      return str;
>>  }
>>  
>> +/* Print a bitmap as '0-3,6-15' */
>> +static char *print_bitmap_list(char *str, char *end,
>> +                               const unsigned long *bitmap, int nr_bits)
>> +{
>> +    /* current bit is 'cur', most recently seen range is [rbot, rtop] */
>> +    int cur, rbot, rtop;
> Including the nr_bits parameter - which of these really have to be
> plain (i.e. signed) int?

Hmm - overall, the bitmap API is a mix and match of signed-ness, both
for nr_bits, and the return value bit positions.

I think these probably can switch, while..

>
>> +/* Print a bitmap as a comma separated hex string. */
>> +static char *print_bitmap_string(char *str, char *end,
>> +                                 const unsigned long *bitmap, int nr_bits)
>> +{
>> +    const unsigned int CHUNKSZ = 32;
>> +    unsigned int chunksz;
>> +    int i;
> Same question here, despite ...
>
>> +    bool first = true;
>> +
>> +    chunksz = nr_bits & (CHUNKSZ - 1);
>> +    if ( chunksz == 0 )
>> +        chunksz = CHUNKSZ;
>> +
>> +    /*
>> +     * First iteration copes with the trailing partial word if nr_bits isn't a
>> +     * round multiple of CHUNKSZ.  All subsequent iterations work on a
>> +     * complete CHUNKSZ block.
>> +     */
>> +    for ( i = ROUNDUP(nr_bits, CHUNKSZ) - CHUNKSZ; i >= 0; i -= CHUNKSZ )
> ... this, which obviously would need adjustment if changed
> (and where hence it is at least worthwhile to consider leaving
> it the way it is).

... this should stay as it is because its by far the cleanest way of
expressing the logic.

~Andrew

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

Re: [PATCH 2/6] xen/sched: Use %*pb[l] instead of cpumask_scn{, list}printf()

[Andrew Cooper]

On 07/09/18 09:03, Jan Beulich wrote:
>>>> On 06.09.18 at 14:08, <andrew.cooper3@citrix.com> wrote:
>> @@ -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),
>> +           nr_cpu_ids, per_cpu(cpu_core_mask, cpu));
> Strictly speaking here and elsewhere you should wrap the CPU mask
> accesses in cpumask_bits().

Why? Its barely used, and is another example of a helper which only adds
to code volume.

> Then again I wonder whether a special
> case for CPU masks wouldn't be warranted, making it unnecessary for
> callers to pass in nr_cpu_ids explicitly.

The only way of special casing is to have a different custom %p
formatter.  All printing of cpu and nodemasks are in keyhandlers so I
don't think a custom case is going to be worth it.

~Andrew

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

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

[Andrew Cooper]

On 07/09/18 09:12, Jan Beulich wrote:
>>>> On 06.09.18 at 14:08, <andrew.cooper3@citrix.com> wrote:
>> --- 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 %*pd",
> DYM %*pb here?

Oops yes.

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

Thanks,

~Andrew

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

Re: [PATCH 2/6] xen/sched: Use %*pb[l] instead of cpumask_scn{, list}printf()

[George Dunlap]

On 09/07/2018 02:56 PM, Andrew Cooper wrote:
>> Then again I wonder whether a special
>> case for CPU masks wouldn't be warranted, making it unnecessary for
>> callers to pass in nr_cpu_ids explicitly.
> 
> The only way of special casing is to have a different custom %p
> formatter.  All printing of cpu and nodemasks are in keyhandlers so I
> don't think a custom case is going to be worth it.

I wouldn't have an objection to someone implementing a cpumask-specific
formatter at some point, but I don't think Andy should be required to
write it.

 -George

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

Re: [PATCH 2/6] xen/sched: Use %*pb[l] instead of cpumask_scn{, list}printf()

[George Dunlap]

On 09/06/2018 01:08 PM, Andrew Cooper 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>

Acked-by: George Dunlap <george.dunlap@citrix.com>

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

Re: [PATCH 1/6] xen/vsprintf: Introduce %*pb[l] for printing bitmaps

[Jan Beulich]

>>> On 07.09.18 at 15:01, <andrew.cooper3@citrix.com> wrote:
> On 07/09/18 08:41, Jan Beulich wrote:
>>>>> On 06.09.18 at 14:08, <andrew.cooper3@citrix.com> wrote:
>>> The format identifier is consistent with Linux.  The code is adapted from
>>> bitmap_scn{,list}printf() but cleaned up.
>> Irrespective of this I'm somewhat worried by ...
>>
>>> --- a/docs/misc/printk-formats.txt
>>> +++ b/docs/misc/printk-formats.txt
>>> @@ -13,6 +13,14 @@ Raw buffer as hex string:
>>>         Up to 64 characters.  Buffer length expected via the field_width
>>>         paramter. i.e. printk("%*ph", 8, buffer);
>>>  
>>> +Bitmaps (e.g. cpumask/nodemask):
>>> +
>>> +       %*pb    4321
>>> +       %*pbl   0,5,8-9,14
>>> +
>>> +       Print a bitmap as either a hex string, or a range list.  Bitmap length
>>> +       (in bits) expected via the field_width parameter.
>> ... the l suffix here. It's not very likely that someone might mean to
>> follow %pb by l, but it's syntactically ambiguous.
> 
> I don't see anything ambiguous here.  The l is for list, not for long,
> and trailing modifiers are consistent with all the other %p infrastructure.

Well, I can accept the single suffix char as a good and useful extension.
I don't, however, think that making the suffixes arbitrarily long is too
good an idea.

>> Since the 'l' qualifier
>> is so far meaningless for %p, why can't we use that instead, making
>> usages look like %*lpb?
> 
> First and foremost, diverging from Linux's well-documented and well-used
> API not something we should do without a very very good reason. 

I'd drop the "very very", but then I agree. Yet we shouldn't slavishly
follow what they do, when it's questionable whether the direction is
indeed right. Hence my response here.

> Irrespective of whether you think it is ambiguous or not, I don't view
> this as a good enough (potential) issue to diverge.
> 
> Furthermore, (and more likely to sway your opinion), N1570 indicates
> that the 'l' length modifier is only applicable for the diouxXcs
> conversion specifiers, and both Clang and GCC enforce this with -Wformat.
> 
> andrewcoop@andrewcoop:/local/xen.git/xen$ clang-6.0 -Wall -Werror -Wextra 
> foo.c -o foo.o
> foo.c:7:22: error: length modifier 'l' results in undefined behavior or no 
> effect with 'p' conversion specifier [-Werror,-Wformat]
>     printf("Testing %lpd\n", ptr);
>                     ~^~
> 1 error generated.

Yeah, I started to be concerned of this happening after I had sent
the reply. Given this I guess we have no (good) choice besides going
the suffix route.

Jan



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

Re: [PATCH 2/6] xen/sched: Use %*pb[l] instead of cpumask_scn{, list}printf()

[Jan Beulich]

>>> On 07.09.18 at 15:56, <andrew.cooper3@citrix.com> wrote:
> On 07/09/18 09:03, Jan Beulich wrote:
>>>>> On 06.09.18 at 14:08, <andrew.cooper3@citrix.com> wrote:
>>> @@ -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),
>>> +           nr_cpu_ids, per_cpu(cpu_core_mask, cpu));
>> Strictly speaking here and elsewhere you should wrap the CPU mask
>> accesses in cpumask_bits().
> 
> Why? Its barely used, and is another example of a helper which only adds
> to code volume.

If anyone added (e.g. for debugging) a leading field to struct cpumask,
your code would break, while all code anywhere else would still be fine.

>> Then again I wonder whether a special
>> case for CPU masks wouldn't be warranted, making it unnecessary for
>> callers to pass in nr_cpu_ids explicitly.
> 
> The only way of special casing is to have a different custom %p
> formatter.  All printing of cpu and nodemasks are in keyhandlers so I
> don't think a custom case is going to be worth it.

Well - it's worth a consideration, because the requirement on the
paired nr_cpu_ids is not overly nice / transparent. Then again I've
intentionally said "I wonder" - I'm in no way meaning to insist.

Jan



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

Re: [PATCH 2/6] xen/sched: Use %*pb[l] instead of cpumask_scn{, list}printf()

[George Dunlap]

On 09/07/2018 04:17 PM, Jan Beulich wrote:
>>>> On 07.09.18 at 15:56, <andrew.cooper3@citrix.com> wrote:
>> On 07/09/18 09:03, Jan Beulich wrote:
>>>>>> On 06.09.18 at 14:08, <andrew.cooper3@citrix.com> wrote:
>>>> @@ -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),
>>>> +           nr_cpu_ids, per_cpu(cpu_core_mask, cpu));
>>> Strictly speaking here and elsewhere you should wrap the CPU mask
>>> accesses in cpumask_bits().
>>
>> Why? Its barely used, and is another example of a helper which only adds
>> to code volume.
> 
> If anyone added (e.g. for debugging) a leading field to struct cpumask,
> your code would break, while all code anywhere else would still be fine.

Do all other current users use cpumask_bits() for dereferencing?

I took Andy's "Its[sic] barely used" comment to mean there were lots of
other places which also just passed a cpumask_t pointer directly into
something expecting a bitmap.  If all other use cases either use
cpumask_bits() or ->bits, then we should do the same here.  If there are
lots of places where we assume (void *)mask == (void *)mask->bits, then
we should probably document that the structure should match that (and
maybe add a BUILD_BUG_ON() if we can manage it).

 -George

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

Re: [PATCH 2/6] xen/sched: Use %*pb[l] instead of cpumask_scn{, list}printf()

[Jan Beulich]

>>> On 07.09.18 at 17:35, <george.dunlap@citrix.com> wrote:
> On 09/07/2018 04:17 PM, Jan Beulich wrote:
>>>>> On 07.09.18 at 15:56, <andrew.cooper3@citrix.com> wrote:
>>> On 07/09/18 09:03, Jan Beulich wrote:
>>>>>>> On 06.09.18 at 14:08, <andrew.cooper3@citrix.com> wrote:
>>>>> @@ -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),
>>>>> +           nr_cpu_ids, per_cpu(cpu_core_mask, cpu));
>>>> Strictly speaking here and elsewhere you should wrap the CPU mask
>>>> accesses in cpumask_bits().
>>>
>>> Why? Its barely used, and is another example of a helper which only adds
>>> to code volume.
>> 
>> If anyone added (e.g. for debugging) a leading field to struct cpumask,
>> your code would break, while all code anywhere else would still be fine.
> 
> Do all other current users use cpumask_bits() for dereferencing?
> 
> I took Andy's "Its[sic] barely used" comment to mean there were lots of
> other places which also just passed a cpumask_t pointer directly into
> something expecting a bitmap.  If all other use cases either use
> cpumask_bits() or ->bits, then we should do the same here.  If there are
> lots of places where we assume (void *)mask == (void *)mask->bits, then
> we should probably document that the structure should match that (and
> maybe add a BUILD_BUG_ON() if we can manage it).

I'm unaware of places which don't go through ->bits.

Jan



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

Re: [PATCH 2/6] xen/sched: Use %*pb[l] instead of cpumask_scn{, list}printf()

[Andrew Cooper]

On 07/09/18 16:53, Jan Beulich wrote:
>>>> On 07.09.18 at 17:35, <george.dunlap@citrix.com> wrote:
>> On 09/07/2018 04:17 PM, Jan Beulich wrote:
>>>>>> On 07.09.18 at 15:56, <andrew.cooper3@citrix.com> wrote:
>>>> On 07/09/18 09:03, Jan Beulich wrote:
>>>>>>>> On 06.09.18 at 14:08, <andrew.cooper3@citrix.com> wrote:
>>>>>> @@ -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),
>>>>>> +           nr_cpu_ids, per_cpu(cpu_core_mask, cpu));
>>>>> Strictly speaking here and elsewhere you should wrap the CPU mask
>>>>> accesses in cpumask_bits().
>>>> Why? Its barely used, and is another example of a helper which only adds
>>>> to code volume.
>>> If anyone added (e.g. for debugging) a leading field to struct cpumask,
>>> your code would break, while all code anywhere else would still be fine.
>> Do all other current users use cpumask_bits() for dereferencing?
>>
>> I took Andy's "Its[sic] barely used" comment to mean there were lots of
>> other places which also just passed a cpumask_t pointer directly into
>> something expecting a bitmap.  If all other use cases either use
>> cpumask_bits() or ->bits, then we should do the same here.  If there are
>> lots of places where we assume (void *)mask == (void *)mask->bits, then
>> we should probably document that the structure should match that (and
>> maybe add a BUILD_BUG_ON() if we can manage it).
> I'm unaware of places which don't go through ->bits.

All the printing, seeing as I didn't hit a single cpumask_bits() in this
series.

The cpumask infrastructure itself uses ->bits, which is less verbose
than the helper.

I don't think we need to go as far as having a BUILD_BUG_ON(), because I
don't expect that the layout of a cpumask would change, even for
debugging, but I also don't see the point in keeping cpumask_bits() when
almost nothing uses it.

~Andrew

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

Re: [PATCH 2/6] xen/sched: Use %*pb[l] instead of cpumask_scn{, list}printf()

[Jan Beulich]

>>> Andrew Cooper <andrew.cooper3@citrix.com> 09/07/18 6:08 PM >>>
>On 07/09/18 16:53, Jan Beulich wrote:
>>>>> On 07.09.18 at 17:35, <george.dunlap@citrix.com> wrote:
>>> I took Andy's "Its[sic] barely used" comment to mean there were lots of
>>> other places which also just passed a cpumask_t pointer directly into
>>> something expecting a bitmap.  If all other use cases either use
>>> cpumask_bits() or ->bits, then we should do the same here.  If there are
>>> lots of places where we assume (void *)mask == (void *)mask->bits, then
>>> we should probably document that the structure should match that (and
>>> maybe add a BUILD_BUG_ON() if we can manage it).
>> I'm unaware of places which don't go through ->bits.
>
>All the printing, seeing as I didn't hit a single cpumask_bits() in this
>series.

That's all because of ...


>The cpumask infrastructure itself uses ->bits, which is less verbose
>than the helper.

... it's part of the cpumask implementation. The helper really is (imo) for
code that's not part of the cpumask infrastructure, i.e. in particular cases
like the ones you add.


Jan


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

Re: [PATCH 2/6] xen/sched: Use %*pb[l] instead of cpumask_scn{, list}printf()

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1053 bytes --]

On Fri, 2018-09-07 at 15:42 +0100, George Dunlap wrote:
> On 09/06/2018 01:08 PM, Andrew Cooper 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>
> 
> Acked-by: George Dunlap <george.dunlap@citrix.com>
>
Acked-by: Dario Faggioli <dfaggioli@suse.com>

As it's a clear improvement wrt current state of things, IMO.

I agree with Jan that having to remember to always pass nr_cpu_ids is
unideal.

I'm fine with adding trying to find a solution to this to my todo-list
(although, not on the top of it, probably :-D ).

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Software Engineer @ SUSE https://www.suse.com/

[-- 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

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

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 637 bytes --]

On Thu, 2018-09-06 at 13:08 +0100, Andrew Cooper wrote:
> 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>
>
Reviewed-by: Dario Faggioli <dfaggioli@suse.com>

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Software Engineer @ SUSE https://www.suse.com/

[-- 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

Re: [PATCH 1/6] xen/vsprintf: Introduce %*pb[l] for printing bitmaps

[Andrew Cooper]

On 07/09/18 16:14, Jan Beulich wrote:
>> Irrespective of whether you think it is ambiguous or not, I don't view
>> this as a good enough (potential) issue to diverge.
>>
>> Furthermore, (and more likely to sway your opinion), N1570 indicates
>> that the 'l' length modifier is only applicable for the diouxXcs
>> conversion specifiers, and both Clang and GCC enforce this with -Wformat.
>>
>> andrewcoop@andrewcoop:/local/xen.git/xen$ clang-6.0 -Wall -Werror -Wextra 
>> foo.c -o foo.o
>> foo.c:7:22: error: length modifier 'l' results in undefined behavior or no 
>> effect with 'p' conversion specifier [-Werror,-Wformat]
>>     printf("Testing %lpd\n", ptr);
>>                     ~^~
>> 1 error generated.
> Yeah, I started to be concerned of this happening after I had sent
> the reply. Given this I guess we have no (good) choice besides going
> the suffix route.

So can I take this as at least an ack?  Currently this series is stalled.

~Andrew

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

Re: [PATCH 1/6] xen/vsprintf: Introduce %*pb[l] for printing bitmaps

[Jan Beulich]

>>> On 25.09.18 at 15:06, <andrew.cooper3@citrix.com> wrote:
> On 07/09/18 16:14, Jan Beulich wrote:
>>> Irrespective of whether you think it is ambiguous or not, I don't view
>>> this as a good enough (potential) issue to diverge.
>>>
>>> Furthermore, (and more likely to sway your opinion), N1570 indicates
>>> that the 'l' length modifier is only applicable for the diouxXcs
>>> conversion specifiers, and both Clang and GCC enforce this with -Wformat.
>>>
>>> andrewcoop@andrewcoop:/local/xen.git/xen$ clang-6.0 -Wall -Werror -Wextra 
>>> foo.c -o foo.o
>>> foo.c:7:22: error: length modifier 'l' results in undefined behavior or no 
>>> effect with 'p' conversion specifier [-Werror,-Wformat]
>>>     printf("Testing %lpd\n", ptr);
>>>                     ~^~
>>> 1 error generated.
>> Yeah, I started to be concerned of this happening after I had sent
>> the reply. Given this I guess we have no (good) choice besides going
>> the suffix route.
> 
> So can I take this as at least an ack?  Currently this series is stalled.

With at least the bug addressed that I had pointed out in the first
reply, and preferably with the int -> unsigned int conversion done
where suitable, yes.

For later patches, where applicable, I insist on using cpumask_bits()
outside of cpumask.h, though.

Jan



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