Comments inlined.
Thanks, Alin. > -----Mesaj original----- > 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. > [Alin Gabriel Serdean: ] from the header ntddndis.h: typedef UINT32 NDIS_SWITCH_PORT_ID, *PNDIS_SWITCH_PORT_ID; I don't think it is a problem of type but a problem with the number itself. > 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. [Alin Gabriel Serdean: ] fwdDetail is not reliable in our case because we could have cloned the nbl and not update the OOB data and this is not what we want to do in our case. We want to reprocess the packet as it came from the same input port (https://github.com/openvswitch/ovs/blob/master/ofproto/ofproto-dpif-rid.h#L64-L69) Indeed, there is a problem because srcportno == vport->portno and we should use in our case vport->portId when doing the comparison. I'll send out a patch to update it. > > > 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. [Alin Gabriel Serdean: ] I would go with option one so we maintain the same of hashing we have at the moment. > > 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 _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
