| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <3E334895-91D5-4FAC-99B0-A0B3B9B547E7@redhat.com>
Date: Thu, 30 Jul 2020 15:51:57 +0200
From: "Eelco Chaudron" <echaudro@...hat.com>
To: "Tonghao Zhang" <xiangxia.m.yue@...il.com>
Cc: "Linux Kernel Network Developers" <netdev@...r.kernel.org>,
"David Miller" <davem@...emloft.net>,
"ovs dev" <dev@...nvswitch.org>,
"Jakub Kicinski" <kuba@...nel.org>,
"Paolo Abeni" <pabeni@...hat.com>,
"Pravin Shelar" <pshelar@....org>,
"Florian Westphal" <fw@...len.de>
Subject: Re: [PATCH net-next v3 2/2] net: openvswitch: make masks cache size
configurable
On 30 Jul 2020, at 15:25, Tonghao Zhang wrote:
> On Thu, Jul 30, 2020 at 8:33 PM Eelco Chaudron <echaudro@...hat.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@...hat.com>
>>> wrote:
>>>>
>>>> This patch makes the masks cache size configurable, or with
>>>> a size of 0, disable it.
>>>>
>>>> Reviewed-by: Paolo Abeni <pabeni@...hat.com>
>>>> Signed-off-by: Eelco Chaudron <echaudro@...hat.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.
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@...il.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
Powered by blists - more mailing lists