lib/mb_mgr_burst_async: cover full queue still processing case

KKaras169169 commented 9 months ago

It is possible for the MB_MGR job queue to have all its jobs busy, so submit_burst_and_check() will not be able to return number of completions greater than 0. In that case, instead of marking the queue as "empty", just return 0 and check if we ran into this corner case during job flush.

The change adds checking for MB_MGR job queue wrapping around and prevents the buffer from being marked "empty" if that happens, while job completions are still being returned.

Description

Changes to submit_burst_and_check() function prevent MB_MGR job buffer from being marked as "empty", while job completions are still being returned.

Changes to FLUSH_BURST() make it return 0 only when the job queue was marked as "empty", otherwise max_ret_jobs is set to the size of the queue.

Changes to kat-app introduce a test case for burst API, which simulates a situation, where job buffer is populated by jobs that are still being processed (one job is marked as completed to simulate that completions are still being returned).

Affected parts

[x] Library
[x] Test Application
[ ] Perf Application
[ ] Other: (please specify)

Motivation and Context

This change avoids losing a whole job buffer by marking it prematurely "empty".

How Has This Been Tested?

This has been first detected in SPDK project. The application used DPDK's crypto driver ipsec_mb to enqueue crypto operations for a period of time, which at some point lead to the MB_MGR queue being full of jobs that are still being processed, and since passing the check state->earliest_job == state->next_job in submit_burst_and_check() marks the job queue as "empty" that lead to a number of jobs never returning completion. The difference was discovered by comparing ipsec_mb_qp->rte_ring producer/consumer counters (they became equal at the end of application run) and qpair's enqueued/dequeued counters.

Additionally change was tested using included unit test.

Types of changes

[x] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

[ ] My code follows the code style of this project.
[ ] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.

I am not sure about the "My code follows the code style of this project.", because make spellcheck returns errors in files that i have not touched (test/kat-app/snow3g_test.c, lib/avx2_t1/mb_mgr_zuc_submit_flush_avx2.asm and lib/include/snow3g_common.h) and checkpatch.pl complains about using multiple spaces as indents instead of tabs, even though the files I have modified also do that in other places.

mdcornu commented 9 months ago

Thanks for the PR. We are looking into this issue and will get back to you soon.

Regards, Marcel

mdcornu commented 9 months ago

Thank you for you contribution, your commit has been merged here https://github.com/intel/intel-ipsec-mb/commit/11b210f1e9d2f1398b9094d8042bf28a70fb8d2e.

Please note that some other updates were needed to fully address this issue so the new behavior of submit_burst_and_check() is to flush N jobs once the queue is full (where N is the number of jobs submitted). This is consistent with submit_job_and_check() which also flushes once the queue is full.

Closing the PR now. If you have any questions / feedback, feel free to open an issue.

Thanks, Marcel

intel / intel-ipsec-mb