[vpp] Dedicated binary-API socket and thread for asynchronous VPP events#1951
[vpp] Dedicated binary-API socket and thread for asynchronous VPP events#1951nicholasching wants to merge 7 commits into
Conversation
|
|
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
f42fbc6 to
f3ab3f9
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
It appears the checks are failing due to an upstream issue which should be resolved when #4696 is merged into sonic-swss. |
yue-fred-gao
left a comment
There was a problem hiding this comment.
Please trigger t1-lag-vpp sanity through PR in sonic-buildimage with this PR branch.
| if (now >= hard_deadline || now >= idle_deadline) \ | ||
| break; \ | ||
| if (scm && scm->socket_enable) { \ | ||
| vl_socket_client_read (1); \ |
There was a problem hiding this comment.
Miss the code to extend idle_deadline
_wr_rv = vl_socket_client_read (1);
if (_wr_rv == 0) {
idle_deadline = vat_time_now (vam) + 1.0;
if (idle_deadline > hard_deadline)
idle_deadline = hard_deadline;
}
| uword *ptr; | ||
|
|
||
| sw_if_index = htonl(mp->sw_if_index); | ||
| sw_if_index = ntohl(mp->sw_if_index); |
There was a problem hiding this comment.
Good catch. Not sure how it works today or it never worked
| @@ -2972,21 +3160,23 @@ int vpp_get_interface_speed (const char *hwif_name, uint32_t *speed) | |||
|
|
|||
| int vpp_sync_for_events () | |||
There was a problem hiding this comment.
Please add a comment that event handler running under EVENT_LOCK should never acquire VPP_LOCK to avoid cross lock.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
The change looks good. If t1-lag-vpp sanity passes, I will approve the PR |
|
Thanks @yue-fred-gao. Test running now; should be ready by tomorrow morning! |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
The t1-lag-vpp sanity test has passed on the associated sonic-buildimage PR: sonic-net/sonic-buildimage#28145 I have attached the results below: |
|
/azpw run |
|
Retrying failed(or canceled) jobs... |
|
Retrying failed(or canceled) stages in build 1163729: ✅Stage TestAsan:
✅Stage Test:
|
|
/azpw run |
|
Retrying failed(or canceled) jobs... |
|
Retrying failed(or canceled) stages in build 1163729: ✅Stage TestAsan:
✅Stage Test:
|
|
/azpw run |
|
Retrying failed(or canceled) jobs... |
|
Retrying failed(or canceled) stages in build 1163729: ✅Stage TestAsan:
✅Stage Test:
|
|
Hello @saiarcot895 @lolyu! The changes in this PR have previously been reviewed by @yue-fred-gao, received final approval from @dypet, and are now passing checks. Our team at Cisco currently does not have anyone with merge permissions. When you have a chance, could you please take a quick look and help us merge into master for SAIVPP UT feature? |
Signed-off-by: Nicholas Ching <nicholaslching@gmail.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
The t1-lag-vpp sanity test, applied to both #1951 and #1952 has passed on the associated sonic-buildimage PR: sonic-net/sonic-buildimage#28145 I have attached the results below: |
Description of PR
Summary:
Part of the SAIVPP unit-test framework (
docker-sai-test-vpp, #1950, see itsdevdocs/). Carries the asynchronous VPP event stream on a dedicated binary-API socket and background thread, separate from the synchronous request/reply command path. This resolves a mutex deadlock between the command path and the async event path that blocked bring-up of the OCPsai_testsuite atPORT_COUNT=32. Reviewers can start invslib/vpp/vppxlate/SaiVppXlate.c.Fixes # (N/A — no upstream issue)
Type of change
Approach
What is the motivation for this PR?
The synchronous command path and the asynchronous VPP event stream shared a single binary-API socket and the global
__plugin_msg_base. At scale this deadlocked between the command path and the async event path, preventing the standalone SAIVPP UT (SaiSanityTest) from passing.How did you do it?
In
vslib/vpp/vppxlate/SaiVppXlate.c: introduced a dedicated event binary-API socket (event_socket_client_main/vat_event_main) and background thread; added event-connection message macros (M_EV/PING_EV/S_EV/WR_EV) that operate on the explicit per-connection socket via thevl_socket_client_*2()APIs; added a separate lock for the event connection; and made__plugin_msg_basethread-local so the command and event threads cannot race between setting the base and the followingM()/PING().How did you verify/test it?
Built into the
docker-sai-test-vppimage and run against the OCPsai_testsuite; the command-vs-event deadlock at 32 ports no longer occurs andSaiSanityTestpasses. The change is orthogonal to the recently-merged route-flow-counter context-table rework (#1894) and ipinip support (#1860); rebased on currentmasterand verified both coexist.Any platform specific information?
VPP platform only (
vslib/vpp/). No change to the synchronous command path or SAI semantics; the event connection is additive.Documentation
Development notes for this work are in
.azure-pipelines/docker-sai-test-vpp/devdocs/in #1950.