Thanks for sending out the fix.
I¹m ok with one of the fixes. The other one needs to be reworked to use
one of the approaches consistently. Pls. see inlined comments.
-- Nithin
-----Original Message-----
From: dev <dev-boun...@openvswitch.org> on behalf of Sairam Venugopal
<vsai...@vmware.com>
Date: Friday, March 25, 2016 at 11:06 AM
To: "dev@openvswitch.org" <dev@openvswitch.org>
Subject: [ovs-dev] [PATCH v2] datapath-windows: Update Recirculation to
use the right parameters
>Update OvsLookupFlow() to include flowKey->RecircId when computing hash.
>Use the right source port Id for checking if a packet is received or
>transmitted.
>
>Signed-off-by: Sairam Venugopal <vsai...@vmware.com>
>---
> datapath-windows/ovsext/Actions.c | 2 +-
> datapath-windows/ovsext/Flow.c | 3 +++
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
>diff --git a/datapath-windows/ovsext/Actions.c
>b/datapath-windows/ovsext/Actions.c
>index a91454d..7742096 100644
>--- a/datapath-windows/ovsext/Actions.c
>+++ b/datapath-windows/ovsext/Actions.c
>@@ -2015,7 +2015,7 @@ OvsDoRecirc(POVS_SWITCH_CONTEXT switchContext,
> }
> status = OvsCreateAndAddPackets(NULL, 0, OVS_PACKET_CMD_MISS,
> vport, key, ovsFwdCtx.curNbl,
>- srcPortNo ==
>+
>ovsFwdCtx.fwdDetail->SourcePortId ==
>
>switchContext->virtualExternalPortId,
This comparison determines the value for ŒisRecv¹ parameter. ŒisRecv¹
determines whether the OOB data should be interpreted as receive data or
send data. So, the existing code is checking for:
srcPortNo == switchContext->virtualExternalPortId
Left side data is a UINT32 and right side data is a NDIS_SWITCH_PORT_ID.
So, clearly this comparison is not right since we are comparing different
types. They are not expected to have the same value even if they are
representing the same vport.
The proposed fix at least makes sure that we are comparing the right
types. So, I¹d go with it. Is the comparison right? It seems so. Basically
we want to determine if the packet came from a VIF or a physical NIC.
ŒfwdDetail¹ is a reliable way of doing it. Only way this could mess up is
if we mean to explicitly change the ŒsrcPortNo¹ during tunneling. In such
cases, the 'fwdDetail->SourcePortId¹ will end up different from the
ŒsrcPortNo¹. This is exactly why we use ŒsrcPortNo¹ for comparison around
the code to allow flexibility.
In any case, the problematic case here is tunneling + recirc. We can deal
with that in a subsequent patch. For now, this is the right fix.
I¹d also add a XXX comment to investigate that "tunneling + recirc² case
in the future.
> &ovsFwdCtx.layers,
> ovsFwdCtx.switchContext,
>diff --git a/datapath-windows/ovsext/Flow.c
>b/datapath-windows/ovsext/Flow.c
>index 02c41b7..d49697c 100644
>--- a/datapath-windows/ovsext/Flow.c
>+++ b/datapath-windows/ovsext/Flow.c
>@@ -2133,6 +2133,9 @@ OvsLookupFlow(OVS_DATAPATH *datapath,
>
> if (!hashValid) {
> *hash = OvsJhashBytes(start, size, 0);
>+ if (key->recircId) {
>+ *hash = OvsJhashWords((UINT32*)hash, 1, key->recircId);
>+ }
Ok, we have two options:
1. Increment ŒkeyLen¹ and include recircId as part of it. So, while
hashing we make sure that recircId gets included.
2. Explicitly add Œkey->recirId¹ during hashing.
I¹m ok with either way. The ŒkeyLen¹ approach is more efficient for now,
since it avoids a Œif¹ check and also an additional function call. In the
future, if we have a lot of meta data such as ŒrecircId¹, then we should
consider adding a Œkey->metaDataLen¹ and go from there. For now, I¹d add a
comment in ŒOvsFlowKey¹ to set future direction, and go with the ŒkeyLen¹
approach.
Of course, there¹s a bug when ŒkeyLen¹ gets set in _MapKeyAttrToFlowPut().
The value gets incremented due to ŒrecircId¹ but gets reset again.
> }
>
> head = &datapath->flowTable[HASH_BUCKET(*hash)];
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
