[AArch64] Cleanup memset

  Cleanup memset.  Remove unnecessary code for unused ZVA sizes.
For zero memsets it's faster use DC ZVA for >= 160 bytes rather
than 256. Add a define which allows skipping the ZVA size test when
the ZVA size is known to be 64 - reading dczid_el0 may be expensive.
This simplifies the Falkor memset implementation.

--
  

On 26/02/20 21:41, Wilco Dijkstra wrote:
> Cleanup memset.  Remove unnecessary code for unused ZVA sizes.
> For zero memsets it's faster use DC ZVA for >= 160 bytes rather
> than 256. Add a define which allows skipping the ZVA size test when
> the ZVA size is known to be 64 - reading dczid_el0 may be expensive.
> This simplifies the Falkor memset implementation.

This looks OK in general, although can you please elaborate on the
following:

- What cores did you test on to conclude that 160 is a better threshold
than 256?

- Is the intention to support non-64-byte zva sizes once there is actual
hardware that implements it and not bother with it for now?  I agree
with the idea if that's the case, just that it would be nice to have
that documented in the git commit message.

Siddhesh
  

Hi Siddhesh,

> This looks OK in general, although can you please elaborate on the
> following:
>
> - What cores did you test on to conclude that 160 is a better threshold
> than 256?

