Re: [PATCH v7 12/14] xen/arm: add Xen cache colors command line parameter

From: Carlo Nonato <carlo.nonato@minervasys.tech>
To: Jan Beulich <jbeulich@suse.com>
Cc: Luca Miccio <lucmiccio@gmail.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	 George Dunlap <george.dunlap@citrix.com>,
	Julien Grall <julien@xen.org>,
	 Stefano Stabellini <sstabellini@kernel.org>,
	Wei Liu <wl@xen.org>,
	 Marco Solieri <marco.solieri@minervasys.tech>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH v7 12/14] xen/arm: add Xen cache colors command line parameter
Date: Thu, 21 Mar 2024 16:36:31 +0100	[thread overview]
Message-ID: <CAG+AhRU6fcN71DfmHb+1xHrCSLyCOuFY5jdWLmLfUQrnmOH1Hw@mail.gmail.com> (raw)
In-Reply-To: <094ace88-eea9-4527-b166-9cd83214928d@suse.com>

Hi Jan

On Tue, Mar 19, 2024 at 4:54 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 15.03.2024 11:59, Carlo Nonato wrote:
> > From: Luca Miccio <lucmiccio@gmail.com>
> >
> > Add a new command line parameter to configure Xen cache colors.
> > These colors can be dumped with the cache coloring info debug-key.
> >
> > By default, Xen uses the first color.
> > Benchmarking the VM interrupt response time provides an estimation of
> > LLC usage by Xen's most latency-critical runtime task. Results on Arm
> > Cortex-A53 on Xilinx Zynq UltraScale+ XCZU9EG show that one color, which
> > reserves 64 KiB of L2, is enough to attain best responsiveness:
> > - Xen 1 color latency:  3.1 us
> > - Xen 2 color latency:  3.1 us
> >
> > More colors are instead very likely to be needed on processors whose L1
> > cache is physically-indexed and physically-tagged, such as Cortex-A57.
> > In such cases, coloring applies to L1 also, and there typically are two
> > distinct L1-colors. Therefore, reserving only one color for Xen would
> > senselessly partitions a cache memory that is already private, i.e.
> > underutilize it.
>
> Here you say that using just a single color is undesirable on such systems.
>
> > The default amount of Xen colors is thus set to one.
>
> Yet then, without any further explanation you conclude that 1 is the
> universal default.

A single default that suits every need doesn't exist, but we know that 1 is
good for the most widespread target we have (Cortex-A53). Having that said,
I think that a simple reorder of the description, while also making it more
explicit, solves the issue.

> > @@ -147,6 +159,21 @@ void __init llc_coloring_init(void)
> >          panic("Number of LLC colors (%u) not in range [2, %u]\n",
> >                max_nr_colors, CONFIG_NR_LLC_COLORS);
> >
> > +    if ( !xen_num_colors )
> > +    {
> > +        unsigned int i;
> > +
> > +        xen_num_colors = MIN(XEN_DEFAULT_NUM_COLORS, max_nr_colors);
> > +
> > +        printk(XENLOG_WARNING
> > +               "Xen LLC color config not found. Using first %u colors\n",
> > +               xen_num_colors);
> > +        for ( i = 0; i < xen_num_colors; i++ )
> > +            xen_colors[i] = i;
> > +    }
> > +    else if ( !check_colors(xen_colors, xen_num_colors) )
> > +        panic("Bad LLC color config for Xen\n");
>
> This "else" branch again lacks a bounds check against max_nr_colors, if
> I'm not mistaken.

Yep.

> Jan

Thanks.