[ovs-dev] [PATCH] datapath: Avoid NULL mask check while building mask

Andy Zhou azhou at nicira.com
Fri Aug 8 20:56:57 UTC 2014


I agree with Daniele's comments.

Acked-by: Andy Zhou <azhou at nicira.com>

mask_set_nlattr() seems to have a single user. may be we cna remove
this function and
call set_nlattr() directly?

Not part of this patch, but set_nlattr() does not look correct in
handling nested attributes.


On Fri, Aug 8, 2014 at 9:25 AM, Daniele Di Proietto
<ddiproietto at vmware.com> wrote:
> Hi Pravin,
>
> couple of comments:
>
> - I would remove also the check for match->mask in update_range__().
> - I think braces are not necessary for if/else statements in the macros.
>
> Of course I would wait for review by Jesse or Andy, but other than that,
> LGTM.
>
> Acked-by: Daniele Di Proietto <ddiproietto at vmware.com>
>
>
> On 8/7/14, 9:26 PM, "Pravin B Shelar" <pshelar at nicira.com> wrote:
>
>>OVS does mask validation even if it does not need to convert
>>netlink mask attributes to mask structure.  ovs_nla_get_match()
>>caller can pass NULL mask structure pointer if the caller does
>>not need mask.  Therefore NULL check is required in SW_FLOW_KEY*
>>macros.  Following patch does not convert mask netlink attributes
>>if mask pointer is NULL, so we do not need these checks in
>>SW_FLOW_KEY* macro.
>>
>>Signed-off-by: Pravin B Shelar <pshelar at nicira.com>
>>---
>> datapath/flow_netlink.c | 74
>>++++++++++++++++++++++++++-----------------------
>> 1 file changed, 39 insertions(+), 35 deletions(-)
>>
>>diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
>>index 75172de..63ab7a6 100644
>>--- a/datapath/flow_netlink.c
>>+++ b/datapath/flow_netlink.c
>>@@ -84,8 +84,7 @@ static void update_range__(struct sw_flow_match *match,
>>               update_range__(match, offsetof(struct sw_flow_key, field),  \
>>                                    sizeof((match)->key->field), is_mask); \
>>               if (is_mask) {                                              \
>>-                      if ((match)->mask)                                  \
>>-                              (match)->mask->key.field = value;           \
>>+                      (match)->mask->key.field = value;                   \
>>               } else {                                                    \
>>                       (match)->key->field = value;                        \
>>               }                                                           \
>>@@ -95,10 +94,9 @@ static void update_range__(struct sw_flow_match *match,
>>       do { \
>>               update_range__(match, offset, len, is_mask);                \
>>               if (is_mask) {                                              \
>>-                      if ((match)->mask)                                  \
>>-                              memcpy((u8 *)&(match)->mask->key + offset, value_p, len);\
>>+                      memcpy((u8 *)&(match)->mask->key + offset, value_p, len);\
>>               } else {                                                    \
>>-                      memcpy((u8 *)(match)->key + offset, value_p, len);         \
>>+                      memcpy((u8 *)(match)->key + offset, value_p, len);  \
>>               }                                                           \
>>       } while (0)
>>
>>@@ -111,9 +109,8 @@ static void update_range__(struct sw_flow_match
>>*match,
>>               update_range__(match, offsetof(struct sw_flow_key, field),  \
>>                                    sizeof((match)->key->field), is_mask); \
>>               if (is_mask) {                                              \
>>-                      if ((match)->mask)                                  \
>>-                              memset((u8 *)&(match)->mask->key.field, value,\
>>-                                     sizeof((match)->mask->key.field));   \
>>+                      memset((u8 *)&(match)->mask->key.field, value,\
>>+                             sizeof((match)->mask->key.field));   \
>>               } else {                                                    \
>>                       memset((u8 *)&(match)->key->field, value,           \
>>                              sizeof((match)->key->field));                \
>>@@ -633,8 +630,7 @@ static int ovs_key_from_nlattrs(struct sw_flow_match
>>*match, u64 attrs,
>>
>>               SW_FLOW_KEY_PUT(match, eth.tci, tci, is_mask);
>>               attrs &= ~(1ULL << OVS_KEY_ATTR_VLAN);
>>-      } else if (!is_mask)
>>-              SW_FLOW_KEY_PUT(match, eth.tci, htons(0xffff), true);
>>+      }
>>
>>       if (attrs & (1ULL << OVS_KEY_ATTR_ETHERTYPE)) {
>>               __be16 eth_type;
>>@@ -859,8 +855,8 @@ static void mask_set_nlattr(struct nlattr *attr, u8
>>val)
>>  * attribute specifies the mask field of the wildcarded flow.
>>  */
>> int ovs_nla_get_match(struct sw_flow_match *match,
>>-                    const struct nlattr *key,
>>-                    const struct nlattr *mask)
>>+                    const struct nlattr *nla_key,
>>+                    const struct nlattr *nla_mask)
>> {
>>       const struct nlattr *a[OVS_KEY_ATTR_MAX + 1];
>>       const struct nlattr *encap;
>>@@ -870,7 +866,7 @@ int ovs_nla_get_match(struct sw_flow_match *match,
>>       bool encap_valid = false;
>>       int err;
>>
>>-      err = parse_flow_nlattrs(key, a, &key_attrs);
>>+      err = parse_flow_nlattrs(nla_key, a, &key_attrs);
>>       if (err)
>>               return err;
>>
>>@@ -911,35 +907,43 @@ int ovs_nla_get_match(struct sw_flow_match *match,
>>       if (err)
>>               return err;
>>
>>-      if (match->mask && !mask) {
>>-              /* Create an exact match mask. We need to set to 0xff all the
>>-               * 'match->mask' fields that have been touched in 'match->key'.
>>-               * We cannot simply memset 'match->mask', because padding bytes
>>-               * and fields not specified in 'match->key' should be left to 0.
>>-               * Instead, we use a stream of netlink attributes, copied from
>>-               * 'key' and set to 0xff: ovs_key_from_nlattrs() will take care
>>-               * of filling 'match->mask' appropriately.
>>-               */
>>-              newmask = kmemdup(key, nla_total_size(nla_len(key)),
>>-                                GFP_KERNEL);
>>-              if (!newmask)
>>-                      return -ENOMEM;
>>+      if (match->mask) {
>>+
>>+              if (!nla_mask) {
>>+                      /* Create an exact match mask. We need to set to 0xff
>>+                       * all the 'match->mask' fields that have been touched
>>+                       * in 'match->key'. We cannot simply memset
>>+                       * 'match->mask', because padding bytes and fields not
>>+                       * specified in 'match->key' should be left to 0.
>>+                       * Instead, we use a stream of netlink attributes,
>>+                       * copied from 'key' and set to 0xff: ovs_key_from_nlattrs()
>>+                       * will take care of filling 'match->mask' appropriately.
>>+                       */
>>+                      newmask = kmemdup(nla_key,
>>+                                        nla_total_size(nla_len(nla_key)),
>>+                                        GFP_KERNEL);
>>+                      if (!newmask)
>>+                              return -ENOMEM;
>>
>>-              mask_set_nlattr(newmask, 0xff);
>>+                      mask_set_nlattr(newmask, 0xff);
>>
>>-              /* The userspace does not send tunnel attributes that are 0,
>>-               * but we should not wildcard them nonetheless. */
>>-              if (match->key->tun_key.ipv4_dst)
>>-                      SW_FLOW_KEY_MEMSET_FIELD(match, tun_key, 0xff, true);
>>+                      /* The userspace does not send tunnel attributes that
>>+                       * are 0, but we should not wildcard them nonetheless.
>>+                       */
>>+                      if (match->key->tun_key.ipv4_dst)
>>+                              SW_FLOW_KEY_MEMSET_FIELD(match, tun_key,
>>+                                                       0xff, true);
>>
>>-              mask = newmask;
>>-      }
>>+                      nla_mask = newmask;
>>+              }
>>
>>-      if (mask) {
>>-              err = parse_flow_mask_nlattrs(mask, a, &mask_attrs);
>>+              err = parse_flow_mask_nlattrs(nla_mask, a, &mask_attrs);
>>               if (err)
>>                       goto free_newmask;
>>
>>+              /* Always match on tci. */
>>+              SW_FLOW_KEY_PUT(match, eth.tci, htons(0xffff), true);
>>+
>>               if (mask_attrs & 1ULL << OVS_KEY_ATTR_ENCAP) {
>>                       __be16 eth_type = 0;
>>                       __be16 tci = 0;
>>--
>>1.9.3
>>
>>_______________________________________________
>>dev mailing list
>>dev at openvswitch.org
>>https://urldefense.proofpoint.com/v1/url?u=http://openvswitch.org/mailman/
>>listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=MV9BdLjtFIdhBDBaw5z%2BU
>>6SSA2gAfY4L%2F1HCy3VjlKU%3D%0A&m=9WggZ4ZovrrQ5GRI9bcKsMv%2BY8JSaoZB1XECseA
>>iUS4%3D%0A&s=06b570a825ecaa3421698357c4647851c197cace2bf3cf1b28d6900c0e5c0
>>cc1
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev



More information about the dev mailing list