UCT/MLX5/UCS: Print error message if SRQ topology is invalid #10479
+46
−5
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yosefe
force-pushed
the
topic/uct-mlx5-ucs-print-error-message-if
branch
from
859f85f to
7ba8a42
Compare
iyastreb
reviewed
Feb 11, 2025
src/ucs/datastruct/string_buffer.c
Outdated
Comment on lines
141
to
149
| size_t i; | ||
|
|
||
| for (i = 0; i < count; ++i) { | ||
| if (i > 0) { | ||
| ucs_string_buffer_appendf(strb, "%s%s", sep, strs[i]); | ||
| } else { | ||
| ucs_string_buffer_appendf(strb, "%s", strs[i]); | ||
| } | ||
| } |
There was a problem hiding this comment.
minor: we can remove branch in the loop:
Suggested change
| size_t i; | |
| for (i = 0; i < count; ++i) { | |
| if (i > 0) { | |
| ucs_string_buffer_appendf(strb, "%s%s", sep, strs[i]); | |
| } else { | |
| ucs_string_buffer_appendf(strb, "%s", strs[i]); | |
| } | |
| } | |
| size_t i; | |
| if (count > 0) { | |
| ucs_string_buffer_appendf(strb, "%s", strs[0]); | |
| } | |
| for (i = 1; i < count; ++i) { | |
| ucs_string_buffer_appendf(strb, "%s%s", sep, strs[i]); | |
| } |
src/ucs/datastruct/string_buffer.h
Outdated
| @@ -179,6 +179,18 @@ void ucs_string_buffer_append_flags(ucs_string_buffer_t *strb, uint64_t mask, | |||
| const char **flag_names); | |||
|
|
|||
|
|
|||
| /** | |||
| * Append a list of strings separated by a custom token. | |||
src/ucs/datastruct/string_buffer.c
Outdated
| @@ -135,6 +135,20 @@ void ucs_string_buffer_append_flags(ucs_string_buffer_t *strb, uint64_t mask, | |||
| ucs_string_buffer_rtrim(strb, ",|"); | |||
| } | |||
|
|
|||
| void ucs_string_buffer_append_list(ucs_string_buffer_t *strb, const char *sep, | |||
There was a problem hiding this comment.
Or ucs_string_buffer_append_all which is quite common in some libraries
src/ucs/datastruct/string_buffer.h
Outdated
| * @param [in] strs Array of strings to append. | ||
| * @param [in] count Number of strings in the array. | ||
| */ | ||
| void ucs_string_buffer_append_list(ucs_string_buffer_t *strb, const char *sep, |
There was a problem hiding this comment.
Does it make sense to make a generic append_array instead, that would accept any type of array?
Something like:
#define UCS_SB_APPEND_ARRAY(_strb, _array, _fmt, _sep) \
ucs_array_for_each(_elem, _array) { \
ucs_string_buffer_appendf(_strb, "%s"_fmt, _sep, _elem); \
}
tvegas1
reviewed
Feb 11, 2025
src/ucs/datastruct/string_buffer.c
Outdated
| @@ -135,6 +135,20 @@ void ucs_string_buffer_append_flags(ucs_string_buffer_t *strb, uint64_t mask, | |||
| ucs_string_buffer_rtrim(strb, ",|"); | |||
| } | |||
|
|
|||
| void ucs_string_buffer_append_list(ucs_string_buffer_t *strb, const char *sep, | |||
roiedanino
reviewed
Feb 11, 2025
test/gtest/ucs/test_string.cc
Outdated
| @@ -311,6 +311,15 @@ UCS_TEST_F(test_string_buffer, flags) { | |||
| EXPECT_EQ(std::string("one|three"), ucs_string_buffer_cstr(&strb)); | |||
| } | |||
|
|
|||
| UCS_TEST_F(test_string_buffer, list) { | |||
| static const char *str_list[] = {"once", "upon", "a", "time"}; | |||
There was a problem hiding this comment.
Minor: str_list -> str_arr
src/ucs/datastruct/string_buffer.c
Outdated
| @@ -135,6 +135,20 @@ void ucs_string_buffer_append_flags(ucs_string_buffer_t *strb, uint64_t mask, | |||
| ucs_string_buffer_rtrim(strb, ",|"); | |||
| } | |||
|
|
|||
| void ucs_string_buffer_append_list(ucs_string_buffer_t *strb, const char *sep, | |||
There was a problem hiding this comment.
Or ucs_string_buffer_append_all which is quite common in some libraries
roiedanino
previously approved these changes
Feb 11, 2025
tvegas1
reviewed
Feb 11, 2025
tvegas1
approved these changes
Feb 12, 2025
iyastreb
reviewed
Feb 12, 2025
| * @param _array Array of elements to append. | ||
| * @param _count Number of elements in the array. | ||
| */ | ||
| #define ucs_string_buffer_append_array(_strb, _sep, _fmt, _array, _count) \ |
There was a problem hiding this comment.
Ok, now it's cool
Just one minor thing: currently this is suitable for C arrays, maybe it makes sense to have an "overload" for ucs_array?
// C array
#define ucs_string_buffer_append_carray(_strb, _sep, _fmt, _array, _count) << current impl
#define ucs_string_buffer_append_array(_strb, _sep, _fmt, _array) \ << overload for UCS array
ucs_string_buffer_append_carray(_strb, _sep, _fmt, _array.buffer, _array.length)Up to you, not really critical
There was a problem hiding this comment.
Today we don't have a place we use it for ucs_array so IMO it's not needed
yosefe
force-pushed
the
topic/uct-mlx5-ucs-print-error-message-if
branch
from
ae2834d to
c6e568b
Compare
roiedanino
approved these changes
Feb 12, 2025
iyastreb
approved these changes
Feb 12, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why
Without this fix, if the none of the SRQ topologies provided in the configuration is supported, there is a failure to create the uct_iface without an indicative error message.