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

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

On Mon, Jan 8, 2024 at 12:44 PM Julien Grall <julien@xxxxxxx> wrote:
>
>
>
> 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

Ok got it. Thanks for the explanation.

> > 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