[PATCH v1] validation: fix classification test init #2282
Conversation
This file contains hidden or 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
The assertion checks for exactly one queue. Changed this to check for non-zero number of queues. Signed-off-by: Satheesh Paul <psatheesh@marvell.com>
odpbuild
changed the title
| ts.pktio = create_pktio(ODP_QUEUE_TYPE_SCHED, pkt_pool, cls == ENABLE_CLS); | ||
| CU_ASSERT_FATAL(ts.pktio != ODP_PKTIO_INVALID); | ||
| CU_ASSERT(odp_pktin_event_queue(ts.pktio, &ts.pktin_queue, 1) == 1); | ||
| CU_ASSERT(odp_pktin_event_queue(ts.pktio, &ts.pktin_queue, 1) > 0); |
Collaborator
There was a problem hiding this comment.
The assertion checks for exactly one queue. Changed this to check for non-zero number of queues.
Why? What is wrong with the current code? If there is a valid reason for the change (but I cannot immediately see it), the commit message could say it.
Collaborator
There was a problem hiding this comment.
This assertion to check for exactly one queue was added recently. Before this, on cn9k, cn10k classificati>on tests have been working fine with 64 queues. This change breaks them. Is there any reason the tests need to run with exactly one queue?
Failure of a validation test may be caused by a bug in the validation test or by a bug in the SW under testing and therefore a test failure alone is not a sufficient reason to change a validation test. If we took care of all test failures by changing the tests, then the tests would be useless.
If an API validation test does not have bugs and uses the API correctly, then a test failure implies that there is problem in the SW being tested. It may also be that the failure exposes a problem in the API itself, but then the API needs a fix too, not just the test.
Looking at the code, test_init() calls create_pktio() which creates a pktio and configures it with the default number of pktin queues, i.e. 1. So checking then that odp_pktin_event_queue() returns exactly 1 seems right to me.
So why are you proposing this change?
Contributor
Author
|
This assertion to check for exactly one queue was added recently. Before this, on cn9k, cn10k classification tests have been working fine with 64 queues. This change breaks them. Is there any reason the tests need to run with exactly one queue?
________________________________
From: JannePeltonen ***@***.***>
Sent: Tuesday, October 21, 2025 12:10
To: OpenDataPlane/odp ***@***.***>
Cc: Satheesh Paul Antonysamy ***@***.***>; Author ***@***.***>
Subject: [EXTERNAL] Re: [OpenDataPlane/odp] [PATCH v1] validation: fix classification test init (PR #2282)
@ JannePeltonen commented on this pull request. In test/validation/api/classification/odp_classification_test_pmr. c: > @@ -129,7 +129,7 @@ static test_state_t test_init(cls_flag_t cls) ts. pktio = create_pktio(ODP_QUEUE_TYPE_SCHED, pkt_pool,
ZjQcmQRYFpfptBannerStart
Prioritize security for external emails:
Confirm sender and content safety before clicking links or opening attachments
Report Suspicious<https://us-phishalarm-ewt.proofpoint.com/EWT/v1/CRVmXkqW!t23WffX_bP_yfiO3M_aQzZdwk5l1kSkYVa4_JC0C4z9tE-8SwiPdry34JEyymDs0cNlUhaC9nBNerZaUC-quYqynmJlubo0ZKC54QIdHtPU5oJE$>
ZjQcmQRYFpfptBannerEnd
@JannePeltonen commented on this pull request.
________________________________
In test/validation/api/classification/odp_classification_test_pmr.c<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_OpenDataPlane_odp_pull_2282-23discussion-5Fr2446870067&d=DwMFaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=HxMha0TKwbmAHhy_WqGXDLbYWewiHegkGYj-LSIBew8&m=RMWNc2V7pAbgG4_sUUIezy0Uz_A-8ASHLTe6dndIX_k3I-qGHSmXMG51M1tJ7AWN&s=79fo_hD-LeXv5e_ktBprfKl0a99mp_Rn_rPl7DI3mEA&e=>:
@@ -129,7 +129,7 @@ static test_state_t test_init(cls_flag_t cls)
ts.pktio = create_pktio(ODP_QUEUE_TYPE_SCHED, pkt_pool, cls == ENABLE_CLS);
CU_ASSERT_FATAL(ts.pktio != ODP_PKTIO_INVALID);
- CU_ASSERT(odp_pktin_event_queue(ts.pktio, &ts.pktin_queue, 1) == 1);
+ CU_ASSERT(odp_pktin_event_queue(ts.pktio, &ts.pktin_queue, 1) > 0);
The assertion checks for exactly one queue. Changed this to check for non-zero number of queues.
Why? What is wrong with the current code? If there is a valid reason for the change (but I cannot immediately see it), the commit message could say it.
—
Reply to this email directly, view it on GitHub<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_OpenDataPlane_odp_pull_2282-23pullrequestreview-2D3358940451&d=DwMFaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=HxMha0TKwbmAHhy_WqGXDLbYWewiHegkGYj-LSIBew8&m=RMWNc2V7pAbgG4_sUUIezy0Uz_A-8ASHLTe6dndIX_k3I-qGHSmXMG51M1tJ7AWN&s=sRSWIleEmjCPvd8Bnpav9O9X3Gnxsngwya2cNEsrLMk&e=>, or unsubscribe<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AR5BJ7QAYI3TSICPQLKPMQL3YXINDAVCNFSM6AAAAACJX3WIUSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTGNJYHE2DANBVGE&d=DwMFaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=HxMha0TKwbmAHhy_WqGXDLbYWewiHegkGYj-LSIBew8&m=RMWNc2V7pAbgG4_sUUIezy0Uz_A-8ASHLTe6dndIX_k3I-qGHSmXMG51M1tJ7AWN&s=WkrT3gv72mBvSPYTLqXvyqeMzMC7Gn7GJGFJ2z48eWA&e=>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
Collaborator
|
Btw, it might be better to reply to review comments through github web interface. If you comment through email, then please try to avoid top posting. |
Contributor
Author
|
________________________________
From: JannePeltonen ***@***.***>
Sent: Tuesday, October 21, 2025 14:26
To: OpenDataPlane/odp ***@***.***>
Cc: Satheesh Paul Antonysamy ***@***.***>; Author ***@***.***>
Subject: [EXTERNAL] Re: [OpenDataPlane/odp] [PATCH v1] validation: fix classification test init (PR #2282)
@ JannePeltonen commented on this pull request. In test/validation/api/classification/odp_classification_test_pmr. c: > @@ -129,7 +129,7 @@ static test_state_t test_init(cls_flag_t cls) ts. pktio = create_pktio(ODP_QUEUE_TYPE_SCHED, pkt_pool,
ZjQcmQRYFpfptBannerStart
Prioritize security for external emails:
Confirm sender and content safety before clicking links or opening attachments
Report Suspicious<https://us-phishalarm-ewt.proofpoint.com/EWT/v1/CRVmXkqW!t23WffX_bP_yfiO3M_aQzZdwk5l1kSkYVa4_JC0C4z9tE-8SwiPdry3ZpIyzONsUt2hnmzneEUTRX-JiGckp9-tPJf4vtjIeVX5qHudgVPvc1Qw$>
ZjQcmQRYFpfptBannerEnd
@JannePeltonen commented on this pull request.
________________________________
In test/validation/api/classification/odp_classification_test_pmr.c<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_OpenDataPlane_odp_pull_2282-23discussion-5Fr2447335577&d=DwMFaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=HxMha0TKwbmAHhy_WqGXDLbYWewiHegkGYj-LSIBew8&m=aAHDPdD5TqLcXiqO3YSRAOpw9tFlisjnd-w1UUDZK1pMMky9bgNIpRVz0vuz5Tmw&s=kVbUOSWZYj_oIcqNGJpV4yJ3SyIZrKyw3i1IvTqxMCI&e=>:
@@ -129,7 +129,7 @@ static test_state_t test_init(cls_flag_t cls)
ts.pktio = create_pktio(ODP_QUEUE_TYPE_SCHED, pkt_pool, cls == ENABLE_CLS);
CU_ASSERT_FATAL(ts.pktio != ODP_PKTIO_INVALID);
- CU_ASSERT(odp_pktin_event_queue(ts.pktio, &ts.pktin_queue, 1) == 1);
+ CU_ASSERT(odp_pktin_event_queue(ts.pktio, &ts.pktin_queue, 1) > 0);
This assertion to check for exactly one queue was added recently. Before this, on cn9k, cn10k classificati>on tests have been working fine with 64 queues. This change breaks them. Is there any reason the tests need to run with exactly one queue?
Failure of a validation test may be caused by a bug in the validation test or by a bug in the SW under testing and therefore a test failure alone is not a sufficient reason to change a validation test. If we took care of all test failures by changing the tests, then the tests would be useless.
If an API validation test does not have bugs and uses the API correctly, then a test failure implies that there is problem in the SW being tested. It may also be that the failure exposes a problem in the API itself, but then the API needs a fix too, not just the test.
Looking at the code, test_init() calls create_pktio() which creates a pktio and configures it with the default number of pktin queues, i.e. 1. So checking then that odp_pktin_event_queue() returns exactly 1 seems right to me.
So why are you proposing this change?
—
Yes, odp_pktin_event_queue() does not seem to honor the default value from create_pktio(). We are checking the implementation on this.
Also, the github interface to reply to comment is not working for me (always says "There was an error posting your comment". Apologies for the clutter.
Reply to this email directly, view it on GitHub<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_OpenDataPlane_odp_pull_2282-23discussion-5Fr2447335577&d=DwMFaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=HxMha0TKwbmAHhy_WqGXDLbYWewiHegkGYj-LSIBew8&m=aAHDPdD5TqLcXiqO3YSRAOpw9tFlisjnd-w1UUDZK1pMMky9bgNIpRVz0vuz5Tmw&s=kVbUOSWZYj_oIcqNGJpV4yJ3SyIZrKyw3i1IvTqxMCI&e=>, or unsubscribe<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AR5BJ7QOAMMR5RH2FTT75L33YXYKBAVCNFSM6AAAAACJX3WIUSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTGNJZGUZDAOJSGA&d=DwMFaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=HxMha0TKwbmAHhy_WqGXDLbYWewiHegkGYj-LSIBew8&m=aAHDPdD5TqLcXiqO3YSRAOpw9tFlisjnd-w1UUDZK1pMMky9bgNIpRVz0vuz5Tmw&s=dIKzKB24m9Z0GhjJ9zVWybtGWOOYUia0TwhHN723AiA&e=>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
The assertion checks for exactly one queue. Changed this to
check for non-zero number of queues.
Signed-off-by: Satheesh Paul psatheesh@marvell.com