[ovs-dev] [PATCH net-next v3 2/2] net: openvswitch: make masks cache size configurable

Eelco Chaudron echaudro at redhat.com
Thu Jul 30 14:28:47 UTC 2020



On 30 Jul 2020, at 16:23, Tonghao Zhang wrote:

> On Thu, Jul 30, 2020 at 9:52 PM Eelco Chaudron <echaudro at redhat.com> 
> wrote:
>>
>>
>>
>> On 30 Jul 2020, at 15:25, Tonghao Zhang wrote:
>>
>>> On Thu, Jul 30, 2020 at 8:33 PM Eelco Chaudron <echaudro at redhat.com>
>>> wrote:
>>>>
>>>> Thanks Tonghao for the review, see comments inline below!
>>>>
>>>> If I get no more reviews by the end of the week I’ll send out a 
>>>> v4.
>>>>
>>>> //Eelco
>>>>
>>>>
>>>> On 30 Jul 2020, at 7:07, Tonghao Zhang wrote:
>>>>
>>>>> On Wed, Jul 29, 2020 at 9:27 PM Eelco Chaudron 
>>>>> <echaudro at redhat.com>
>>>>> wrote:
>>>>>>
>>>>>> This patch makes the masks cache size configurable, or with
>>>>>> a size of 0, disable it.
>>>>>>
>>>>>> Reviewed-by: Paolo Abeni <pabeni at redhat.com>
>>>>>> Signed-off-by: Eelco Chaudron <echaudro at redhat.com>
>>>>>> ---
>>>>>> Changes in v3:
>>>>>>  - Use is_power_of_2() function
>>>>>>  - Use array_size() function
>>>>>>  - Fix remaining sparse errors
>>>>>>
>>>>>> Changes in v2:
>>>>>>  - Fix sparse warnings
>>>>>>  - Fix netlink policy items reported by Florian Westphal
>>>>>>
>>>>>>  include/uapi/linux/openvswitch.h |    1
>>>>>>  net/openvswitch/datapath.c       |   14 +++++
>>>>>>  net/openvswitch/flow_table.c     |   97
>>>>>> +++++++++++++++++++++++++++++++++-----
>>>>>>  net/openvswitch/flow_table.h     |   10 ++++
>>>>>>  4 files changed, 108 insertions(+), 14 deletions(-)
>>>>>>
>>>>>> diff --git a/include/uapi/linux/openvswitch.h
>>>>>> b/include/uapi/linux/openvswitch.h
>>>>>> index 7cb76e5ca7cf..8300cc29dec8 100644
>>>>>> --- a/include/uapi/linux/openvswitch.h
>>>>>> +++ b/include/uapi/linux/openvswitch.h
>>>>>> @@ -86,6 +86,7 @@ enum ovs_datapath_attr {
>>>>>>         OVS_DP_ATTR_MEGAFLOW_STATS,     /* struct
>>>>>> ovs_dp_megaflow_stats */
>>>>>>         OVS_DP_ATTR_USER_FEATURES,      /* OVS_DP_F_*  */
>>>>>>         OVS_DP_ATTR_PAD,
>>>>>> +       OVS_DP_ATTR_MASKS_CACHE_SIZE,
>>>>>>         __OVS_DP_ATTR_MAX
>>>>>>  };
>>>>>>
>>>>>> diff --git a/net/openvswitch/datapath.c
>>>>>> b/net/openvswitch/datapath.c
>>>>>> index a54df1fe3ec4..114b2ddb8037 100644
>>>>>> --- a/net/openvswitch/datapath.c
>>>>>> +++ b/net/openvswitch/datapath.c
>>>>>> @@ -1498,6 +1498,7 @@ static size_t ovs_dp_cmd_msg_size(void)
>>>>>>         msgsize += nla_total_size_64bit(sizeof(struct
>>>>>> ovs_dp_stats));
>>>>>>         msgsize += nla_total_size_64bit(sizeof(struct
>>>>>> ovs_dp_megaflow_stats));
>>>>>>         msgsize += nla_total_size(sizeof(u32)); /*
>>>>>> OVS_DP_ATTR_USER_FEATURES */
>>>>>> +       msgsize += nla_total_size(sizeof(u32)); /*
>>>>>> OVS_DP_ATTR_MASKS_CACHE_SIZE */
>>>>>>
>>>>>>         return msgsize;
>>>>>>  }
>>>>>> @@ -1535,6 +1536,10 @@ static int ovs_dp_cmd_fill_info(struct
>>>>>> datapath *dp, struct sk_buff *skb,
>>>>>>         if (nla_put_u32(skb, OVS_DP_ATTR_USER_FEATURES,
>>>>>> dp->user_features))
>>>>>>                 goto nla_put_failure;
>>>>>>
>>>>>> +       if (nla_put_u32(skb, OVS_DP_ATTR_MASKS_CACHE_SIZE,
>>>>>> +                       
>>>>>> ovs_flow_tbl_masks_cache_size(&dp->table)))
>>>>>> +               goto nla_put_failure;
>>>>>> +
>>>>>>         genlmsg_end(skb, ovs_header);
>>>>>>         return 0;
>>>>>>
>>>>>> @@ -1599,6 +1604,13 @@ static int ovs_dp_change(struct datapath
>>>>>> *dp,
>>>>>> struct nlattr *a[])
>>>>>>  #endif
>>>>>>         }
>>>>>>
>>>>>> +       if (a[OVS_DP_ATTR_MASKS_CACHE_SIZE]) {
>>>>>> +               u32 cache_size;
>>>>>> +
>>>>>> +               cache_size =
>>>>>> nla_get_u32(a[OVS_DP_ATTR_MASKS_CACHE_SIZE]);
>>>>>> +               ovs_flow_tbl_masks_cache_resize(&dp->table,
>>>>>> cache_size);
>>>>> Do we should return error code, if we can't change the 
>>>>> "mask_cache"
>>>>> size ? for example, -EINVAL, -ENOMEM
>>>>
>>>> Initially, I did not do this due to the fact the new value is
>>>> reported,
>>>> and on failure, the old value is shown.
>>>> However thinking about it again, it makes more sense to return an
>>>> error.
>>>> Will sent a v4 with the following to return:
>>>>
>>>> -
>>>> -void ovs_flow_tbl_masks_cache_resize(struct flow_table *table, u32
>>>> size)
>>>> +int ovs_flow_tbl_masks_cache_resize(struct flow_table *table, u32
>>>> size)
>>>>   {
>>>>          struct mask_cache *mc = 
>>>> rcu_dereference(table->mask_cache);
>>>>          struct mask_cache *new;
>>>>
>>>>          if (size == mc->cache_size)
>>>> -               return;
>>>> +               return 0;
>>>> +
>>>> +       if (!is_power_of_2(size) && size != 0)
>>>> +               return -EINVAL;
>>> add check as below, and we can remove the check in
>>> tbl_mask_cache_alloc function. That
>>> function will only return NULL, if there is no memory. And we can
>>> comment for tbl_mask_cache_alloc, to indicate that size should be a
>>> power of 2.
>>>  if ((!is_power_of_2(size) && size != 0) ||
>>>            (size * sizeof(struct mask_cache_entry)) >
>>> PCPU_MIN_UNIT_SIZE)
>>
>> I do not think the extra check hurts as it’s not in any fast path.
> Hi Eelco.
> Yes, I agree. Sorry for not being clear. I mean that:
> if we don't check the "(size * sizeof(struct mask_cache_entry))  >
> PCPU_MIN_UNIT_SIZE)" in the ovs_flow_tbl_masks_cache_resize.
> tbl_mask_cache_alloc (ovs_flow_tbl_masks_cache_resize will invoke this
> function)will return the NULL,
> ovs_flow_tbl_masks_cache_resize will return -ENOMEM. I guess we should
> return -EINVAL in that case.

Thanks got it now, will add the additional check, and not remove it from 
tbl_mask_cache_alloc().

>> The comment would easily be missed, especially if someone changes the
>> default value, so I would prefer to keep it as is.
>>
>>> Thanks Eelco.
>>> Reviewed-by: Tonghao Zhang <xiangxia.m.yue at gmail.com>
>>>
>>>>          new = tbl_mask_cache_alloc(size);
>>>>          if (!new)
>>>> -               return;
>>>> +               return -ENOMEM;
>>>>
>>>>          rcu_assign_pointer(table->mask_cache, new);
>>>>          call_rcu(&mc->rcu, mask_cache_rcu_cb);
>>>> +
>>>> +       return 0;
>>>>   }
>>>>
>>>>>> +       }
>>>>>> +
>>>>>>         dp->user_features = user_features;
>>>>>>
>>>>>>         if (dp->user_features & OVS_DP_F_TC_RECIRC_SHARING)
>>>>>> @@ -1894,6 +1906,8 @@ static const struct nla_policy
>>>>>> datapath_policy[OVS_DP_ATTR_MAX + 1] = {
>>>>>>         [OVS_DP_ATTR_NAME] = { .type = NLA_NUL_STRING, .len =
>>>>>> IFNAMSIZ - 1 },
>>>>>>         [OVS_DP_ATTR_UPCALL_PID] = { .type = NLA_U32 },
>>>>>>         [OVS_DP_ATTR_USER_FEATURES] = { .type = NLA_U32 },
>>>>>> +       [OVS_DP_ATTR_MASKS_CACHE_SIZE] =  
>>>>>> NLA_POLICY_RANGE(NLA_U32,
>>>>>> 0,
>>>>>> +               PCPU_MIN_UNIT_SIZE / sizeof(struct
>>>>>> mask_cache_entry)),
>>>>>>  };
>>>>>>
>>>>>>  static const struct genl_ops dp_datapath_genl_ops[] = {
>>>>>> diff --git a/net/openvswitch/flow_table.c
>>>>>> b/net/openvswitch/flow_table.c
>>>>>> index a5912ea05352..5280aeeef628 100644
>>>>>> --- a/net/openvswitch/flow_table.c
>>>>>> +++ b/net/openvswitch/flow_table.c
>>>>>> @@ -38,8 +38,8 @@
>>>>>>  #define MASK_ARRAY_SIZE_MIN    16
>>>>>>  #define REHASH_INTERVAL                (10 * 60 * HZ)
>>>>>>
>>>>>> +#define MC_DEFAULT_HASH_ENTRIES        256
>>>>>>  #define MC_HASH_SHIFT          8
>>>>>> -#define MC_HASH_ENTRIES                (1u << MC_HASH_SHIFT)
>>>>>>  #define MC_HASH_SEGS           ((sizeof(uint32_t) * 8) /
>>>>>> MC_HASH_SHIFT)
>>>>>>
>>>>>>  static struct kmem_cache *flow_cache;
>>>>>> @@ -341,15 +341,75 @@ static void flow_mask_remove(struct
>>>>>> flow_table
>>>>>> *tbl, struct sw_flow_mask *mask)
>>>>>>         }
>>>>>>  }
>>>>>>
>>>>>> +static void __mask_cache_destroy(struct mask_cache *mc)
>>>>>> +{
>>>>>> +       if (mc->mask_cache)
>>>>>> +               free_percpu(mc->mask_cache);
>>>>> free_percpu the NULL is safe. we can remove the "if".
>>>>
>>>> Makes sense, will remove the if() check.
>>>>
>>>>>> +       kfree(mc);
>>>>>> +}
>>>>>> +
>>>>>> +static void mask_cache_rcu_cb(struct rcu_head *rcu)
>>>>>> +{
>>>>>> +       struct mask_cache *mc = container_of(rcu, struct
>>>>>> mask_cache,
>>>>>> rcu);
>>>>>> +
>>>>>> +       __mask_cache_destroy(mc);
>>>>>> +}
>>>>>> +
>>>>>> +static struct mask_cache *tbl_mask_cache_alloc(u32 size)
>>>>>> +{
>>>>>> +       struct mask_cache_entry __percpu *cache = NULL;
>>>>>> +       struct mask_cache *new;
>>>>>> +
>>>>>> +       /* Only allow size to be 0, or a power of 2, and does not
>>>>>> exceed
>>>>>> +        * percpu allocation size.
>>>>>> +        */
>>>>>> +       if ((!is_power_of_2(size) && size != 0) ||
>>>>>> +           (size * sizeof(struct mask_cache_entry)) >
>>>>>> PCPU_MIN_UNIT_SIZE)
>>>>>> +               return NULL;
>>>>>> +       new = kzalloc(sizeof(*new), GFP_KERNEL);
>>>>>> +       if (!new)
>>>>>> +               return NULL;
>>>>>> +
>>>>>> +       new->cache_size = size;
>>>>>> +       if (new->cache_size > 0) {
>>>>>> +               cache = __alloc_percpu(array_size(sizeof(struct
>>>>>> mask_cache_entry),
>>>>>> +                                                 
>>>>>> new->cache_size),
>>>>>> +                                      __alignof__(struct
>>>>>> mask_cache_entry));
>>>>>> +               if (!cache) {
>>>>>> +                       kfree(new);
>>>>>> +                       return NULL;
>>>>>> +               }
>>>>>> +       }
>>>>>> +
>>>>>> +       new->mask_cache = cache;
>>>>>> +       return new;
>>>>>> +}
>>>>>> +
>>>>>> +void ovs_flow_tbl_masks_cache_resize(struct flow_table *table, 
>>>>>> u32
>>>>>> size)
>>>>>> +{
>>>>>> +       struct mask_cache *mc = 
>>>>>> rcu_dereference(table->mask_cache);
>>>>>> +       struct mask_cache *new;
>>>>>> +
>>>>>> +       if (size == mc->cache_size)
>>>>>> +               return;
>>>>>> +
>>>>>> +       new = tbl_mask_cache_alloc(size);
>>>>>> +       if (!new)
>>>>>> +               return;
>>>>>> +
>>>>>> +       rcu_assign_pointer(table->mask_cache, new);
>>>>>> +       call_rcu(&mc->rcu, mask_cache_rcu_cb);
>>>>>> +}
>>>>>> +
>>>>>>  int ovs_flow_tbl_init(struct flow_table *table)
>>>>>>  {
>>>>>>         struct table_instance *ti, *ufid_ti;
>>>>>> +       struct mask_cache *mc;
>>>>>>         struct mask_array *ma;
>>>>>>
>>>>>> -       table->mask_cache = __alloc_percpu(sizeof(struct
>>>>>> mask_cache_entry) *
>>>>>> -                                          MC_HASH_ENTRIES,
>>>>>> -                                          __alignof__(struct
>>>>>> mask_cache_entry));
>>>>>> -       if (!table->mask_cache)
>>>>>> +       mc = tbl_mask_cache_alloc(MC_DEFAULT_HASH_ENTRIES);
>>>>>> +       if (!mc)
>>>>>>                 return -ENOMEM;
>>>>>>
>>>>>>         ma = tbl_mask_array_alloc(MASK_ARRAY_SIZE_MIN);
>>>>>> @@ -367,6 +427,7 @@ int ovs_flow_tbl_init(struct flow_table 
>>>>>> *table)
>>>>>>         rcu_assign_pointer(table->ti, ti);
>>>>>>         rcu_assign_pointer(table->ufid_ti, ufid_ti);
>>>>>>         rcu_assign_pointer(table->mask_array, ma);
>>>>>> +       rcu_assign_pointer(table->mask_cache, mc);
>>>>>>         table->last_rehash = jiffies;
>>>>>>         table->count = 0;
>>>>>>         table->ufid_count = 0;
>>>>>> @@ -377,7 +438,7 @@ int ovs_flow_tbl_init(struct flow_table 
>>>>>> *table)
>>>>>>  free_mask_array:
>>>>>>         __mask_array_destroy(ma);
>>>>>>  free_mask_cache:
>>>>>> -       free_percpu(table->mask_cache);
>>>>>> +       __mask_cache_destroy(mc);
>>>>>>         return -ENOMEM;
>>>>>>  }
>>>>>>
>>>>>> @@ -453,9 +514,11 @@ void ovs_flow_tbl_destroy(struct flow_table
>>>>>> *table)
>>>>>>  {
>>>>>>         struct table_instance *ti = 
>>>>>> rcu_dereference_raw(table->ti);
>>>>>>         struct table_instance *ufid_ti =
>>>>>> rcu_dereference_raw(table->ufid_ti);
>>>>>> +       struct mask_cache *mc = 
>>>>>> rcu_dereference(table->mask_cache);
>>>>>> +       struct mask_array *ma =
>>>>>> rcu_dereference_ovsl(table->mask_array);
>>>>>>
>>>>>> -       free_percpu(table->mask_cache);
>>>>>> -       call_rcu(&table->mask_array->rcu, mask_array_rcu_cb);
>>>>>> +       call_rcu(&mc->rcu, mask_cache_rcu_cb);
>>>>>> +       call_rcu(&ma->rcu, mask_array_rcu_cb);
>>>>>>         table_instance_destroy(table, ti, ufid_ti, false);
>>>>>>  }
>>>>>>
>>>>>> @@ -724,6 +787,7 @@ struct sw_flow
>>>>>> *ovs_flow_tbl_lookup_stats(struct
>>>>>> flow_table *tbl,
>>>>>>                                           u32 *n_mask_hit,
>>>>>>                                           u32 *n_cache_hit)
>>>>>>  {
>>>>>> +       struct mask_cache *mc = rcu_dereference(tbl->mask_cache);
>>>>>>         struct mask_array *ma = rcu_dereference(tbl->mask_array);
>>>>>>         struct table_instance *ti = rcu_dereference(tbl->ti);
>>>>>>         struct mask_cache_entry *entries, *ce;
>>>>>> @@ -733,7 +797,7 @@ struct sw_flow
>>>>>> *ovs_flow_tbl_lookup_stats(struct
>>>>>> flow_table *tbl,
>>>>>>
>>>>>>         *n_mask_hit = 0;
>>>>>>         *n_cache_hit = 0;
>>>>>> -       if (unlikely(!skb_hash)) {
>>>>>> +       if (unlikely(!skb_hash || mc->cache_size == 0)) {
>>>>>>                 u32 mask_index = 0;
>>>>>>                 u32 cache = 0;
>>>>>>
>>>>>> @@ -749,11 +813,11 @@ struct sw_flow
>>>>>> *ovs_flow_tbl_lookup_stats(struct flow_table *tbl,
>>>>>>
>>>>>>         ce = NULL;
>>>>>>         hash = skb_hash;
>>>>>> -       entries = this_cpu_ptr(tbl->mask_cache);
>>>>>> +       entries = this_cpu_ptr(mc->mask_cache);
>>>>>>
>>>>>>         /* Find the cache entry 'ce' to operate on. */
>>>>>>         for (seg = 0; seg < MC_HASH_SEGS; seg++) {
>>>>>> -               int index = hash & (MC_HASH_ENTRIES - 1);
>>>>>> +               int index = hash & (mc->cache_size - 1);
>>>>>>                 struct mask_cache_entry *e;
>>>>>>
>>>>>>                 e = &entries[index];
>>>>>> @@ -867,6 +931,13 @@ int ovs_flow_tbl_num_masks(const struct
>>>>>> flow_table *table)
>>>>>>         return READ_ONCE(ma->count);
>>>>>>  }
>>>>>>
>>>>>> +u32 ovs_flow_tbl_masks_cache_size(const struct flow_table 
>>>>>> *table)
>>>>>> +{
>>>>>> +       struct mask_cache *mc = 
>>>>>> rcu_dereference(table->mask_cache);
>>>>>> +
>>>>>> +       return READ_ONCE(mc->cache_size);
>>>>>> +}
>>>>>> +
>>>>>>  static struct table_instance *table_instance_expand(struct
>>>>>> table_instance *ti,
>>>>>>                                                     bool ufid)
>>>>>>  {
>>>>>> @@ -1095,8 +1166,8 @@ void ovs_flow_masks_rebalance(struct
>>>>>> flow_table
>>>>>> *table)
>>>>>>         for (i = 0; i < masks_entries; i++) {
>>>>>>                 int index = masks_and_count[i].index;
>>>>>>
>>>>>> -               new->masks[new->count++] =
>>>>>> -                       rcu_dereference_ovsl(ma->masks[index]);
>>>>>> +               if (ovsl_dereference(ma->masks[index]))
>>>>>> +                       new->masks[new->count++] =
>>>>>> ma->masks[index];
>>>>>>         }
>>>>>>
>>>>>>         rcu_assign_pointer(table->mask_array, new);
>>>>>> diff --git a/net/openvswitch/flow_table.h
>>>>>> b/net/openvswitch/flow_table.h
>>>>>> index 325e939371d8..f2dba952db2f 100644
>>>>>> --- a/net/openvswitch/flow_table.h
>>>>>> +++ b/net/openvswitch/flow_table.h
>>>>>> @@ -27,6 +27,12 @@ struct mask_cache_entry {
>>>>>>         u32 mask_index;
>>>>>>  };
>>>>>>
>>>>>> +struct mask_cache {
>>>>>> +       struct rcu_head rcu;
>>>>>> +       u32 cache_size;  /* Must be ^2 value. */
>>>>>> +       struct mask_cache_entry __percpu *mask_cache;
>>>>>> +};
>>>>>> +
>>>>>>  struct mask_count {
>>>>>>         int index;
>>>>>>         u64 counter;
>>>>>> @@ -53,7 +59,7 @@ struct table_instance {
>>>>>>  struct flow_table {
>>>>>>         struct table_instance __rcu *ti;
>>>>>>         struct table_instance __rcu *ufid_ti;
>>>>>> -       struct mask_cache_entry __percpu *mask_cache;
>>>>>> +       struct mask_cache __rcu *mask_cache;
>>>>>>         struct mask_array __rcu *mask_array;
>>>>>>         unsigned long last_rehash;
>>>>>>         unsigned int count;
>>>>>> @@ -77,6 +83,8 @@ int ovs_flow_tbl_insert(struct flow_table 
>>>>>> *table,
>>>>>> struct sw_flow *flow,
>>>>>>                         const struct sw_flow_mask *mask);
>>>>>>  void ovs_flow_tbl_remove(struct flow_table *table, struct 
>>>>>> sw_flow
>>>>>> *flow);
>>>>>>  int  ovs_flow_tbl_num_masks(const struct flow_table *table);
>>>>>> +u32  ovs_flow_tbl_masks_cache_size(const struct flow_table
>>>>>> *table);
>>>>>> +void ovs_flow_tbl_masks_cache_resize(struct flow_table *table, 
>>>>>> u32
>>>>>> size);
>>>>>>  struct sw_flow *ovs_flow_tbl_dump_next(struct table_instance
>>>>>> *table,
>>>>>>                                        u32 *bucket, u32 *idx);
>>>>>>  struct sw_flow *ovs_flow_tbl_lookup_stats(struct flow_table *,
>>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Best regards, Tonghao
>>>>
>>>
>>>
>>> --
>>> Best regards, Tonghao
>>
>
>
> --
> Best regards, Tonghao



More information about the dev mailing list