Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ON-16265: Set no-poison vi flag when ctpio_mode=sf-np #62

Open
wants to merge 1 commit into
base: v8_1
Choose a base branch
from

Conversation

dchadwic-xilinx
Copy link
Contributor

Since ctpio_mode can be <0, using a signed int is more appropriate. (https://github.com/Xilinx-CNS/tcpdirect/blob/tcpdirect-9.0.0/src/lib/zf/private/stack_alloc.c#L52-L54)

We also decided that there is no obvious reason for the no-poison flag to be set in the event that ctpio_mode < 0. My assumption is that there was an earlier decision that any future 'no-poisoning' CTPIO modes would also have negative values like CTPIO_MODE_SF_NP=-1.

Another solution would be to use an enum but I do not see examples of this being used elsewhere in this codebase. This solution is the least disruptive imo.

@dchadwic-xilinx dchadwic-xilinx requested a review from a team as a code owner January 16, 2025 15:49
Copy link
Contributor

@jfeather-amd jfeather-amd left a comment

Choose a reason for hiding this comment

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

Looks reasonable, have you done any testing? It would be nice to see if you could force CTPIO poisoning with CTPIO_MODE_SF then show that no poisoning happens with CTPIO_MODE_SF_NP although I don't know how easy/plausible that would be to do.

@jfeather-amd jfeather-amd requested a review from a team January 16, 2025 17:27
@dchadwic-xilinx
Copy link
Contributor Author

dchadwic-xilinx commented Jan 16, 2025

Testing was done here: proving that the vi_flags are now showing correctly 3000000 rather than 1000000. I'll have a go at forcing poisoning.

Copy link
Contributor

@jfeather-amd jfeather-amd left a comment

Choose a reason for hiding this comment

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

proving that the vi_flags are now showing correctly 3000000 rather than 1000000

This is enough to make me happy, so approving on that basis.

I'll have a go at forcing poisoning

I'd still be interested in the result of this just for my own curiosity, but don't feel that you need to do it - at this point it's more a test of whether the VI flag EF_VI_TX_CTPIO_NO_POISON works!

@jfeather-amd jfeather-amd requested a review from a team January 17, 2025 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants