[ovs-dev] [PATCH v10 4/5] dpif-netdev: refactor generic implementation

Van Haaren, Harry harry.van.haaren at intel.com
Fri Jul 12 16:20:06 UTC 2019


> -----Original Message-----
> From: Stokes, Ian
> Sent: Friday, July 12, 2019 3:19 PM
> To: Van Haaren, Harry <harry.van.haaren at intel.com>; dev at openvswitch.org
> Cc: i.maximets at samsung.com; malvika.gupta at arm.com
> 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


More information about the dev mailing list