search

Sandia-OpenSHMEM / SOS

Sandia OpenSHMEM is an implementation of the OpenSHMEM specification over multiple Networking APIs, including Portals 4, the Open Fabric Interface (OFI), and UCX. Please click on the Wiki tab for help with building and using SOS.
Other
61 stars 53 forks source link

Disabling put_scalar when disable-ofi-inject is used #1110

Closed wrrobin closed 6 months ago

davidozog commented 6 months ago

@wrrobin - We can ignore the UCX over pmi-mpi failure, I'm removing it in #1106. Do you think we should add at least one test with --disable-ofi-inject here?

wrrobin commented 6 months ago

Thanks @davidozog. You are right about the diffs. I will add a test in CI. We can merge this after #1106 is merged.

davidozog commented 6 months ago

This looks good to merge when ready @wrrobin

wrrobin commented 6 months ago

@davidozog - Yes, this is good to go. One thing I should mention, we are not disabling OFI atomic inject with the same config flag. We might need to do this, but I am thinking to create a separate PR for that with a new flag option -disable-ofi-atomic-inject. Thoughts?

davidozog commented 6 months ago

@davidozog - Yes, this is good to go. One thing I should mention, we are not disabling OFI atomic inject with the same config flag. We might need to do this, but I am thinking to create a separate PR for that with a new flag option -disable-ofi-atomic-inject. Thoughts?

That makes sense to me, thanks for the note.

It's slightly unfortunately the DISABLE_OFI_INJECT macro is outside/above the transport_ofi layer, but I do think this is the simplest way to do it. Do you think it's possible to keep it within the transport layer? Something to think about when adding "disable-ofi-atomic-inject".

It's not the first time we've done something similar to this though - see here: https://github.com/Sandia-OpenSHMEM/SOS/blob/v1.5.2/src/init.c#L206

wrrobin commented 6 months ago

Yeah.. I thought about it too. And, initially, I made changes only on the transport layer for a particular function, but it requires more changes. If it is ok, lets move forward with this. I am keeping a issue created for this and will handle separately.