Re: [PATCH v5 03/13] xen/arm: add Dom0 cache coloring support

[Julien Grall <julien@xxxxxxx>]

On 08/01/2024 11:04, Carlo Nonato wrote:
> Hi Julien,
>
> On Mon, Jan 8, 2024 at 11:25 AM Julien Grall <julien@xxxxxxx> wrote:
> > Hi Carlo,
> >
> > On 08/01/2024 10:06, Carlo Nonato wrote:
> > > > One of the reason is at least in the dom0less case, you will override
> > > > the value afterwards.
> > > In that case I need to allocate the array before parsing the string.
> > > I allocate a full array then the string is parsed and the actual size is found
> > > at the end of this phase. Knowing the actual size would require two parsing
> > > stages. Yes I'm wasting a bit of memory by oversizing the array here. Is it
> > > a problem?
> > While wasting memory is indeed not nice. This wasn't the main reason of
> > this comment.
> >
> > The reason is that you seem to set d->num_lcc_colors will but will never
> > be read before it gets overwritten. Looking again at the code, you are
> > also assuming parse_colors() will always take an array of nr_colors.
> Ok, I think I understood, but that happens only in dom0less case because
> d->num_llc_colors is overwritten after parsing. In other cases it's ok to set
> it there. Anyway I can move the assignment out of the function if that is
> clearer.
>
> > It would be better if parse_colors() takes the maximum size of the array
> > in parameter. This would harden the code and it makes more sense for
> > domain_alloc_colors() to set d->num_lcc_colors.
> I don't understand this. parse_colors() must take only arrays of nr_colors
> size (the global, maximum number of colors), otherwise the parsed string
> config could exceed the array size. Since we don't know in advance the real
> size before parsing, I think it's better to pass only arrays that are already
> allocated with the maximum size.
My concern is there is a disconnect. From the code, it is not obvious at 
all that parse_colors() only want to accept an array of nr_colors. If 
you pass an extra argument (or re-use the one you pass) for the array 
size and use within the code, then it makes more obvious that your array 
is always the correct size.
At least to me, this is a good practice in C to always pass the array 
and its size together (other language have that embedded). But I can 
appreciate this is not view like that for everyone. The minimum would be 
to document this requirement in a comment
> Doing as you said I would still pass nr_colors as the maximum size, but that
> would be strange since the global would still be accessible.
I don't really see the problem here. Your code doesn't need to use the 
global variable.
> If domain_alloc_colors() setting d->num_llc_colors is so confusing,
> I will just move the assignment after the function call.
>
> > Also, I just noticed you have a global variable named nr_colors and the
> > function parse_colors() takes an argument called *num_colors. This is
> > quite confusing, can we have better name?
> >
> > Maybe rename nr_colors to nr_global_colors and and num_colors to
> > nr_array_colors?
> I agree with the fact that naming is confusing. I would opt for max_nr_colors
> for the global.
I am fine with that.

Cheers,

--
Julien Grall