Hey Sai, Thanks for the patch. Comments inlined. I'm okay with the rest.
Alin. > -----Mesaj original----- > De la: dev [mailto:dev-boun...@openvswitch.org] În numele Sairam > Venugopal > Trimis: Wednesday, March 30, 2016 3:44 AM > Către: dev@openvswitch.org > Subiect: [ovs-dev] [PATCH 2/3] datapath-windows: Update flow lookup to > support RecircId and DpHash > > Update the OvsLookupFlow to include RecircId and DpHash in its flow > comparison. Revert the keyLen related changes until they are aligned > appropriately. > > Signed-off-by: Sairam Venugopal <vsai...@vmware.com> > --- > datapath-windows/ovsext/Flow.c | 31 ++++++++++++++++++++++--------- > 1 file changed, 22 insertions(+), 9 deletions(-) > > diff --git a/datapath-windows/ovsext/Flow.c b/datapath- > windows/ovsext/Flow.c index 02c41b7..0d48963 100644 > --- a/datapath-windows/ovsext/Flow.c > +++ b/datapath-windows/ovsext/Flow.c > @@ -1382,12 +1382,10 @@ _MapKeyAttrToFlowPut(PNL_ATTR *keyAttrs, > > if (keyAttrs[OVS_KEY_ATTR_RECIRC_ID]) { > destKey->recircId = > NlAttrGetU32(keyAttrs[OVS_KEY_ATTR_RECIRC_ID]); > - destKey->l2.keyLen += sizeof(destKey->recircId); > } > > if (keyAttrs[OVS_KEY_ATTR_DP_HASH]) { > destKey->dpHash = > NlAttrGetU32(keyAttrs[OVS_KEY_ATTR_DP_HASH]); > - destKey->l2.keyLen += sizeof(destKey->dpHash); > } > > /* ===== L2 headers ===== */ > @@ -1765,12 +1763,10 @@ OvsGetFlowMetadata(OvsFlowKey *key, > > if (keyAttrs[OVS_KEY_ATTR_RECIRC_ID]) { > key->recircId = NlAttrGetU32(keyAttrs[OVS_KEY_ATTR_RECIRC_ID]); > - key->l2.keyLen += sizeof(key->recircId); > } > > if (keyAttrs[OVS_KEY_ATTR_DP_HASH]) { > key->dpHash = NlAttrGetU32(keyAttrs[OVS_KEY_ATTR_DP_HASH]); > - key->l2.keyLen += sizeof(key->dpHash); > } > > return status; > @@ -2032,7 +2028,7 @@ OvsExtractFlow(const NET_BUFFER_LIST *packet, } > > __inline BOOLEAN > -FlowEqual(UINT64 *src, UINT64 *dst, UINT32 size) > +FlowMemoryEqual(UINT64 *src, UINT64 *dst, UINT32 size) > { > UINT32 i; > ASSERT((size & 0x7) == 0); > @@ -2046,6 +2042,20 @@ FlowEqual(UINT64 *src, UINT64 *dst, UINT32 size) > return TRUE; > } > > +__inline BOOLEAN > +FlowEqual(OvsFlow *flow, const OvsFlowKey *key, UINT64 hash) { [Alin Gabriel Serdean: ] it is confusing flow, key can be renamed to source, destination or src/dst > + UINT16 size = key->l2.keyLen; > + UINT16 offset = key->l2.offset; > + UINT64 *src = (UINT64 *)((UINT8 *)&flow->key + offset); > + UINT64 *dst = (UINT64 *)((UINT8 *)key + offset); [Alin Gabriel Serdean: ] Although a new function looks cleaner, it does not make sense to recompute/restore the abose since you already had the values in OvsLookupFlow > + > + return (flow->hash == hash && > + flow->key.l2.val == key->l2.val && > + flow->key.recircId == key->recircId && > + flow->key.dpHash == key->dpHash && > + FlowMemoryEqual(src, dst, size)); } > > /* > * > ---------------------------------------------------------------------------- > @@ -2133,6 +2143,12 @@ OvsLookupFlow(OVS_DATAPATH *datapath, > > if (!hashValid) { > *hash = OvsJhashBytes(start, size, 0); > + if (key->recircId) { > + *hash = OvsJhashWords((UINT32*)hash, 1, key->recircId); > + } > + if (key->dpHash) { > + *hash = OvsJhashWords((UINT32*)hash, 1, key->dpHash); > + } > } > > head = &datapath->flowTable[HASH_BUCKET(*hash)]; > @@ -2140,10 +2156,7 @@ OvsLookupFlow(OVS_DATAPATH *datapath, > while (link != head) { > OvsFlow *flow = CONTAINING_RECORD(link, OvsFlow, ListEntry); > > - if (flow->hash == *hash && > - flow->key.l2.val == key->l2.val && > - FlowEqual((UINT64 *)((uint8 *)&flow->key + offset), > - (UINT64 *)start, size)) { > + if (FlowEqual(flow, key, *hash)) { > return flow; > } > link = link->Flink; > -- > 2.5.0.windows.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
