[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v6 04/15] xen/arm: add Dom0 cache coloring support
Hi Jan, On Mon, Feb 5, 2024 at 10:35 AM Jan Beulich <jbeulich@xxxxxxxx> wrote: > > On 03.02.2024 12:39, Carlo Nonato wrote: > > On Thu, Feb 1, 2024 at 2:30 PM Jan Beulich <jbeulich@xxxxxxxx> wrote: > >> On 29.01.2024 18:18, Carlo Nonato wrote: > >>> --- a/xen/common/llc-coloring.c > >>> +++ b/xen/common/llc-coloring.c > >>> @@ -17,6 +17,63 @@ size_param("llc-way-size", llc_way_size); > >>> /* Number of colors available in the LLC */ > >>> static unsigned int __ro_after_init max_nr_colors = CONFIG_NR_LLC_COLORS; > >>> > >>> +static unsigned int __initdata dom0_colors[CONFIG_NR_LLC_COLORS]; > >>> +static unsigned int __initdata dom0_num_colors; > >>> + > >>> +/* > >>> + * Parse the coloring configuration given in the buf string, following > >>> the > >>> + * syntax below. > >>> + * > >>> + * COLOR_CONFIGURATION ::= COLOR | RANGE,...,COLOR | RANGE > >>> + * RANGE ::= COLOR-COLOR > >>> + * > >>> + * Example: "0,2-6,15-16" represents the set of colors: > >>> 0,2,3,4,5,6,15,16. > >>> + */ > >>> +static int parse_color_config(const char *buf, unsigned int *colors, > >>> + unsigned int num_colors, unsigned int > >>> *num_parsed) > >> > >> Is this function going to be re-used? If not, it wants to be __init. > >> If so, I wonder where the input string is going to come from ... > > > > You're right. It needs __init. > > Am I misremembering to have spotted a non-init use in a later patch? The only usage of the function other than for parameter parsing is in domain_set_llc_colors_from_str() which is in turn used in dom0less domain creation, so from another __init function. Thanks. > >> Also "num_colors" looks to be misnamed - doesn't this specify an > >> upper bound only? > > > > It's the real size of the colors array. > > Hence my remark: It is _not_ the number of colors. > > >>> +int __init dom0_set_llc_colors(struct domain *d) > >>> +{ > >>> + unsigned int *colors; > >>> + > >>> + if ( !dom0_num_colors ) > >>> + return domain_set_default_colors(d); > >>> + > >>> + colors = alloc_colors(dom0_num_colors); > >>> + if ( !colors ) > >>> + return -ENOMEM; > >>> + > >>> + memcpy(colors, dom0_colors, sizeof(unsigned int) * dom0_num_colors); > >> > >> sizeof(*colors) or some such please. Plus a check that colors and > >> dom0_colors are actually of the same type. Alternatively, how about > >> making dom0_colors[] __ro_after_init? Is this too much of a waste? > > > > You mean an ASSERT on the two arrays type? > > I don't think you can use ASSERT() for such very well. It's runtime > check, when here we want a build-time one. I'd therefore rather see > it be something like > > (void)(colors == dom0_colors); > > Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |