[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v5 01/13] xen/common: add cache coloring common code
Hi Jan, On Mon, Jan 8, 2024 at 5:53 PM Jan Beulich <jbeulich@xxxxxxxx> wrote: > > On 02.01.2024 10:51, Carlo Nonato wrote: > > This commit adds the Last Level Cache (LLC) coloring common header, Kconfig > > options and functions. Since this is an arch specific feature, actual > > implementation is postponed to later patches and Kconfig options are placed > > under xen/arch. > > As a general remark / nit: "This commit", "this patch", or alike aren't > well suited for descriptions. > > > --- a/xen/arch/Kconfig > > +++ b/xen/arch/Kconfig > > @@ -31,3 +31,19 @@ config NR_NUMA_NODES > > associated with multiple-nodes management. It is the upper bound of > > the number of NUMA nodes that the scheduler, memory allocation and > > other NUMA-aware components can handle. > > + > > +config LLC_COLORING > > + bool "Last Level Cache (LLC) coloring" if EXPERT > > + depends on HAS_LLC_COLORING > > + > > +config NR_LLC_COLORS > > + int "Maximum number of LLC colors" > > + default 128 > > What if I set to value to 0? Or to an unreasonably large one? You don't > bound the value range at all. I can reintroduce the range (it was there in previous versions). I just don't know what numbers to put. range 2 1024 seems reasonable since having 1 color only actually lowers performances because of the obvious sharing of resources. 1024 are the colors that can fit into a standard 4KiB page. It's big enough for currently supported hardware that normally has 16 or 32 colors. > > --- a/xen/common/domain.c > > +++ b/xen/common/domain.c > > @@ -7,6 +7,7 @@ > > #include <xen/compat.h> > > #include <xen/init.h> > > #include <xen/lib.h> > > +#include <xen/llc-coloring.h> > > #include <xen/ctype.h> > > #include <xen/err.h> > > #include <xen/param.h> > > @@ -1144,6 +1145,9 @@ static void cf_check complete_domain_destroy(struct > > rcu_head *head) > > struct vcpu *v; > > int i; > > > > + if ( is_domain_llc_colored(d) ) > > + domain_llc_coloring_free(d); > > Would be nice if the freeing function could be called unconditionally, > being a no-op for non-colored domains. Ok. > Further - is it really necessary to do this freeing this late? No, I can move it in domain_destroy(). > > --- a/xen/common/keyhandler.c > > +++ b/xen/common/keyhandler.c > > @@ -6,6 +6,7 @@ > > #include <xen/debugger.h> > > #include <xen/delay.h> > > #include <xen/keyhandler.h> > > +#include <xen/llc-coloring.h> > > #include <xen/param.h> > > #include <xen/shutdown.h> > > #include <xen/event.h> > > @@ -307,6 +308,9 @@ static void cf_check dump_domains(unsigned char key) > > > > arch_dump_domain_info(d); > > > > + if ( is_domain_llc_colored(d) ) > > + domain_dump_llc_colors(d); > > I'm less concerned of the conditional here, but along the lines of the > comment above, it could of course again be the function that simply is > a no-op for non-colored domains. Ok. > > --- /dev/null > > +++ b/xen/include/xen/llc-coloring.h > > @@ -0,0 +1,46 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* > > + * Last Level Cache (LLC) coloring common header > > + * > > + * Copyright (C) 2022 Xilinx Inc. > > + * > > + * Authors: > > + * Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx> > > + */ > > +#ifndef __COLORING_H__ > > +#define __COLORING_H__ > > + > > +#include <xen/sched.h> > > +#include <public/domctl.h> > > + > > +#ifdef CONFIG_HAS_LLC_COLORING > > Why does this matter here? IOW why ... > > > +#include <asm/llc-coloring.h> > > + > > +#ifdef CONFIG_LLC_COLORING > > ... is it not just this which is checked? > > > +extern bool llc_coloring_enabled; > > +#define llc_coloring_enabled (llc_coloring_enabled) > > +#endif > > + > > +#endif > > + > > +#ifndef llc_coloring_enabled > > +#define llc_coloring_enabled (false) > > +#endif > > +1 to the question Julien has raised here. Yes this whole block can be better structured. > > --- a/xen/include/xen/sched.h > > +++ b/xen/include/xen/sched.h > > @@ -626,6 +626,11 @@ struct domain > > > > /* Holding CDF_* constant. Internal flags for domain creation. */ > > unsigned int cdf; > > + > > +#ifdef CONFIG_LLC_COLORING > > + unsigned int *llc_colors; > > Can the color values change over the lifetime of a domain? If not, > it may be prudent to have this be pointer-to-const. Can I free a pointer-to-const array? > Jan > > > + unsigned int num_llc_colors; > > +#endif > > }; > > > > static inline struct page_list_head *page_to_list( > Thanks.
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |