Hi Doug,
On 2020-08-18 20:42, Doug Anderson wrote:
Hi,
<snip>...
I guess to start, it wasn't obvious (to me) that there were two
choices and we were picking one. Mentioning that the other
alternative was way-based allocation would help a lot. Even if you
can't fully explain the differences between the two, adding something
to the commit message indicating that this is a policy decision (in
other words, both work but each have their tradeoffs) would help.
Something like this, if it's correct:
In general we try to enable capacity based allocation (instead of the
default way based allocation) since that gives us better performance
with the current software / hardware configuration.
Thanks, I will add it for next version. Let me also go poke some arch
teams
to understand if we actually do gain something with this selection, who
knows
we might get some additional details as well.
>
> Why are you introducing a whole second table? Shouldn't you just add
> a field to "struct qcom_llcc_config" ?
>
This was my 2nd option, first one was to have this based on the
version
of LLCC
which are exposed by hw info registers. But that didn't turn out good
since I
couldn't find any relation of this property with LLCC version.
Second option was as you mentioned to have a field to
qcom_llcc_config.
Now this is good,
but then I thought that if we add LLCC support for 20(random number)
SoCs of which
10 is capable of supporting cap_based_alloc and rest 10 are not, then
we
will still be adding
20 more lines to each SoC's llcc_config if we follow this 2nd option.
If you do it right, you'd only need to add lines to the SoCs that need
it. Linux conventions in general are that it's OK (and encouraged) to
rely on the fact that if you don't mention a variable in static
initialization it's initted to 0/False/NULL. So if the member
variable is "need_llcc_config" then you only need to add this in
places where you're setting it to true. It only needs to be a boolean
so if later someone really is worried about all the bytes that flags
like this are taking up we can use a bitfield. For now (presumably)
just adding a boolean would be more efficient since (presumably) the
extra code needed to access a bitfield would be more than the extra
data bytes used. In theory this could also be initdata probably, too.
Yes but in this case we would better be explicit about the capable SoCs
because for someone in QCOM it would be easier to confirm if the SoC is
actually capable but it will not be very obvious for outside folks that
the particular SoC actually supports it. I will use a boolean field
properly
initialized to indicate if a particular SoC is capable or not.
So why not opt for a 3rd option with the table where you just need to
specify only the capable
targets which is just 10 in our sample case above.
Are you trying to save space? ...or complexity? Sure a good compiler
will probably pool the constant string here so you won't need to
allocate it twice, but IMO having a second match table is more
complex. You also need at least a pointer + bool per entry. Each
probe will now need to parse through all these strings, too. None of
this is a big deal, but I'd bet just adding a field to the existing
struct is lower overhead all around.
Mostly space, but I agree now that the boolean field is indeed better
than
a separate table.
Am I just overthinking this too much and should just go with the 2nd
option as you mentioned?
Someone could feel free to vote against me, but I'd just add to the
existing config.
I vote for you :)
Thanks,
Sai
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member
of Code Aurora Forum, hosted by The Linux Foundation
