> On Dec 19, 2016, at 2:43 PM, antonio.fische...@intel.com wrote: > > This patch skips the chunk comprising of dp_hash and in_port in the > subtable mask when the dpcls refers to a physical port. This will > slightly speed up the hash computation as one expensive function call > to hash_add64() can be skipped. >
Do you have any performance test results to share? > The idea behind is that the packets coming from a physical port > shall have the same 8-bytes chunk in their dp_hash/in_port portion of > the miniflow. When hash is computed in the NIC, dp_hash is zero leaving > only the in_port in the chunk. This doesn't contribute effectively in > spreading hash values and avoiding collisions. > This approach could be extended to other port types. At first this seems too hacky for me. However, since the dpcls is explicitly selected for the in_port, it makes sense to wildcard the in_port in the dpcls lookup itself. This has nothing to do with the port type. For dp_hash, it would typically not be matched, so there is no need to worry about it. Also, when it is matched (after recirculation), the in_port value would be the same as before recirculation, so it is not safe to assume anything about dp_hash based on the in_port value. Proposals for improvement below. > > Signed-off-by: Antonio Fischetti <antonio.fische...@intel.com> > Signed-off-by: Bhanuprakash Bodireddy <bhanuprakash.bodire...@intel.com> > Co-authored-by: Bhanuprakash Bodireddy bhanuprakash.bodire...@intel.com > --- > lib/dpif-netdev.c | 28 ++++++++++++++++++++++++++-- > 1 file changed, 26 insertions(+), 2 deletions(-) > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index 1a47a45..cd8715f 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -1856,18 +1856,34 @@ netdev_flow_key_from_flow(struct netdev_flow_key *dst, > /* Initialize a netdev_flow_key 'mask' from 'match'. */ > static inline void > netdev_flow_mask_init(struct netdev_flow_key *mask, > - const struct match *match) > + const struct match *match, > + char * port_type) It’s better to handle all this in the caller and not modify this function at all. > { > uint64_t *dst = miniflow_values(&mask->mf); > struct flowmap fmap; > uint32_t hash = 0; > size_t idx; > + bool phy_port = false; > + > + if (port_type && !strcmp(port_type, "dpdk")) { > + phy_port = true; > + } > > /* Only check masks that make sense for the flow. */ > flow_wc_map(&match->flow, &fmap); > flowmap_init(&mask->mf.map); > + /* Check that dp_hash and in_port must be into the same structure chunk. > */ > + BUILD_ASSERT_DECL(offsetof(struct flow, dp_hash)/sizeof(*dst) == > + offsetof(struct flow, in_port)/sizeof(*dst)); > +#define DPHASH_INPORT_MAP_IDX (offsetof(struct flow, dp_hash)/sizeof(*dst)) > > FLOWMAP_FOR_EACH_INDEX(idx, fmap) { > + if (phy_port && idx == DPHASH_INPORT_MAP_IDX) { > + /* This chunk contains Sw-Hash and Port Nr. As all packets > coming > + * from the same physical port have the same in_port value > + * and dp_hash set to 0, this 8-bytes chunk will be ignored. */ > + continue; > + } > uint64_t mask_u64 = flow_u64_value(&match->wc.masks, idx); > > if (mask_u64) { > @@ -2278,7 +2294,15 @@ dp_netdev_flow_add(struct dp_netdev_pmd_thread *pmd, > struct dpcls *cls; > odp_port_t in_port = match->flow.in_port.odp_port; > > - netdev_flow_mask_init(&mask, match); > + struct dpif_port dpifport; > + > + if (pmd && pmd->dp) { > + const struct dpif * pdpif = pmd->dp->dpif; > + if (pdpif) { > + dpif_netdev_port_query_by_number(pdpif, in_port, &dpifport); > + } > + } > + netdev_flow_mask_init(&mask, match, dpifport.type); Instead of the above you could move the in_port.odp_port assert later in the function to be done before the netdev_flow_mask_init() call, and then after the assert zero it out, and after the netdev_flow_mask_init() make it all-ones again. This has the effect of always wildcarding in_port in the dpcls, which is explicitly selected based on the port number. Then, in the typical case where dp_hash is also wildcarded, the end result is the same as what you intended with this patch. Jarno > /* Make sure wc does not have metadata. */ > ovs_assert(!FLOWMAP_HAS_FIELD(&mask.mf.map, metadata) > && !FLOWMAP_HAS_FIELD(&mask.mf.map, regs)); > -- > 2.4.11 > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev