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

Andy Zhou azhou at nicira.com
Fri Aug 8 23:46:38 UTC 2014


On Fri, Aug 8, 2014 at 3:58 PM, Pravin Shelar <pshelar at nicira.com> wrote:
> On Fri, Aug 8, 2014 at 1:56 PM, Andy Zhou <azhou at nicira.com> wrote:
>> I agree with Daniele's comments.
>>
>> Acked-by: Andy Zhou <azhou at nicira.com>
>>
> Thanks I pushed patch to master.
>
>> 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.
>>
> can you elaborate on that? It should handle ovs key attributes.

The minor issue is that the name of the 3rd argument
"is_attr_mask_key" seems wrong. We first pass in with 'true' then
change to 'false' for nested attribute does not make sense.  It is
still part of mask.

The main issue is that this logic does not handle genuine multiple
level of nested attributes. For example, encap within tunnel.  OVS
does not support ittoday but Geneve does allow for outer header to
have a vlan tag.

If the issue is to handle variable length key types, such as GENEVE
options, we should try to tell that apart from the nested ones, may be
using key_len of -2?

Make sense?



>
>>
>> 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