ARCH/X86: Add Turin support #10272
ARCH/X86: Add Turin support #10272
Conversation
Signed-off-by: Arun Chandran <Arun.Chandran@amd.com> Acked-by: Edgar Gabriel <edgar.gabriel@amd.com>
src/ucs/arch/x86_64/cpu.c
Outdated
| case 0x00 ... 0x2f: | ||
| case 0x40 ... 0x4f: | ||
| case 0x60 ... 0x7f: |
There was a problem hiding this comment.
seems it's a non-standard extension? maybe use if condition?
There was a problem hiding this comment.
If I use the 'if condition' for 'model', it would make the code less structured. The 'case ranges' are a well-known extension supported by both GCC and Clang. Can we stick with it?
There was a problem hiding this comment.
I will change the style then.
|
It would be good to have this patch btw. in 1.18.0 (if possible) |
config/ucx.conf
Outdated
| @@ -35,3 +35,7 @@ UCX_GDR_COPY_LAT=get:1.65e-6,put:0.65e-6 | |||
| [AMD Genoa] | |||
| CPU model=Genoa | |||
| UCX_DISTANCE_BW=auto,sys:5100MBs | |||
|
|
|||
| [AMD Turin] | |||
There was a problem hiding this comment.
Is that possible to set UCX_DISTANCE_BW one time for all AMD CPU families? Something like:
[AMD]
CPU vendor=AMD
UCX_DISTANCE_BW=auto,sys:5100MBs
There was a problem hiding this comment.
Could you please explain more about "UCX_DISTANCE_BW" ? Why we have to keep 'sys' at 5100MBs?[I was not sure about this, I used the same value that used in Rome, Milan & Genoa] Is this should be calculated at runtime?
There was a problem hiding this comment.
That's a good question, that's pretty old setting that was measured long time ago on some AMD system and I am not sure whether we need to set it for Turin systems too. @yosefe WDYT?
My previous comment was only suggesting to set it one time for all AMD system instead of patching each CPU generation separately, but probably if we don't want to set this value for this generation this change would redundant too.
There was a problem hiding this comment.
@yosefe could you please comment on "UCX_DISTANCE_BW" ?
There was a problem hiding this comment.
UCX_DISTANCE_BW is specifying the expected PCIe bandwidth as a function of distance as defined here: https://github.com/openucx/ucx/blob/master/src/ucs/config/global_opts.c#L78
There was a problem hiding this comment.
Currently UCX cannot determine the actual ucs_global_opts.dist.sys.bandwidth value, the default is 220 MB/s which is probably not right value for AMD.
There was a problem hiding this comment.
Can it be derrived reading /proc or /sys entries ? How shall we proceed further here?
There was a problem hiding this comment.
Can it be derrived reading /proc or /sys entries ? How shall we proceed further here?
This is a question for AMD, I guess. I'm not aware of such way, but would be great to have.
I'd suggest for now to define single BW value for all AMD CPU's , as suggest by @ivankochin
There was a problem hiding this comment.
@yosefe @ivankochin Apologies for my ignorance, but I want to ensure I understand this correctly before proceeding. With 2 Turin-only nodes connected via IB, are there benchmark tests available to measure the impact of varying "UCX_DISTANCE_BW=auto,sys:values_from_low_to_high" values? Or Does this only apply to GPU-to-RDMA transfers in a GPU cluster?
There was a problem hiding this comment.
This setting may affect the threshold switching of GPU protocols, so probably benchmarks like "osu_bw D D" could uncover the difference on some systems.
arun-chandran-edarath
force-pushed
the
add_turin
branch
2 times, most recently
from
d8692fa
to
e591a17
Compare
Acked-by: Edgar Gabriel edgar.gabriel@amd.com
What?
Adding support for AMD's Turin processors.
Ref: https://www.amd.com/en/products/processors/server/epyc/9005-series.html