Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add znver1 arches with 32-bit + 64-bit variants and proper CPU detection #1035

Closed

Conversation

Conan-Kudo
Copy link
Member

@Conan-Kudo Conan-Kudo commented Feb 2, 2020

This change adds the ability for RPM to detect systems running with AMD Ryzen CPUs and allow the installation of packages that are built specifically for this subarchitecture.

As part of this change, a new %x86_64 macro has been introduced to alias all 64-bit x86 architecture variants in the same manner that the %ix86 macro has done for 32-bit x86 architecture variants.

This patch was originally authored by @berolinux and revised by both of us and has been in use in OpenMandriva since near the start of the development of OpenMandriva Lx 4.0 in early 2018. It has undergone several iterations in OpenMandriva, and now I'm confident that it is ready to land in RPM upstream.

Aside from making it possible to build RPMs with Ryzen specific optimizations and ensure those packages are properly selected, the integration of this patch upstream will restore full architecture compatibility of OpenMandriva RPMs with upstream RPM.

@Conan-Kudo Conan-Kudo force-pushed the add-znver1-arch branch 2 times, most recently from ff7d18b to 4b71e36 Compare February 2, 2020 04:02
This change adds the ability for RPM to detect systems running with
AMD Ryzen CPUs and allow the installation of packages that are
built specifically for this subarchitecture.

As part of this change, a new %x86_64 macro has been introduced
to alias all 64-bit x86 architecture variants in the same manner
that the %ix86 macro has done for 32-bit x86 architecture variants.

This change was originally written by Bernhard Rosenkränzer for
OpenMandriva to support Ryzen-optimized builds of OpenMandriva with
the OpenMandriva Lx 4.0 release. It has been improved and revised
in OpenMandriva over the past two years and is essentially finalized
now.

Co-authored-by: Neal Gompa <ngompa13@gmail.com>
Co-authored-by: Bernhard Rosenkränzer <bero@lindev.ch>
@mikhailnov
Copy link
Contributor

Why znver1 only, why only this architecture? It looks a bit strange

@Conan-Kudo
Copy link
Member Author

@mikhailnov I'm not going to add architectures nobody has...

@pmatilai
Copy link
Member

pmatilai commented Feb 3, 2020

Erm. So we have this nice sane architecture family known as x86_64, so lets go call it znver to make it ... more exciting? Sorry but I just don't think so.

@pmatilai
Copy link
Member

pmatilai commented Feb 3, 2020

Ignoring the actual micro architecture, as a general principle we do not add new assembler foo into rpm.
That there is pre-existing asm code doesn't mean it's a good idea, on the contrary the existing stuff needs to go too but that's obviously not in the scope here.

@Conan-Kudo
Copy link
Member Author

@pmatilai The addition of this new architecture name is because we need a way to declare incompatibility at the RPM level for CPUs that don't support it. This is pretty much the only way we can do it. When this was first being worked on, nobody seemed to think there was any better way to do this then, either. 😞

As for the assembler code.... Changing that to C would likely require introducing a library dependency to shift the assembler to something out of our purview. That's not to say that isn't necessarily a bad idea, but I don't have a good idea of what to pick and how it would work in RPM...

@ffesti
Copy link
Contributor

ffesti commented Feb 3, 2020

I think one of the questions is: Why doesn't the name start with x86_64 or at least something closer to that?

@Conan-Kudo
Copy link
Member Author

@ffesti Mainly because nothing else seemed to follow that convention? They all seemed to use the GCC machine architecture name, and so we went with that.

lib/rpmrc.c Show resolved Hide resolved
@pmatilai
Copy link
Member

