From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.1 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 852EBC433DF for ; Thu, 30 Jul 2020 13:52:12 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 59D1C2074B for ; Thu, 30 Jul 2020 13:52:12 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="Ky83k/iM" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727072AbgG3NwL (ORCPT ); Thu, 30 Jul 2020 09:52:11 -0400 Received: from us-smtp-delivery-1.mimecast.com ([207.211.31.120]:53227 "EHLO us-smtp-1.mimecast.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1726281AbgG3NwK (ORCPT ); Thu, 30 Jul 2020 09:52:10 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1596117127; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=G23vA5r0luxFgGiDURx8wbx3CfnDxWc9m2Cf6hvKm/g=; b=Ky83k/iMTNY+5NhPUwqHgKdCViAHgW/486u5Y66nxDgCutR81So4P+9arjmxXv16+C77wN b7kh99SJn4m3WzkFLwk+EsJQ7OmW/Tp0ZXyAvd2PE3+uSOCOOLzTEvLVE2gdv1/kDJ8KUA VJgjvaCV7VOcybMPfjCUek/rFdO+kJU= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-445-Jl5dvUwiMiqlcPzS_ovUiA-1; Thu, 30 Jul 2020 09:52:03 -0400 X-MC-Unique: Jl5dvUwiMiqlcPzS_ovUiA-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 6FC3A106B245; Thu, 30 Jul 2020 13:52:01 +0000 (UTC) Received: from [10.36.114.39] (ovpn-114-39.ams2.redhat.com [10.36.114.39]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 886C487B13; Thu, 30 Jul 2020 13:51:59 +0000 (UTC) From: "Eelco Chaudron" To: "Tonghao Zhang" Cc: "Linux Kernel Network Developers" , "David Miller" , "ovs dev" , "Jakub Kicinski" , "Paolo Abeni" , "Pravin Shelar" , "Florian Westphal" Subject: Re: [PATCH net-next v3 2/2] net: openvswitch: make masks cache size configurable Date: Thu, 30 Jul 2020 15:51:57 +0200 Message-ID: <3E334895-91D5-4FAC-99B0-A0B3B9B547E7@redhat.com> In-Reply-To: References: <159602912600.937753.3123982828905970322.stgit@ebuild> <159602917888.937753.17718463910615059058.stgit@ebuild> MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On 30 Jul 2020, at 15:25, Tonghao Zhang wrote: > On Thu, Jul 30, 2020 at 8:33 PM Eelco Chaudron > 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 >>> wrote: >>>> >>>> This patch makes the masks cache size configurable, or with >>>> a size of 0, disable it. >>>> >>>> Reviewed-by: Paolo Abeni >>>> Signed-off-by: Eelco Chaudron >>>> --- >>>> 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 > >> 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