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

2020-07-24 Thread Eelco Chaudron 



On 23 Jul 2020, at 18:42, Jakub Kicinski wrote:


On Thu, 23 Jul 2020 14:58:31 +0200 Eelco Chaudron wrote:

+   if ((size & (size - 1)) != 0 ||


is_power_of_2() ?


Was not aware of this macro, will replace…



+   (size * sizeof(struct mask_cache_entry)) > PCPU_MIN_UNIT_SIZE)
+   return NULL;



+   new->cache_size = size;
+   if (new->cache_size > 0) {
+   cache = __alloc_percpu(sizeof(struct mask_cache_entry) *
+  new->cache_size,


array_size() ?


Will change to:

cache = __alloc_percpu(array_size(
   sizeof(struct mask_cache_entry),
   new->cache_size),
   __alignof__(struct mask_cache_entry));
if (!cache) {



+  __alignof__(struct mask_cache_entry));
+


No need for the new line here


Will remove


+   if (!cache) {
+   kfree(new);
+   return NULL;
+   }
+   }



+   if (size == mc->cache_size || (size & (size - 1)) != 0)


why check "is power of 2" twice?


Will remove as it will always call tbl_mask_cache_alloc() which has the 
check above.




@@ -454,7 +516,7 @@ 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);


-   free_percpu(table->mask_cache);
+   call_rcu(>mask_cache->rcu, mask_cache_rcu_cb);
call_rcu(>mask_array->rcu, mask_array_rcu_cb);
table_instance_destroy(table, ti, ufid_ti, false);
 }


This adds a new warning :(

net/openvswitch/flow_table.c:519:24: warning: incorrect type in 
argument 1 (different address spaces)
net/openvswitch/flow_table.c:519:24:expected struct callback_head 
*head
net/openvswitch/flow_table.c:519:24:got struct callback_head 
[noderef] __rcu *


:( Yes my diff with older did not work… Sure I’ve fixed them all now 
:)



Will sent out a V3 after some testing…

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

2020-07-23 Thread Jakub Kicinski 
On Thu, 23 Jul 2020 14:58:31 +0200 Eelco Chaudron wrote:
> + if ((size & (size - 1)) != 0 ||

is_power_of_2() ?

> + (size * sizeof(struct mask_cache_entry)) > PCPU_MIN_UNIT_SIZE)
> + return NULL;

> + new->cache_size = size;
> + if (new->cache_size > 0) {
> + cache = __alloc_percpu(sizeof(struct mask_cache_entry) *
> +new->cache_size,

array_size() ?

> +__alignof__(struct mask_cache_entry));
> +

No need for the new line here

> + if (!cache) {
> + kfree(new);
> + return NULL;
> + }
> + }

> + if (size == mc->cache_size || (size & (size - 1)) != 0)

why check "is power of 2" twice?

> @@ -454,7 +516,7 @@ 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);
>  
> - free_percpu(table->mask_cache);
> + call_rcu(>mask_cache->rcu, mask_cache_rcu_cb);
>   call_rcu(>mask_array->rcu, mask_array_rcu_cb);
>   table_instance_destroy(table, ti, ufid_ti, false);
>  }

This adds a new warning :(

net/openvswitch/flow_table.c:519:24: warning: incorrect type in argument 1 
(different address spaces)
net/openvswitch/flow_table.c:519:24:expected struct callback_head *head
net/openvswitch/flow_table.c:519:24:got struct callback_head [noderef] 
__rcu *
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

2020-07-23 Thread Eelco Chaudron 
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 
---
 include/uapi/linux/openvswitch.h |1 
 net/openvswitch/datapath.c   |   14 ++
 net/openvswitch/flow_table.c |   90 ++
 net/openvswitch/flow_table.h |   10 
 4 files changed, 104 insertions(+), 11 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(>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(>table, cache_size);
+   }
+
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..386b7e7a0454 100644
--- a/net/openvswitch/flow_table.c
+++ b/net/openvswitch/flow_table.c
@@ -38,8 +38,8 @@
 #define MASK_ARRAY_SIZE_MIN16
 #define REHASH_INTERVAL(10 * 60 * HZ)
 
+#define MC_DEFAULT_HASH_ENTRIES256
 #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,76 @@ 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);
+   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 ((size & (size - 1)) != 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(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;