I'm just really, really weary and dubious about these architecture tweaks because they're so bleeping arbitrary. Looking at gcc manual around znver:

      znver1
           AMD Family 17h core based CPUs with x86-64 instruction set
           support.  (This supersets BMI, BMI2, F16C, FMA, FSGSBASE, AVX,
           AVX2, ADCX, RDSEED, MWAITX, SHA, CLZERO, AES, PCL_MUL, CX16,
           MOVBE, MMX, SSE, SSE2, SSE3, SSE4A, SSSE3, SSE4.1, SSE4.2, ABM,
           XSAVEC, XSAVES, CLFLUSHOPT, POPCNT, and 64-bit instruction set
           extensions.

       znver2
           AMD Family 17h core based CPUs with x86-64 instruction set
           support. (This supersets BMI, BMI2, ,CLWB, F16C, FMA, FSGSBASE,
           AVX, AVX2, ADCX, RDSEED, MWAITX, SHA, CLZERO, AES, PCL_MUL,
           CX16, MOVBE, MMX, SSE, SSE2, SSE3, SSE4A, SSSE3, SSE4.1,
           SSE4.2, ABM, XSAVEC, XSAVES, CLFLUSHOPT, POPCNT, and 64-bit
           instruction set extensions.)

       btver1
           CPUs based on AMD Family 14h cores with x86-64 instruction set
           support.  (This supersets MMX, SSE, SSE2, SSE3, SSSE3, SSE4A,
           CX16, ABM and 64-bit instruction set extensions.)

       btver2
           CPUs based on AMD Family 16h cores with x86-64 instruction set
           support. This includes MOVBE, F16C, BMI, AVX, PCL_MUL, AES,
           SSE4.2, SSE4.1, CX16, ABM, SSE4A, SSSE3, SSE3, SSE2, SSE, MMX
           and 64-bit instruction set extensions.

Why do we need znver when we didn't need btver? Or the million other things that are there - gcc 9.2.1 manual lists no less than 79 cpu types for the x86 family, we know about a dozen. Which is already more than we should, I would say.

@pmatilai
Copy link
Member

Just FWIW, I've grown particularly averse to architecture patches because in the last few years, every single one of them has been nothing but a source of controversy and grief.

@Conan-Kudo
Copy link
Member Author

I'm just really, really weary and dubious about these architecture tweaks because they're so bleeping arbitrary.

I know, I don't particularly love it either, but RPM doesn't support defining arbitrary architectures and architecture filter mechanisms. Each architecture that people want to support needs to be added to RPM in a similar manner.

Why do we need znver when we didn't need btver?

There was demand for an AMD Ryzen optimized variant because of the potential to provide seriously interesting performance advantages, and so OpenMandriva built one. We did consider doing the same thing that Fedora proposed to x86_64, but it was determined to be absolutely insane. At this point, this patch has existed since early 2018 and we haven't encountered issues with the architecture filtering in over a year.

The Ryzen variant allows for a lot of instructions to improve performance on CPUs that we can guarantee all have them and allows us to retain compatibility on baseline x86_64 by retaining the same instruction set base there.

Just FWIW, I've grown particularly averse to architecture patches because in the last few years, every single one of them has been nothing but a source of controversy and grief.

I know, and most of them recently have been my fault. I've become particularly averse to sending patches upstream related to architecture work in the distros I maintain rpm because of this... 😢

But this one is important enough that I really want this in mainline rpm, since without it, it means regular rpm can't handle an entire distro set of packages...

@pmatilai
Copy link
Member

That arch patches are systematically so problematic is to me mostly just further testimony that we (as in rpm) are doing something seriously wrong in that department, not to be taken personally.

These are all about instruction set extensions, and those that actually matter should be exposed via hwcap. I'd much rather see something based on that than assembler code to detect an exact cpu type that will be obsoleted by the next generation in a year or two.

@Conan-Kudo
Copy link
Member Author

At least in this case, all Ryzen generation 1 and newer CPUs will match on znver1, so I don't think that'd be a problem with this patch. But I take your point about the general approach of adding more architectures.

I think it's probably something to explore on improving how we do this handling, but I think that's a greater refactor than I want to do for this particular architecture addition.

@berolinux
Copy link
Contributor

Come to think of it, it may be best to rework architecture handling to sit on top of Provides: (where there's implied Provides: for anything supported by the CPU).

That way emulators like qemu-static-arm could add Provides: arch(armv7hnl) etc. so you wouldn't need to force installation of packages you're intending to use with qemu static emulation.
Also installing something that is only available for the wrong architecture would know what qemu package it needs to pull in (imagine "dnf install my-prehistoric-binary-only-i386-custom-solution" on an ARM box)

@pmatilai
Copy link
Member

But this one is important enough that I really want this in mainline rpm, since without it, it means regular rpm can't handle an entire distro set of packages...

This is a dangerous argument, as by this logic we're required to accept anything at all that distros come up with.

I think it's probably something to explore on improving how we do this handling, but I think that's a greater refactor than I want to do for this particular architecture addition.

...and this argument repeats itself for each new architecture we get. Not to be taken personally, just noting. But adding more assembler detection is exactly in the opposite direction of where we want to go, which is why I'm pushing back so hard on that. Other arch families are merrily using hwcap for these sort of things, x86 should be able to follow. That's at least in the general direction of where we want to go on the "going south" level of things.

Oh, and I'm inclined to agree with @berolinux, provided (and required) capabilities would probably be the more flexible route of looking at this all.

@Conan-Kudo
Copy link
Member Author

This is a dangerous argument, as by this logic we're required to accept anything at all that distros come up with.

I am very much aware of this. But considering this is how we were told to implement this when we started this, I'm a little frustrated now that the approach isn't good enough anymore...

Oh, and I'm inclined to agree with @berolinux, provided (and required) capabilities would probably be the more flexible route of looking at this all.

Sure, but that still doesn't help with architecture property for things like %ifarch/%ifnarch and other similar things. Ideally, we'd have virtual dependencies for all kinds of hardware things, so that we could do fancy things like kernel modules supplement hardware and auto-install, automatic proposals for enhanced packages when CPU has certain instructions, etc.

@ignatenkobrain
Copy link
Contributor

For Rust packaging, we would appreciate being able to use rich dependencies together with architectures. Now we have to simply remove those specific bits or require everything everywhere.

Same goes to OS handling. you can't use %ifarch in deps =(

@mikhailnov
Copy link
Contributor

mikhailnov commented Mar 8, 2020

Did anyone measure the real performance gain from having a separate znver1 architecture? It is a lot of maintenance and QA burden, but what is the result?
As time goes futher, probably new hardware instructions will apear in modern x86 CPUs, so new arches like znver1 will have to be added.
Also do not forget about function multiversioning in Clear Linux - this is when some specific libraries are compiled in different variants.
I think that here a more generic approach is needed. In the vast majority of cases building packages for an x86_64 subarch like znver1 will not make sense, for example, is there a reason to make and test by QA separate variants of e.g. lightdm, sddm, plasma5-workspace, thunar, nautilus etc.? Is there any reason to spend resources on testing a separate branch of packages?
OpenMandriva does not have QA mostly, they probably did not consider this aspect.

So, to my mind, a more generic approach must allow to build only some special packages for a subarch and keep most of the packages of the main architecture. Something like function multiversioning intergated into the packaging system.

@mikhailnov
Copy link
Contributor

mikhailnov commented Mar 8, 2020

I meant not function multiversioning (https://docs.01.org/clearlinux/latest/tutorials/fmv.html), but building a separate binary is built with optimizations - https://clearlinux.org/news-blogs/transparent-use-library-packages-optimized-intel-architecture
Where needed, building of optimized libs may be turned on by a macro and thay can be automatically put to packages of an optimized subarch.

mikhailnov added a commit to mikhailnov/rpm that referenced this pull request Mar 23, 2021
This crap won't be upstreamized and we do not have so many resources
to build and test yet another pack of binaries without big performance gain.

rpm-software-management#1035 (comment)

To enable automatic detection of Ryzen CPUs create a file
/etc/rpm/znver1_enable
mikhailnov added a commit to mikhailnov/rpm that referenced this pull request Sep 30, 2021
This crap won't be upstreamized and we do not have so many resources
to build and test yet another pack of binaries without big performance gain.

Of course one can redefine %_target manually, but let's make automatic
detection of target follow the general policy of the distribution
which is building for generic x86_64, but not znver1.

rpm-software-management#1035 (comment)

To enable automatic detection of Ryzen CPUs create a file
/etc/rpm/znver1_enable
@romulasry
Copy link

Update?

@ffesti
Copy link
Contributor

ffesti commented May 5, 2022

I agree with @mikhailnov 's comment that we need a more generic solution if we want support the plethora of sub architectures. The added assembler code doesn't help either.
With no consensus reached in nearly two and a half years let's just close this now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants