| Message ID | 1553700739-11387-3-git-send-email-gvrose8192@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Linux datapath upstream fixes | expand |
Bleep bloop. Greetings Gregory Rose, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: ERROR: Author Kent Overstreet <kent.overstreet@gmail.com> needs to sign off. WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Greg Rose <gvrose8192@gmail.com> Lines checked: 208, Warnings: 1, Errors: 1 Please check this out. If you feel there has been an error, please email aconole@bytheb.org Thanks, 0-day Robot
As commented, previously flex_array was used because "we can have large
hash-table without large contiguous kernel memory".
Actually how large can the flow hash table can be? Will this backport
affect currently supported hash table size?
Thanks,
Yifeng
On Wed, Mar 27, 2019 at 9:06 AM 0-day Robot <robot@bytheb.org> wrote:
>
> Bleep bloop. Greetings Gregory Rose, I am a robot and I have tried out your patch.
> Thanks for your contribution.
>
> I encountered some error that I wasn't expecting. See the details below.
>
>
> checkpatch:
> ERROR: Author Kent Overstreet <kent.overstreet@gmail.com> needs to sign off.
> WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Greg Rose <gvrose8192@gmail.com>
> Lines checked: 208, Warnings: 1, Errors: 1
>
>
> Please check this out. If you feel there has been an error, please email aconole@bytheb.org
>
> Thanks,
> 0-day Robot
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 4/1/2019 10:21 AM, Yifeng Sun wrote: > As commented, previously flex_array was used because "we can have large > hash-table without large contiguous kernel memory". > > Actually how large can the flow hash table can be? Will this backport > affect currently supported hash table size? Hi Yifeng, sorry for the late reply... The introduction of kvmalloc in the Linux kernel with commit a7c3e901a4 "mm: introduce kv[mz]alloc helpers" helps explain. kvmalloc_array() will allocate large, contiguous memory areas. As noted in this patch by Kent Overstreet we should probably use rhashtable instead but that would take additional work and is for future development. Thanks, - Greg > > Thanks, > Yifeng > > On Wed, Mar 27, 2019 at 9:06 AM 0-day Robot <robot@bytheb.org> wrote: >> Bleep bloop. Greetings Gregory Rose, I am a robot and I have tried out your patch. >> Thanks for your contribution. >> >> I encountered some error that I wasn't expecting. See the details below. >> >> >> checkpatch: >> ERROR: Author Kent Overstreet <kent.overstreet@gmail.com> needs to sign off. >> WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Greg Rose <gvrose8192@gmail.com> >> Lines checked: 208, Warnings: 1, Errors: 1 >> >> >> Please check this out. If you feel there has been an error, please email aconole@bytheb.org >> >> Thanks, >> 0-day Robot >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Got it, thanks! Reviewed-by: Yifeng Sun <pkusunyifeng@gmail.com> On Wed, Apr 10, 2019 at 10:43 AM Gregory Rose <gvrose8192@gmail.com> wrote: > > > > On 4/1/2019 10:21 AM, Yifeng Sun wrote: > > As commented, previously flex_array was used because "we can have large > > hash-table without large contiguous kernel memory". > > > > Actually how large can the flow hash table can be? Will this backport > > affect currently supported hash table size? > Hi Yifeng, > > sorry for the late reply... > > The introduction of kvmalloc in the Linux kernel with commit a7c3e901a4 > "mm: introduce kv[mz]alloc helpers" > helps explain. kvmalloc_array() will allocate large, contiguous memory > areas. > > As noted in this patch by Kent Overstreet we should probably use > rhashtable instead but that would take > additional work and is for future development. > > Thanks, > > - Greg > > > > > Thanks, > > Yifeng > > > > On Wed, Mar 27, 2019 at 9:06 AM 0-day Robot <robot@bytheb.org> wrote: > >> Bleep bloop. Greetings Gregory Rose, I am a robot and I have tried out your patch. > >> Thanks for your contribution. > >> > >> I encountered some error that I wasn't expecting. See the details below. > >> > >> > >> checkpatch: > >> ERROR: Author Kent Overstreet <kent.overstreet@gmail.com> needs to sign off. > >> WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Greg Rose <gvrose8192@gmail.com> > >> Lines checked: 208, Warnings: 1, Errors: 1 > >> > >> > >> Please check this out. If you feel there has been an error, please email aconole@bytheb.org > >> > >> Thanks, > >> 0-day Robot > >> _______________________________________________ > >> dev mailing list > >> dev@openvswitch.org > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/datapath/flow.h b/datapath/flow.h index c63e586..87e212f 100644 --- a/datapath/flow.h +++ b/datapath/flow.h @@ -30,7 +30,6 @@ #include <linux/in6.h> #include <linux/jiffies.h> #include <linux/time.h> -#include <linux/flex_array.h> #include <linux/cpumask.h> #include <net/inet_ecn.h> #include <net/ip_tunnels.h> diff --git a/datapath/flow_netlink.h b/datapath/flow_netlink.h index 97153f3..e10df2b 100644 --- a/datapath/flow_netlink.h +++ b/datapath/flow_netlink.h @@ -30,7 +30,6 @@ #include <linux/in6.h> #include <linux/jiffies.h> #include <linux/time.h> -#include <linux/flex_array.h> #include <net/inet_ecn.h> #include <net/ip_tunnels.h> diff --git a/datapath/flow_table.c b/datapath/flow_table.c index 47057a1..83ea2be 100644 --- a/datapath/flow_table.c +++ b/datapath/flow_table.c @@ -117,29 +117,6 @@ int ovs_flow_tbl_count(const struct flow_table *table) return table->count; } -static struct flex_array *alloc_buckets(unsigned int n_buckets) -{ - struct flex_array *buckets; - int i, err; - - buckets = flex_array_alloc(sizeof(struct hlist_head), - n_buckets, GFP_KERNEL); - if (!buckets) - return NULL; - - err = flex_array_prealloc(buckets, 0, n_buckets, GFP_KERNEL); - if (err) { - flex_array_free(buckets); - return NULL; - } - - for (i = 0; i < n_buckets; i++) - INIT_HLIST_HEAD((struct hlist_head *) - flex_array_get(buckets, i)); - - return buckets; -} - static void flow_free(struct sw_flow *flow) { int cpu; @@ -174,31 +151,30 @@ void ovs_flow_free(struct sw_flow *flow, bool deferred) flow_free(flow); } -static void free_buckets(struct flex_array *buckets) -{ - flex_array_free(buckets); -} - - static void __table_instance_destroy(struct table_instance *ti) { - free_buckets(ti->buckets); + kvfree(ti->buckets); kfree(ti); } static struct table_instance *table_instance_alloc(int new_size) { struct table_instance *ti = kmalloc(sizeof(*ti), GFP_KERNEL); + int i; if (!ti) return NULL; - ti->buckets = alloc_buckets(new_size); - + ti->buckets = kvmalloc_array(new_size, sizeof(struct hlist_head), + GFP_KERNEL); if (!ti->buckets) { kfree(ti); return NULL; } + + for (i = 0; i < new_size; i++) + INIT_HLIST_HEAD(&ti->buckets[i]); + ti->n_buckets = new_size; ti->node_ver = 0; ti->keep_flows = false; @@ -319,7 +295,7 @@ static void table_instance_destroy(struct table_instance *ti, for (i = 0; i < ti->n_buckets; i++) { struct sw_flow *flow; - struct hlist_head *head = flex_array_get(ti->buckets, i); + struct hlist_head *head = &ti->buckets[i]; struct hlist_node *n; int ver = ti->node_ver; int ufid_ver = ufid_ti->node_ver; @@ -366,7 +342,7 @@ struct sw_flow *ovs_flow_tbl_dump_next(struct table_instance *ti, ver = ti->node_ver; while (*bucket < ti->n_buckets) { i = 0; - head = flex_array_get(ti->buckets, *bucket); + head = &ti->buckets[*bucket]; hlist_for_each_entry_rcu(flow, head, flow_table.node[ver]) { if (i < *last) { i++; @@ -385,8 +361,7 @@ struct sw_flow *ovs_flow_tbl_dump_next(struct table_instance *ti, static struct hlist_head *find_bucket(struct table_instance *ti, u32 hash) { hash = jhash_1word(hash, ti->hash_seed); - return flex_array_get(ti->buckets, - (hash & (ti->n_buckets - 1))); + return &ti->buckets[hash & (ti->n_buckets - 1)]; } static void table_instance_insert(struct table_instance *ti, @@ -419,9 +394,7 @@ static void flow_table_copy_flows(struct table_instance *old, /* Insert in new table. */ for (i = 0; i < old->n_buckets; i++) { struct sw_flow *flow; - struct hlist_head *head; - - head = flex_array_get(old->buckets, i); + struct hlist_head *head = &old->buckets[i]; if (ufid) hlist_for_each_entry(flow, head, diff --git a/datapath/flow_table.h b/datapath/flow_table.h index 8fa99d8..1a76886 100644 --- a/datapath/flow_table.h +++ b/datapath/flow_table.h @@ -29,7 +29,6 @@ #include <linux/in6.h> #include <linux/jiffies.h> #include <linux/time.h> -#include <linux/flex_array.h> #include <net/inet_ecn.h> #include <net/ip_tunnels.h> @@ -48,7 +47,7 @@ struct mask_array { }; struct table_instance { - struct flex_array *buckets; + struct hlist_head *buckets; unsigned int n_buckets; struct rcu_head rcu; int node_ver;