Attaching XDP to VF #3651
Attaching XDP to VF #3651
Conversation
| @@ -78,6 +78,7 @@ typedef struct CXPLAT_DATAPATH { | |||
| typedef struct CXPLAT_INTERFACE { | |||
| CXPLAT_LIST_ENTRY Link; | |||
| uint32_t IfIndex; | |||
| uint32_t ActualIfIndex; | |||
There was a problem hiding this comment.
IMO we should do this the other way around: we should track each interface's IfIndex and also an optional VfIfIndex. We should then not have any VFs directly in the interface table, and only explicitly VF-aware code needs to use the VfIfIndex.
There was a problem hiding this comment.
And if we can't find the VF for an interface, we could just set VfIfIndex to the regular IfIndex value, or some magic value like IFI_UNSPECIFIED.
There was a problem hiding this comment.
Currently I'm not sure where the conventional IfIndex should be used and so use ActualIfIndex everywhere.
If we use VfIfIndex as VF-aware code, there whould require if-else where code refers IfIndex
src/platform/datapath_raw_xdp.c
Outdated
| for (int i = 0; i < (int) pIfTable->NumEntries; i++) { | ||
| MIB_IF_ROW2* pIfRow = &pIfTable->Table[i]; | ||
| if (pIfRow->InterfaceAndOperStatusFlags.FilterInterface || | ||
| !pIfRow->InterfaceAndOperStatusFlags.HardwareInterface || |
There was a problem hiding this comment.
Do we need the HardwareInterface or ConnectorPresent checks?
There was a problem hiding this comment.
Empirically these three flags are useful for filter out useless interfaces. Not sure other corner case
src/platform/datapath_raw_xdp.c
Outdated
| // NOTE: O(n^2), but realistically enough small | ||
| for (int j = 0; j < IfRowsCount; j++) { | ||
| struct IfMatching* pMatching = &IfMatching[j]; | ||
| if (pIfRow->PhysicalMediumType == NdisPhysicalMedium802_3 && |
There was a problem hiding this comment.
Do we need to look at the physical mediums? This strikes me as being potentially very fragile.
There was a problem hiding this comment.
This is also empirical point of view. However it should be impossible to identify which is VF if we don't use the PhysicalMediumType
|
Oh, test issue was only on my dev system. something corrupted in my side |
src/platform/datapath_raw_xdp.c
Outdated
|
|
||
| CXPLAT_FRE_ASSERT(pIfTable->NumEntries <= 128); | ||
| BOOLEAN Initialized[128] = {0}; | ||
| for (int i = 0; i < (int) pIfTable->NumEntries; i++) { |
There was a problem hiding this comment.
Using the MIB interface table for the outer loop could cause us to create XDP interfaces for interfaces that don't have an IP address at all, right? I am still leaning towards keeping the same outer loop (over adapter addresses) to pick which interfaces to use, but then use the MIB table for the inner loop to find an interface with a matching physical address.
There was a problem hiding this comment.
I thought it is covered by first flag check.
I was not 100% sure whether these flags is for whether having IP address or not. just empirically verified this check works (duonic and NetVSC and VF can pass through)
if (pIfRow->InterfaceAndOperStatusFlags.FilterInterface ||
!pIfRow->InterfaceAndOperStatusFlags.HardwareInterface ||
!pIfRow->InterfaceAndOperStatusFlags.ConnectorPresent ||
Initialized[i]) if (Adapter->IfType == IF_TYPE_ETHERNET_CSMACD &&
Adapter->OperStatus == IfOperStatusUp &&
Adapter->PhysicalAddressLength == ETH_MAC_ADDR_LEN)There was a problem hiding this comment.
You meant use GetAdaptersAddresses as outer loop?
There was a problem hiding this comment.
Yes, with GAA as outer loop. But if you really think based on your testing this is the way to go, feel free to "won't fix" this comment.
There was a problem hiding this comment.
I think the idea works. let me quickly push it.
src/platform/datapath_raw_xdp.c
Outdated
| CXPLAT_FRE_ASSERT(pIfTable->NumEntries <= 128); | ||
| BOOLEAN Initialized[128] = {0}; |
There was a problem hiding this comment.
I'm a bit unsure, but do we really need this array? I don't like the hard coded limit, but it seems more like an optimization that's not really necessary.
There was a problem hiding this comment.
I want to use std::bitset or heap allocated array. It is possible to remove this by checking Xdp->Interfaces linked list at beginning of outer loop whether there is adapter with same Mac address is already registered.
x is same mac address (VF and NetVSC) merged as one interface, then later
outer |--->x |
inner | ----->x |
outer | -----o--> |
inner | |
o is skipped because it was marked as Initialized earlier
There was a problem hiding this comment.
I'm still not sure why we need to keep this at all? It's not a big deal if you search the whole array N^2, since it's so small.
There was a problem hiding this comment.
Can be removed by using GAA for outer loop
Description
If XDP runs on VM with SR-IOV, trying to attach XDP to VF
Testing
Send/Recv is failing
Documentation
N/A