I've mostly done testing on Neoverse N1, Cortex-A72 and Cortex-A53.
(the latter seems to be always faster with DC ZVA disabled, so the threshold
doesn't really matter). I wrote a random memset benchmark similar to the
memcpy one, and performance is unchanged there given there is no change
in the way small cases are handled.

> - Is the intention to support non-64-byte zva sizes once there is actual
> hardware that implements it and not bother with it for now?  I agree
> with the idea if that's the case, just that it would be nice to have
> that documented in the git commit message.

Yes, otherwise it's hard to test or prove it helps performance after all. We've
had issues with the non-64 ZVA sizes before, so it's best to keep it simple.

I'm also trying to reduce the amount of code and avoid unnecessary proliferation
of almost identical ifuncs. I think we can remove most of the memset ifuncs,
it seems we need one version without ZVA and a ZVA version for size 64.

Cheers,
Wilco
  

On 03/03/20 20:12, Wilco Dijkstra wrote:
> Hi Siddhesh,
> 
>> This looks OK in general, although can you please elaborate on the
>> following:
>>
>> - What cores did you test on to conclude that 160 is a better threshold
>> than 256?
> 
> I've mostly done testing on Neoverse N1, Cortex-A72 and Cortex-A53.
> (the latter seems to be always faster with DC ZVA disabled, so the threshold
> doesn't really matter). I wrote a random memset benchmark similar to the
> memcpy one, and performance is unchanged there given there is no change
> in the way small cases are handled.

OK, please mention this in the commit log.

>> - Is the intention to support non-64-byte zva sizes once there is actual
>> hardware that implements it and not bother with it for now?  I agree
>> with the idea if that's the case, just that it would be nice to have
>> that documented in the git commit message.
> 
> Yes, otherwise it's hard to test or prove it helps performance after all. We've
> had issues with the non-64 ZVA sizes before, so it's best to keep it simple.
> 
> I'm also trying to reduce the amount of code and avoid unnecessary proliferation
> of almost identical ifuncs. I think we can remove most of the memset ifuncs,
> it seems we need one version without ZVA and a ZVA version for size 64.

Sounds like a plan.

Thanks,
Siddhesh
  

On Tue, Mar 3, 2020 at 6:42 AM Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:
>
> Hi Siddhesh,
>
> > This looks OK in general, although can you please elaborate on the
> > following:
> >
> > - What cores did you test on to conclude that 160 is a better threshold
> > than 256?
>
> I've mostly done testing on Neoverse N1, Cortex-A72 and Cortex-A53.
> (the latter seems to be always faster with DC ZVA disabled, so the threshold
> doesn't really matter). I wrote a random memset benchmark similar to the
> memcpy one, and performance is unchanged there given there is no change
> in the way small cases are handled.
>
> > - Is the intention to support non-64-byte zva sizes once there is actual
> > hardware that implements it and not bother with it for now?  I agree
> > with the idea if that's the case, just that it would be nice to have
> > that documented in the git commit message.
>
> Yes, otherwise it's hard to test or prove it helps performance after all. We've
> had issues with the non-64 ZVA sizes before, so it's best to keep it simple.
>
> I'm also trying to reduce the amount of code and avoid unnecessary proliferation
> of almost identical ifuncs. I think we can remove most of the memset ifuncs,
> it seems we need one version without ZVA and a ZVA version for size 64.

I am trying to understand what was the decision here.  The main reason
is OcteonTX2 does ZVA 128 which was faster than doing one without
(OcteonTX1 is similar but has an errata which causes ZVA to be turned
off).

Thanks,
Andrew Pinski

>
> Cheers,
> Wilco
  

Hi Andrew,

>> Yes, otherwise it's hard to test or prove it helps performance after all. We've
>> had issues with the non-64 ZVA sizes before, so it's best to keep it simple.
>>
>> I'm also trying to reduce the amount of code and avoid unnecessary proliferation
>> of almost identical ifuncs. I think we can remove most of the memset ifuncs,
>> it seems we need one version without ZVA and a ZVA version for size 64.
>
> I am trying to understand what was the decision here.  The main reason
> is OcteonTX2 does ZVA 128 which was faster than doing one without
> (OcteonTX1 is similar but has an errata which causes ZVA to be turned
> off).

So you're saying there may soon be a new microarchitecture which supports
ZVA size 128? Is there a significant speedup? Is querying the ZVA size fast?

Cheers,
Wilco
  

On Thu, Mar 12, 2020 at 8:46 AM Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:
>
> Hi Andrew,
>
> >> Yes, otherwise it's hard to test or prove it helps performance after all. We've
> >> had issues with the non-64 ZVA sizes before, so it's best to keep it simple.
> >>
> >> I'm also trying to reduce the amount of code and avoid unnecessary proliferation
> >> of almost identical ifuncs. I think we can remove most of the memset ifuncs,
> >> it seems we need one version without ZVA and a ZVA version for size 64.
> >
> > I am trying to understand what was the decision here.  The main reason
> > is OcteonTX2 does ZVA 128 which was faster than doing one without
> > (OcteonTX1 is similar but has an errata which causes ZVA to be turned
> > off).
>
> So you're saying there may soon be a new microarchitecture which supports
> ZVA size 128? Is there a significant speedup? Is querying the ZVA size fast?

By new you mean just publicly announced within a month that supports
ZVA size of 128; OcteonTX 2 annoucment:
https://www.marvell.com/company/newsroom/marvell-announces-octeon-tx2-family-of-multi-core-infrastructure-processors.html
?
Yes there is significant speedup for using "DC ZVA" as the latency is
one cycle compared to the multiple (8) cycles needed to fill the full
128byte cache line using stp.
Yes querying ZVA size fast, only a 1 cycles latency.

Thanks,
Andrew


>
> Cheers,
> Wilco
>
  

On 12/03/20 22:36, Andrew Pinski wrote:
> By new you mean just publicly announced within a month that supports
> ZVA size of 128; OcteonTX 2 annoucment:
> https://www.marvell.com/company/newsroom/marvell-announces-octeon-tx2-family-of-multi-core-infrastructure-processors.html
> ?
> Yes there is significant speedup for using "DC ZVA" as the latency is
> one cycle compared to the multiple (8) cycles needed to fill the full
> 128byte cache line using stp.
> Yes querying ZVA size fast, only a 1 cycles latency.

Wouldn't a zva128 be an improvement over zva_other?  Perhaps even move
towards per-zva size ifuncs that gets rid of the need to do a size check
inside memset.  That way, memset_falkor simply becomes memset_zva64.
IIRC the reason why that suggestion was resisted (that was my initial
approach for the falkor memset) was that it didn't give any significant
improvement on other architectures.  That's probably not a strong enough
reason now that I think of it, given that there will be a reasonable
limit to the number of ifuncs that get spawned off here.

IIRC Richard had proposed a similar patch for multiple zva sizes some
years ago too, not sure what happened to that series.

Siddhesh