-
Notifications
You must be signed in to change notification settings - Fork 22
ON-16435: Replace arch checks with nic capabilies #75
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
base: master
Are you sure you want to change the base?
Conversation
This makes it easier to change whether these flags are in zf_stack or zf_stack_impl. Keeping them in the latter struct may have impact on cache hit rate in the hotpath where these flags may be queried.
| pkt_id next_packet_id = 0; | ||
| char* next_packet; | ||
| bool is_efct = vi->nic_type.arch == EF_VI_ARCH_EFCT; | ||
| bool is_rx_ref = *zf_stack_res_nic_flags(st, nic) & |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're using these flags on the critical path. The zf_stack_impl is used for cold path state. If we need the checks on the hot path that information should move to zf_stack_nic. That might just be the RX_REF capability - probably needs a bit of care to ensure that this is done in a way that doesn't impact performance.
| zf_assert_nequal(vi->nic_type.arch, EF_VI_ARCH_EFCT); | ||
| zf_assert_nequal(vi->nic_type.arch, EF_VI_ARCH_EF10CT); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assert that CTPIO_ONLY capability wasn't set?
| if( vi->vi_flags & EF_VI_TX_CTPIO && vi->nic_type.arch != EF_VI_ARCH_EFCT | ||
| && vi->nic_type.arch != EF_VI_ARCH_EF10CT ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check here is on whether we need to post a dma fallback, which we don't if we have the CTPIO_ONLY capability, so I think that's what we should use.
The final commit is probably not what we want but I am at a bit of a loss for what capabilities should be used in those cases. I therefore added ef10ct arch checks. Opening this as a draft because of this.
I'm also wondering about the performance implications of this change. It queries the flags in the hot path of RX and TX. I wonder if it's therefore worth moving the location of these flags into
zf_stack::nic. Or perhaps we should usevi::nic_type::nic_flagsinstead for the capability checks.Testing C-Model
zfsinkzfsendAll packets received at the other end. I had to modify zfsend to sleep 1000us between sends (not sure of the sends are batched) because otherwise the sends would fail and zfsend would hit an assertion. This is fine because the C-Model cannot keep up with the sends.