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

UCT/MD/IB: Fix indirect key creation for MT registered memory #9424

Conversation

ivankochin
Copy link
Contributor

What

Support indirect keys creation for memory regions that were created by multiple treads.

Why ?

MT memory registration can happen (or not) implicitly for large buffers, but the current code doesn't support indirect keys creation for such registration (it always assumes that MR was registered as contig buffer by one thread)

How ?

Create uct_ib_mlx5_devx_reg_ksm_data wrapper that separates KSM creation method by MR flags.

Copy link
Contributor

@pentschev pentschev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ivankochin , this seems to resolve the issue I was experiencing. I'm +1 on this change.

@ivankochin ivankochin self-assigned this Oct 19, 2023
@ivankochin ivankochin force-pushed the uct/md/ib/support-mt-ksm-reg-for-indirect-keys branch from c3317ce to a409552 Compare October 20, 2023 05:57
contrib/configure-devel Outdated Show resolved Hide resolved
@ivankochin ivankochin requested a review from brminich October 26, 2023 06:52
test/gtest/uct/ib/test_ib_xfer.cc Outdated Show resolved Hide resolved
test/gtest/uct/test_p2p_mix.h Show resolved Hide resolved
@@ -556,6 +567,7 @@ static void uct_ib_mlx5_devx_reg_symmetric(uct_ib_mlx5_md_t *md,
uint32_t symmetric_rkey;
ucs_status_t status;

ucs_assert(!(memh->super.flags & UCT_IB_MEM_MULTITHREADED));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tvegas1 do we need to update symmetric key creation? To check whether original memh was registered in multiple threads or not

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if ksm (symmetric key) in front of ksm (multi-thread) is not supported, i think we should either:

  1. prevent MT when ((flags & UCT_MD_MEM_SYMMETRIC_RKEY) && (md->flags & UCT_IB_MLX5_MD_FLAG_MKEY_BY_NAME_RESERVE) is true
  2. prevent symmetric key when multi-thread flag is set on the memh

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

started #9447 to skip altogether mt reg for smkey.

src/uct/ib/mlx5/dv/ib_mlx5dv_md.c Outdated Show resolved Hide resolved
@brminich
Copy link
Contributor

brminich commented Oct 27, 2023

@tvegas1, @Artemy-Mellanox can you pls review

(uint64_t)memh->address, 0, 0,
"exported key", &cross_mr,
&exported_lkey);
ucs_assert(!(memh->super.flags & UCT_IB_MEM_MULTITHREADED));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, do someone know should we care about that in case of uct_ib_mlx5_devx_reg_exported_key or not? This call also always assumes that MR was create by single thread.

Should we use uct_ib_mlx5_devx_reg_ksm wrapper? Or assert is OK for now?

Copy link
Contributor

@brminich brminich Oct 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm not sure it is even supported (ksm + gvmi alias). You can leave an assert for now

ucs_assertv_always(mr->ksm_data->length > iova_offset,
"mr->ksm_data->length=%ld iova_offset=%u",
mr->ksm_data->length, iova_offset);
length = mr->ksm_data->length - iova_offset;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We set iova (which is address + iova_offset) to address field during atomic ksm creation. If I pass full length, then iova + length exceed the registered buffer size and test fails during devx_create_obj() call.

@Artemy-Mellanox WDYT is that OK to pass mr->ksm_data->length - iova_offset as length?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems we were not doing it previously on uct_ib_mlx5_devx_reg_atomic_key() code path

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree, seems it was mr->ksm_data->length previously, @ivankochin is this another bug fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, without the subtraction of the offset I get the following error:

create mkey: window size exceed translations_octword_size

Copy link
Contributor

@yosefe yosefe Oct 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is it needed now after the change?

src/uct/ib/mlx5/dv/ib_mlx5dv_md.c Outdated Show resolved Hide resolved
test/gtest/uct/ib/test_ib_md.cc Outdated Show resolved Hide resolved
ucs_assertv_always(mr->ksm_data->length > iova_offset,
"mr->ksm_data->length=%ld iova_offset=%u",
mr->ksm_data->length, iova_offset);
length = mr->ksm_data->length - iova_offset;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems we were not doing it previously on uct_ib_mlx5_devx_reg_atomic_key() code path

size_t length, uint64_t iova, uint32_t mkey_index,
const char *reason, struct mlx5dv_devx_obj **mr_p,
uint32_t *mkey)
uct_ib_mlx5_devx_reg_ksm_mt(uct_ib_mlx5_md_t *md, int atomic, void *address,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO uct_ib_mlx5_devx_reg_ksm_data is a better name - since this function accepts ksm_data structure to register. It's not actually doing a multi-threaded registration

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

size_t length, int list_size, size_t entity_size,
char *in, uint32_t mkey_index, const char *reason,
struct mlx5dv_devx_obj **mr_p, uint32_t *mkey)
uct_ib_mlx5_devx_create_ksm(uct_ib_mlx5_md_t *md, int atomic, uint64_t address,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it's worth adding 1-2 lines of documentation to each KSM registration function to document what it's doing in high level. The names are very similar to each other..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

ucs_assertv_always(mr->ksm_data->length > iova_offset,
"mr->ksm_data->length=%ld iova_offset=%u",
mr->ksm_data->length, iova_offset);
length = mr->ksm_data->length - iova_offset;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree, seems it was mr->ksm_data->length previously, @ivankochin is this another bug fix?

Comment on lines 392 to 395
ucs_debug("KSM registered memory %p..%p offset 0x%x%s on %s rkey 0x%x",
address, UCS_PTR_BYTE_OFFSET(address, length),
iova_offset, atomic ? " atomic" : "",
uct_ib_device_name(&md->super.dev), *mkey);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe put the debug message inside uct_ib_mlx5_devx_reg_ksm_mt and uct_ib_mlx5_devx_reg_ksm_contig, to distinguish between them in the log?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@ivankochin ivankochin requested review from brminich and yosefe November 7, 2023 05:38
src/uct/ib/mlx5/dv/ib_mlx5dv_md.c Show resolved Hide resolved
Comment on lines 2058 to 2059
ucs_assert(exported_lkey != 0);
ucs_assert(cross_mr != NULL);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why needed? Does it fix some valgrind error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before my patch, both exported_lkey and cross_mr were only defined in the beginning of this function. After adding the logging func in the end of uct_ib_mlx5_devx_reg_ksm_data_contig, compilation on Ubuntu 18.04 and SLES 15 started to fail (all other platforms passed) with message that these variables can be uninitialized.

So I decided to initialize them and check that we change them by the right way by these asserts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the log of failure:

/__w/1/s/contrib/../src/uct/ib/mlx5/dv/ib_mlx5dv_md.c: In function 'uct_ib_mlx5_devx_reg_exported_key':
/__w/1/s/contrib/../src/uct/ib/mlx5/dv/ib_mlx5dv_md.c:487:17: error: 'exported_lkey' may be used uninitialized in this function [-Werror=maybe-uninitialized]
     return mkey >> 8;
            ~~~~~^~~~
/__w/1/s/contrib/../src/uct/ib/mlx5/dv/ib_mlx5dv_md.c:2071:14: note: 'exported_lkey' was declared here
     uint32_t exported_lkey;
              ^~~~~~~~~~~~~
/__w/1/s/contrib/../src/uct/ib/mlx5/dv/ib_mlx5dv_md.c:2109:25: error: 'cross_mr' may be used uninitialized in this function [-Werror=maybe-uninitialized]
     memh->cross_mr      = cross_mr;
     ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~
/__w/1/s/contrib/../src/uct/ib/mlx5/dv/ib_mlx5dv_md.c:2070:29: note: 'cross_mr' was declared here
     struct mlx5dv_devx_obj *cross_mr;
                             ^~~~~~~~

It appeared after adding uct_ib_mlx5_devx_ksm_log function to uct_ib_mlx5_devx_reg_ksm_data_contig which isn't touch any uninitialized memory. Removing log function solves the issue.

Initialization of these variables seems the best way to solve this issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found that the issue can be solved by removing information about mkey from uct_ib_mlx5_devx_ksm_log function. That information is already printed inside uct_ib_mlx5_devx_reg_ksm in case of successfull registration, so, I think it can be removed here.

uct_mem_h memh;

if ((access_mask & UCT_MD_MEM_ACCESS_REMOTE_ATOMIC) && is_bf_arm()) {
UCS_TEST_SKIP_R("FIXME: AMO reg key bug on BF device, skipping");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copied that message from test_md::test_reg_mem since it's doing the pretty similar thing. I didn't catch this error by myself.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe it's not relevant anymore and can be removed? Can you try remove it and see whether CI passes.
@Artemy-Mellanox are you aware about this issue?

Comment on lines +278 to +277
comp().comp.func = dereg_cb;
comp().comp.count = 1;
comp().comp.status = UCS_OK;
comp().self = this;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we really need completion in this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

completion is required if UCT_MD_MEM_DEREG_FLAG_INVALIDATE is set

test/gtest/uct/ib/test_ib_xfer.cc Show resolved Hide resolved
test/gtest/uct/test_p2p_mix.cc Outdated Show resolved Hide resolved
@ivankochin ivankochin requested a review from brminich November 27, 2023 05:23
brminich
brminich previously approved these changes Nov 27, 2023
@ivankochin
Copy link
Contributor Author

@Artemy-Mellanox pls review

@ivankochin
Copy link
Contributor Author

@yosefe pls review

Copy link
Contributor

@yosefe yosefe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to make sure it does not increase testing time

src/uct/ib/mlx5/dv/ib_mlx5dv_md.c Outdated Show resolved Hide resolved
src/uct/ib/mlx5/dv/ib_mlx5dv_md.c Outdated Show resolved Hide resolved
src/uct/ib/mlx5/dv/ib_mlx5dv_md.c Show resolved Hide resolved
src/uct/ib/mlx5/dv/ib_mlx5dv_md.c Show resolved Hide resolved
src/uct/ib/mlx5/dv/ib_mlx5dv_md.c Show resolved Hide resolved
test/gtest/uct/ib/test_ib_md.cc Show resolved Hide resolved
test/gtest/uct/ib/test_ib_md.cc Outdated Show resolved Hide resolved
test/gtest/uct/ib/test_ib_xfer.cc Outdated Show resolved Hide resolved
test/gtest/uct/ib/test_ib_xfer.cc Outdated Show resolved Hide resolved
test/gtest/uct/ib/test_ib_xfer.cc Outdated Show resolved Hide resolved
@ivankochin ivankochin dismissed stale reviews from Artemy-Mellanox and brminich via 133003a December 15, 2023 08:47
Copy link
Contributor

@yosefe yosefe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove use_last_byte from test_p2p_mix
otherwise, LGTM

Copy link
Contributor

@yosefe yosefe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also there is a build failure

UCS_TEST_P(uct_p2p_mix_test_mt, mix1000_last_byte_offset)
{
/* Alloc 2 chunks buffer, but perform all the operation on the last 8 bytes */
run(1000, reg_mt_chunk * 2 - 8, 8);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use ( ) around * expression

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

m_rkey.rkey = other.m_rkey.rkey;
m_rkey.handle = other.m_rkey.handle;

other.fill_empty();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename fill_empty -> reset

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

yosefe
yosefe previously approved these changes Dec 26, 2023
@ivankochin ivankochin force-pushed the uct/md/ib/support-mt-ksm-reg-for-indirect-keys branch from 09d9928 to 173511d Compare December 27, 2023 02:57
@ivankochin ivankochin enabled auto-merge December 27, 2023 02:58
@ivankochin ivankochin force-pushed the uct/md/ib/support-mt-ksm-reg-for-indirect-keys branch from 4bb8d60 to 9622c19 Compare December 28, 2023 06:42
yosefe
yosefe previously approved these changes Dec 28, 2023
@ivankochin
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 4 pipeline(s).

yosefe
yosefe previously approved these changes Jan 4, 2024
@yosefe
Copy link
Contributor

yosefe commented Jan 4, 2024

@ivankochin can u pls squash?

@ivankochin ivankochin force-pushed the uct/md/ib/support-mt-ksm-reg-for-indirect-keys branch from 5ac9620 to 1706b1c Compare January 5, 2024 08:23
@ivankochin ivankochin force-pushed the uct/md/ib/support-mt-ksm-reg-for-indirect-keys branch from 1706b1c to 967fe61 Compare January 7, 2024 17:42
@ivankochin
Copy link
Contributor Author

I did the rebase to resolve the merge conflicts. The only change is add IB_ prefix for all the modified env variables in test_ib_xfer.

@yosefe
Copy link
Contributor

yosefe commented Jan 8, 2024

I did the rebase to resolve the merge conflicts. The only change is add IB_ prefix for all the modified env variables in test_ib_xfer.

Why not a merge commit?

@ivankochin
Copy link
Contributor Author

ivankochin commented Jan 8, 2024

I did the rebase to resolve the merge conflicts. The only change is add IB_ prefix for all the modified env variables in test_ib_xfer.

Why not a merge commit?

To patch was already squased so I did it to prevent merge commit creation and squashing it since the merge conflict solution was straightforward for test_ib_xfer. But I completely forgot about required changres in test_ib_md, so I created one more commit yesterday evening.

@yosefe
Copy link
Contributor

yosefe commented Jan 8, 2024

To patch was already squased so I did it to prevent merge commit creation and squashing it since the merge conflict solution was straightforward for test_ib_xfer. But I completely forgot about required changres in test_ib_md, so I created one more commit yesterday evening.

Even in this case, need to use a merge commit, since there is no good way to review the conflict resolution changes

…ultiple threads
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

6 participants