Skip to content
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/uct/api/v2/uct_v2.h
Original file line number Diff line number Diff line change
Expand Up @@ -1916,8 +1916,12 @@ typedef struct uct_ep_op_info {
/**
* @ingroup UCT_RESOURCE
* @brief Callback invoked for each undelivered outstanding operation.
*

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The return value is not consumed or documented anywhere. The new doc says UCS_OK means the error was handled and otherwise it is "returned back", but: no in-tree caller consumes the status (the only invocation is in test_ib_mlx5_ext.cc:72, which discards the return value); and the uct_ep_outstanding_purge function doc and uct_ep_outstanding_purge_params_t::cb doc are not updated to describe what "returned back" means (e.g. does a non-UCS_OK from the callback abort the purge, and does it propagate out of uct_ep_outstanding_purge?). Please clarify in the API docs how the status flows back to the uct_ep_outstanding_purge caller, and update the test callback so the change is exercised.

* @return @ref UCS_OK - The operation was purged successfully.
* Otherwise - The operation was not purged and is returned back

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @return @ref UCS_OK - The operation was purged successfully.
* Otherwise - The operation was not purged and is returned back
* @return @ref UCS_OK - The purged operation was handled successfully.
* Otherwise - The purged operation was handled with error and returned back

additional question - how should transport handle this error? what's the side effect of it for user?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should probably stop processing the next wqes?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it may lead to hang of upper layer (UCP), it looks like this status useless, UCT cannot react on this and must continue purging.
if UCP is not able to handle its requests, it looks like unrecoverable error on UCP side.

* to the transport.
*/
typedef void (*uct_ep_outstanding_purge_callback_t)(
typedef ucs_status_t (*uct_ep_outstanding_purge_callback_t)(
const uct_ep_op_info_t *op_info, void *arg);


Expand Down
3 changes: 2 additions & 1 deletion test/gtest/uct/ib/test_ib_mlx5_ext.cc
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,10 @@ class test_uct_ib_mlx5_ext_rc : public test_rc {
return UCS_OK;
}

static void purge_cb(const uct_ep_op_info_t*, void *arg)
static ucs_status_t purge_cb(const uct_ep_op_info_t*, void *arg)
{
*static_cast<bool*>(arg) = true;
return UCS_OK;
}

static void
Expand Down