> -----Original Message----- > From: Stokes, Ian > Sent: Friday, July 12, 2019 3:19 PM > To: Van Haaren, Harry <[email protected]>; [email protected] > Cc: [email protected]; [email protected] > Subject: Re: [PATCH v10 4/5] dpif-netdev: refactor generic implementation > > On 7/9/2019 1:34 PM, Harry van Haaren wrote: > > This commit refactors the generic implementation. The > > goal of this refactor is to simply the code to enable > 'simply' -> 'simplify'?
Simply done now :) > > -/* Returns a hash value for the bits of 'key' where there are 1-bits in > > - * 'mask'. */ > > -static inline uint32_t > > -netdev_flow_key_hash_in_mask(const struct netdev_flow_key *key, > > - const struct netdev_flow_key *mask) > > +VLOG_DEFINE_THIS_MODULE(dpif_lookup_generic); > > + > > +/* netdev_flow_key_flatten_unit: > > No need to mention function name in comments. Applies to other function > comments in the patch also. > > For function comments in OVS you can follow: > > http://docs.openvswitch.org/en/latest/internals/contributing/coding- > style/#functions Ah thanks - removed function names from comments. > > > + * Given a packet, table and mf_masks, this function iterates over each bit > > + * set in the subtable, and calculates the appropriate metadata to store in > the > > + * blocks_scratch[]. > > + * > > + * The results of the blocks_scratch[] can be used for hashing, and later > for > > + * verification of if a rule matches the given packet. > > + */ > > +static inline void > > +netdev_flow_key_flatten_unit(const uint64_t *pkt_blocks, > > + const uint64_t *tbl_blocks, > > + const uint64_t *mf_masks, > > + uint64_t *blocks_scratch, > > + const uint64_t pkt_mf_bits, > > + const uint32_t count) > > { > > - const uint64_t *p = miniflow_get_values(&mask->mf); > > - uint32_t hash = 0; > > - uint64_t value; > > + uint32_t i; > New line just to separate variable declaration form function body. Done. > > + for (i = 0; i < count; i++) { > > + uint64_t mf_mask = mf_masks[i]; > > + /* Calculate the block index for the packet metadata */ > > + uint64_t idx_bits = mf_mask & pkt_mf_bits; > > + const uint32_t pkt_idx = count_1bits(idx_bits); > > > > - NETDEV_FLOW_KEY_FOR_EACH_IN_FLOWMAP (value, key, mask->mf.map) { > > - hash = hash_add64(hash, value & *p); > > - p++; > > + /* check if the packet has the subtable miniflow bit set. If yes, > the > > For above and for other comments in the patch, capitalization at > beginning, period to finish. Fixed in a number of places. Kinda like Pokemon, tryna catch 'em all! <snip> > > + /* Hash the now linearized blocks of packet metadata */ > > + ULLONG_FOR_EACH_1(i, keys_map) { > > + uint32_t hash = 0; > > + uint32_t i_off = i * bit_count_total; > > + for (int h = 0; h < bit_count_total; h++) { > > + hash = hash_add64(hash, blocks_scratch[i_off + h]); > > + } > > + hashes[i] = hash_finish(hash, bit_count_total * 8); > > Can we replace magic 8 above. Done. Sizeof() with parenthesis required, failed to build without here. <snip> > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > > index 190cc8918..03ab5267a 100644 > > --- a/lib/dpif-netdev.c > > +++ b/lib/dpif-netdev.c > > @@ -233,6 +233,15 @@ struct dpcls { > > odp_port_t in_port; > > struct cmap subtables_map; > > struct pvector subtables; > > + > > + /* Region of memory for this DPCLS instance to use as scratch. > > + * Size is garaunteed to be large enough to hold all blocks required > for > 'garaunteed' -> 'guaranteed' Done. > > + subtable->mf_masks = xmalloc(sizeof(uint64_t) * (unit0 + unit1)); > > + netdev_flow_key_gen_masks(mask, subtable->mf_masks, unit0, unit1); > > + > > + /* Allocate blocks scratch space only if subtable requires more size > than > > + * is currently allocated. */ > > + const uint32_t blocks_required_per_pkt = unit0 + unit1; > > + if (cls->blocks_scratch_size < blocks_required_per_pkt) { > > + free(cls->blocks_scratch); > > + cls->blocks_scratch = xmalloc(sizeof(uint64_t) * NETDEV_MAX_BURST * > > + blocks_required_per_pkt); > > For sizeof above I don't think you need the () as it's part of an > expression. See OVS code standard below. This may apply to the xmalloc > for mf_masks above as well. > > The sizeof operator is unique among C operators in that it accepts two > very different kinds of operands: an expression or a type. In general, > prefer to specify an expression, e.g. int *x = xmalloc(sizeof *x);. When > the operand of sizeof is an expression, there is no need to parenthesize > that operand, and please don't. Hmmm, I get compile failures here with "sizeof uint64_t" instead of "sizeof(uint64_t)". There's also some ambiguity possible, see here: https://stackoverflow.com/a/26702432 In short, I've left the sizeof() with brackets, as code that compiles wins in the end. > <snip> > > > @@ -95,8 +97,18 @@ struct dpcls_subtable { > > * subtable matches on. The miniflow "bits" are used to select the > actual > > * dpcls lookup implementation at subtable creation time. > > */ > Although you can't see it here the comment above seems a little > misleading, maybe a copy and paste typo? > > "The lookup function to use for this subtable. From a technical point of > view, this is just an implementation of mutliple dispatch. The attribute > used to identify the ideal function is the miniflow fingerprint that the > subtable matches on. The miniflow "bits" are used to select the actual > dpcls lookup implementation at subtable creation time." > > The mf_bits_set_units are not the lookup function themselves. You could > probably re-word it as something like > > "Miniflow fingerprint that the subtable matches on. The miniflow "bits" > are used to select the actual dpcls lookup implementation at subtable > creation time" Updated to suggested comment - thanks. > 1 liner comment explaining the prototype comment would be nice here. > > +void > > +netdev_flow_key_gen_masks(const struct netdev_flow_key *tbl, > > + uint64_t *mf_masks, > > + const uint32_t mf_bits_u0, > > + const uint32_t mf_bits_u1); Done. Cheers, -H _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
