Re: xentrace buffer size, maxcpus and online cpus

[Olaf Hering <olaf@xxxxxxxxx>]

Fri, 16 Jun 2023 15:22:24 +0100 George Dunlap <george.dunlap@xxxxxxxxx>:

> I agree; the clear implication is that with smt=0, you might have
> num_online_cpus() return 4, but cpuids that look like {1, 3, 5, 7}; so you
> need the trace buffer array to be of size 8.

I have tested the patch below with this cmdline:
conring_size=32M loglvl=all guest_loglvl=all crashkernel=264M,below=4G
console=com1 com1=115200 dom0_mem=16G dom0_max_vcpus=8 dom0_vcpus_pin
maxcpus=10 console_timestamps=datems tbuf_size=-1 smt=0

It appears to work as expected. One downside is that xenalyze reports
each cpu it finds based on index, instead of the real smp_processor_id.
That is apparently a limitation of the interface. In the end this detail
may not matter.

I think this change should go on top of a revert of
commit 74584a367051bc0d6f4b96fd360fa7bc6538fc39.

Regarding coding style: can this_cpu and per_cpu be used as array index,
or is a temporary variable required? This would affect the number of
lines changed in next_record.


Olaf

--- a/xen/common/trace.c
+++ b/xen/common/trace.c
@@ -53,6 +53,7 @@ integer_param("tevt_mask", opt_tevt_mask);
 static struct t_info *t_info;
 static unsigned int t_info_pages;
 
+static DEFINE_PER_CPU_READ_MOSTLY(uint16_t, t_info_mfn_offset);
 static DEFINE_PER_CPU_READ_MOSTLY(struct t_buf *, t_bufs);
 static DEFINE_PER_CPU_READ_MOSTLY(spinlock_t, t_lock);
 static u32 data_size __read_mostly;
@@ -110,7 +111,8 @@ static int calculate_tbuf_size(unsigned int pages, uint16_t 
t_info_first_offset)
     struct t_info dummy_pages;
     typeof(dummy_pages.tbuf_size) max_pages;
     typeof(dummy_pages.mfn_offset[0]) max_mfn_offset;
-    unsigned int max_cpus = nr_cpu_ids;
+    unsigned int nr_cpus = num_online_cpus();
+    unsigned int max_cpus = nr_cpus;
     unsigned int t_info_words;
 
     /* force maximum value for an unsigned type */
@@ -148,11 +150,11 @@ static int calculate_tbuf_size(unsigned int pages, 
uint16_t t_info_first_offset)
      * NB this calculation is correct, because t_info_first_offset is
      * in words, not bytes
      */
-    t_info_words = nr_cpu_ids * pages + t_info_first_offset;
+    t_info_words = nr_cpus * pages + t_info_first_offset;
     t_info_pages = PFN_UP(t_info_words * sizeof(uint32_t));
     printk(XENLOG_INFO "xentrace: requesting %u t_info pages "
            "for %u trace pages on %u cpus\n",
-           t_info_pages, pages, nr_cpu_ids);
+           t_info_pages, pages, nr_cpus);
     return pages;
 }
 
@@ -173,6 +175,7 @@ static int alloc_trace_bufs(unsigned int pages)
     uint32_t *t_info_mfn_list;
     uint16_t t_info_first_offset;
     uint16_t offset;
+    uint16_t index = 0;
 
     if ( t_info )
         return -EBUSY;
@@ -201,8 +204,9 @@ static int alloc_trace_bufs(unsigned int pages)
      */
     for_each_online_cpu(cpu)
     {
-        offset = t_info_first_offset + (cpu * pages);
-        t_info->mfn_offset[cpu] = offset;
+        per_cpu(t_info_mfn_offset, cpu) = index;
+        offset = t_info_first_offset + (index * pages);
+        t_info->mfn_offset[index] = offset;
 
         for ( i = 0; i < pages; i++ )
         {
@@ -216,6 +220,7 @@ static int alloc_trace_bufs(unsigned int pages)
             }
             t_info_mfn_list[offset + i] = virt_to_mfn(p);
         }
+        index++;
     }
 
     /*
@@ -227,7 +232,8 @@ static int alloc_trace_bufs(unsigned int pages)
 
         spin_lock_init(&per_cpu(t_lock, cpu));
 
-        offset = t_info->mfn_offset[cpu];
+        index = per_cpu(t_info_mfn_offset, cpu);
+        offset = t_info->mfn_offset[index];
 
         /* Initialize the buffer metadata */
         per_cpu(t_bufs, cpu) = buf = mfn_to_virt(t_info_mfn_list[offset]);
@@ -260,7 +266,8 @@ static int alloc_trace_bufs(unsigned int pages)
 out_dealloc:
     for_each_online_cpu(cpu)
     {
-        offset = t_info->mfn_offset[cpu];
+        index = per_cpu(t_info_mfn_offset, cpu);
+        offset = t_info->mfn_offset[index];
         if ( !offset )
             continue;
         for ( i = 0; i < pages; i++ )
@@ -509,6 +516,7 @@ static unsigned char *next_record(const struct t_buf *buf, 
uint32_t *next,
     uint32_t per_cpu_mfn_nr;
     uint32_t *mfn_list;
     uint32_t mfn;
+    uint16_t index;
     unsigned char *this_page;
 
     barrier(); /* must read buf->prod and buf->cons only once */
@@ -527,7 +535,8 @@ static unsigned char *next_record(const struct t_buf *buf, 
uint32_t *next,
 
     /* offset into array of mfns */
     per_cpu_mfn_nr = x >> PAGE_SHIFT;
-    per_cpu_mfn_offset = t_info->mfn_offset[smp_processor_id()];
+    index = this_cpu(t_info_mfn_offset);
+    per_cpu_mfn_offset = t_info->mfn_offset[index];
     mfn_list = (uint32_t *)t_info;
     mfn = mfn_list[per_cpu_mfn_offset + per_cpu_mfn_nr];
     this_page = mfn_to_virt(mfn);

Attachment: pgpgm60aBasfm.pgp
Description: Digitale Signatur von OpenPGP