xentrace buffer size, maxcpus and online cpus

xentrace buffer size, maxcpus and online cpus

[Olaf Hering]

[-- Attachment #1: Type: text/plain, Size: 362 bytes --]

While looking again through calculate_tbuf_size after a very long time,
I was wondering why the code uses nr_cpu_ids instead of num_online_cpus.
In case Xen was booted with maxcpus=N, would it be safe to use N as
upper limit? I think this would increase the per-cpu buffer size for
each active pcpu, and as a result more events could be captured.


Olaf

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: xentrace buffer size, maxcpus and online cpus

[Jan Beulich]

On 30.05.2023 09:58, Olaf Hering wrote:
> While looking again through calculate_tbuf_size after a very long time,
> I was wondering why the code uses nr_cpu_ids instead of num_online_cpus.
> In case Xen was booted with maxcpus=N, would it be safe to use N as
> upper limit? I think this would increase the per-cpu buffer size for
> each active pcpu, and as a result more events could be captured.

Using this N would be correct afaict, but that N isn't num_online_cpus().
CPUs may have been offlined by the time trace buffers are initialized, so
without looking too closely I think it would be num_present_cpus() that
you're after.

Jan

Re: xentrace buffer size, maxcpus and online cpus

[Olaf Hering]

[-- Attachment #1: Type: text/plain, Size: 1884 bytes --]

Tue, 30 May 2023 10:41:07 +0200 Jan Beulich <jbeulich@suse.com>:

> Using this N would be correct afaict, but that N isn't num_online_cpus().
> CPUs may have been offlined by the time trace buffers are initialized, so
> without looking too closely I think it would be num_present_cpus() that
> you're after.

In my testing num_online_cpus returns N, while num_present_cpus returns
all available pcpus. There is also num_possible_cpus, but this appears to
be an ARM thing.

If Xen is booted with maxcpus=, is there a way to use the remaining cpus?
In case this is possible, the code needs adjustment to reinitialize the
trace buffers. This is not an easy change. But if the remaining cpus
will remain offline, then something like this may work:

+++ b/xen/common/trace.c
@@ -110,7 +110,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 +149,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;
 }
 


Olaf

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: xentrace buffer size, maxcpus and online cpus

[Jan Beulich]

On 30.05.2023 22:06, Olaf Hering wrote:
> Tue, 30 May 2023 10:41:07 +0200 Jan Beulich <jbeulich@suse.com>:
> 
>> Using this N would be correct afaict, but that N isn't num_online_cpus().
>> CPUs may have been offlined by the time trace buffers are initialized, so
>> without looking too closely I think it would be num_present_cpus() that
>> you're after.
> 
> In my testing num_online_cpus returns N, while num_present_cpus returns
> all available pcpus. There is also num_possible_cpus, but this appears to
> be an ARM thing.
> 
> If Xen is booted with maxcpus=, is there a way to use the remaining cpus?

In general no, because then nr_cpu_ids will be too constrained. But
note that CPU parking also comes into play here, leading to nr_cpu_ids
being set to all possible (present + hotplug) CPUs. Iirc parked CPUs
can be brought online even beyond what "maxcpus=" says (albeit I think
that's more a side effect of the parking implementation than an
intended goal).

> In case this is possible, the code needs adjustment to reinitialize the
> trace buffers. This is not an easy change. But if the remaining cpus
> will remain offline, then something like this may work:
> 
> +++ b/xen/common/trace.c
> @@ -110,7 +110,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;

As said before, num_online_cpus() will under-report for the purpose
here, as CPUs may have been brought offline, and may be brought online
again later (independent of the use of "maxcpus=").

Re-initializing trace buffers when more CPUs come online might of
course be an option, but it would need doing in a race free manner.

Jan

Re: xentrace buffer size, maxcpus and online cpus

[Olaf Hering]

[-- Attachment #1: Type: text/plain, Size: 705 bytes --]

Wed, 31 May 2023 11:05:52 +0200 Jan Beulich <jbeulich@suse.com>:

> As said before, num_online_cpus() will under-report for the purpose
> here, as CPUs may have been brought offline, and may be brought online
> again later (independent of the use of "maxcpus=").

It turned out, commit 74584a367051bc0d6f4b96fd360fa7bc6538fc39 broke
the expected behavior. But to me it is unclear what bug was fixed by
this commit.

If I read alloc_trace_bufs correctly, it already operates on online
cpus. And __trace_var will do nothing if called on a cpu which was
not online, t_bufs will likely be NULL.

To me it looks like commit 74584a367051bc0d6f4b96fd360fa7bc6538fc39
could be reverted.


Olaf

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: xentrace buffer size, maxcpus and online cpus

[Jan Beulich]

On 16.06.2023 13:47, Olaf Hering wrote:
> Wed, 31 May 2023 11:05:52 +0200 Jan Beulich <jbeulich@suse.com>:
> 
>> As said before, num_online_cpus() will under-report for the purpose
>> here, as CPUs may have been brought offline, and may be brought online
>> again later (independent of the use of "maxcpus=").
> 
> It turned out, commit 74584a367051bc0d6f4b96fd360fa7bc6538fc39 broke
> the expected behavior. But to me it is unclear what bug was fixed by
> this commit.

Hmm, I find title and description quite clear there.

> If I read alloc_trace_bufs correctly, it already operates on online
> cpus. And __trace_var will do nothing if called on a cpu which was
> not online, t_bufs will likely be NULL.

Yielding an incomplete overall trace, at best.

> To me it looks like commit 74584a367051bc0d6f4b96fd360fa7bc6538fc39
> could be reverted.

I don't think so. I'll add George to Cc as well, as he's the maintainer
of this stuff.

Jan

Re: xentrace buffer size, maxcpus and online cpus

[George Dunlap]

[-- Attachment #1: Type: text/plain, Size: 2003 bytes --]

On Fri, Jun 16, 2023 at 12:52 PM Jan Beulich <jbeulich@suse.com> wrote:

> On 16.06.2023 13:47, Olaf Hering wrote:
> > Wed, 31 May 2023 11:05:52 +0200 Jan Beulich <jbeulich@suse.com>:
> >
> >> As said before, num_online_cpus() will under-report for the purpose
> >> here, as CPUs may have been brought offline, and may be brought online
> >> again later (independent of the use of "maxcpus=").
> >
> > It turned out, commit 74584a367051bc0d6f4b96fd360fa7bc6538fc39 broke
> > the expected behavior. But to me it is unclear what bug was fixed by
> > this commit.
>
> Hmm, I find title and description quite clear there.
>
[SNIP]

> > To me it looks like commit 74584a367051bc0d6f4b96fd360fa7bc6538fc39
> > could be reverted.
>
> I don't think so. I'll add George to Cc as well, as he's the maintainer
> of this stuff.
>

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.


> > If I read alloc_trace_bufs correctly, it already operates on online
> > cpus. And __trace_var will do nothing if called on a cpu which was
> > not online, t_bufs will likely be NULL.
>
> Yielding an incomplete overall trace, at best.
>

Historically we've avoided the thorny problems of synchronization wrt the
trace buffers by saying that you get one chance to set them up the way you
want, and that's what you get until you reboot the host.  If someone felt
like sorting all that out I wouldn't oppose it.  But in the meantime, the
current "policy" is that it's OK if *tracing* breaks when changing the
number of cpus, as long as it doesn't cause the *hypervisor* to break.

The proposed change to calculate_tbuf_size() wouldn't work as-is, because
it looks like at the moment alloc_trace_bufs() leaves a gap in the mfn list
for "offline" CPUs.  But it looks like maybe the "interface" would allow
those "holes" to be compacted?

 -George

[-- Attachment #2: Type: text/html, Size: 2916 bytes --]

Re: xentrace buffer size, maxcpus and online cpus

[Olaf Hering]

[-- Attachment #1: Type: text/plain, Size: 389 bytes --]

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

> 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 see. Apparently some remapping is required to map a cpuid to an index
into the trace buffer metadata.


Olaf

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: xentrace buffer size, maxcpus and online cpus

[Andrew Cooper]

On 16/06/2023 4:37 pm, Olaf Hering wrote:
> Fri, 16 Jun 2023 15:22:24 +0100 George Dunlap <george.dunlap@cloud.com>:
>
>> 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 see. Apparently some remapping is required to map a cpuid to an index
> into the trace buffer metadata.

The xentrace mapping interface is horrible, and makes a lot of
assumptions which date from the early PV-only days.

If you want to improve things, we've got all the building blocks now for
a much more sane interface.

XENMEM_acquire_resource is a new mapping interface with far more sane
semantics which, amongst other things, will work in PVH guests too.

If we specify a new mapping space of type xentrace, using cpu id's as
the sub-index space (see vmtrace as an example), then you'll remove that
entire opencoded mechanism of passing mfns around, as well as reducing
the number of unstable hypercalls that the xentrace infrastructure uses.

I can talk you through it further if you feel up to tackling this.

~Andrew

Re: xentrace buffer size, maxcpus and online cpus

[Olaf Hering]

[-- Attachment #1: Type: text/plain, Size: 4922 bytes --]

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

> 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 #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: xentrace buffer size, maxcpus and online cpus

[Olaf Hering]

[-- Attachment #1: Type: text/plain, Size: 428 bytes --]

Fri, 16 Jun 2023 17:08:25 +0100 Andrew Cooper <andrew.cooper3@citrix.com>:

> XENMEM_acquire_resource is a new mapping interface with far more sane
> semantics which, amongst other things, will work in PVH guests too.

Does that indicate xentrace will not work in a PVH dom0?

I will have a look how XENMEM_acquire_resource is used in other places.
The resulting change will likely be too intrusive for 4.18.


Olaf

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: xentrace buffer size, maxcpus and online cpus

[Jan Beulich]

On 19.06.2023 12:13, Olaf Hering wrote:
> 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.

I see no reason why it shouldn't be possible to be used. At least as
long as overall line length doesn't make the result unwieldy. (It's
no different from a function call, in the end.)

Jan

Re: xentrace buffer size, maxcpus and online cpus

[Andrew Cooper]

On 19/06/2023 11:16 am, Olaf Hering wrote:
> Fri, 16 Jun 2023 17:08:25 +0100 Andrew Cooper <andrew.cooper3@citrix.com>:
>
>> XENMEM_acquire_resource is a new mapping interface with far more sane
>> semantics which, amongst other things, will work in PVH guests too.
> Does that indicate xentrace will not work in a PVH dom0?

To the best of my knowledge, it does not right now.

I seem to recall that ARM did some bodge to make it work for them, as
all guests are effectively PVH, but I don't recall exactly what that was.

> I will have a look how XENMEM_acquire_resource is used in other places.
> The resulting change will likely be too intrusive for 4.18.

It shouldn't be too bad IMO, but it's not the end of the world if it
falls into 4.19.

~Andrew