Re: [ovs-dev] [PATCH v3 3/4] netdev-offload: Add xdp flow api provider
Bleep bloop. Greetings Toshiaki Makita, 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:
WARNING: Comment with 'xxx' marker
#252 FILE: lib/netdev-afxdp.c:329:
/* XXX: close output_map_fd somewhere? */
ERROR: Improper whitespace around control block
#734 FILE: lib/netdev-offload-xdp.c:258:
FLOWMAP_FOR_EACH_INDEX(idx, mf->map) {
ERROR: Improper whitespace around control block
#806 FILE: lib/netdev-offload-xdp.c:330:
NL_ATTR_FOR_EACH_UNSAFE(a, left, actions, actions_len) {
ERROR: Improper whitespace around control block
#963 FILE: lib/netdev-offload-xdp.c:487:
HMAP_FOR_EACH_WITH_HASH(netdev_info, port_node, port_hash,
ERROR: Improper whitespace around control block
#1018 FILE: lib/netdev-offload-xdp.c:542:
NL_ATTR_FOR_EACH_UNSAFE(a, left, actions, actions_len) {
WARNING: Comment with 'xxx' marker
#1038 FILE: lib/netdev-offload-xdp.c:562:
/* XXX: Some NICs cannot handle XDP_REDIRECT'ed packets even with
ERROR: Improper whitespace around control block
#1056 FILE: lib/netdev-offload-xdp.c:580:
HMAP_FOR_EACH_WITH_HASH(data, node, hash, &devmap_idx_table) {
ERROR: Improper whitespace around control block
#1255 FILE: lib/netdev-offload-xdp.c:779:
FLOWMAP_FOR_EACH_INDEX(fidx, minimatch.mask->masks.map) {
ERROR: Improper whitespace around control block
#1528 FILE: lib/netdev-offload-xdp.c:1052:
HMAP_FOR_EACH_WITH_HASH(data, ufid_node, hash, &ufid_to_xdp) {
WARNING: Line lacks whitespace around operator
#1604 FILE: lib/netdev-offload-xdp.c:1128:
if (entry->count == (uint16_t)-1) {
WARNING: Line is 87 characters long (recommended limit is 79)
#1735 FILE: lib/netdev-offload-xdp.c:1259:
VLOG_ERR("\"subtbl_masks\" map has different max_entries from
\"flow_table\"");
WARNING: Line is 83 characters long (recommended limit is 79)
#1782 FILE: lib/netdev-offload-xdp.c:1306:
VLOG_WARN("%s: Failed to get output_map fd on uninitializing xdp flow
api",
Lines checked: 1879, Warnings: 5, Errors: 7
Please check this out. If you feel there has been an error, please email
acon...@redhat.com
Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 3/4] netdev-offload: Add xdp flow api provider
On 2020/06/30 1:17, 0-day Robot wrote:
Bleep bloop. Greetings Toshiaki Makita, 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:
WARNING: Comment with 'xxx' marker
#252 FILE: lib/netdev-afxdp.c:329:
/* XXX: close output_map_fd somewhere? */
ERROR: Improper whitespace around control block
#734 FILE: lib/netdev-offload-xdp.c:258:
FLOWMAP_FOR_EACH_INDEX(idx, mf->map) {
Adding a whitespace like
FLOWMAP_FOR_EACH_INDEX (idx, mf->map) {
fixes the error, but as far as I can see, all existing usage of this macro
does not have this kind of whitespace.
Which is correct, need whitespace or not?
Toshiaki Makita
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 3/4] netdev-offload: Add xdp flow api provider
On 2020/06/30 0:30, Toshiaki Makita wrote:
...
int netdev_afxdp_init(void)
{
libbpf_set_print(libbpf_print);
-return 0;
+return netdev_register_flow_api_provider(&netdev_offload_xdp);
This causes duplicate flow api provider error because afxdp and afxdp-nonpmd
are using
the same init function, and afxdp-nonpmd netdev registration fails.
Probably afxdp-nonpmd does not need to call this init function.
I'll fix this in next version.
Toshiaki Makita
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 3/4] netdev-offload: Add xdp flow api provider
On Tue, Jun 30, 2020 at 12:11 AM Toshiaki Makita
wrote:
>
> On 2020/06/30 1:17, 0-day Robot wrote:
> > Bleep bloop. Greetings Toshiaki Makita, 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:
> > WARNING: Comment with 'xxx' marker
> > #252 FILE: lib/netdev-afxdp.c:329:
> > /* XXX: close output_map_fd somewhere? */
> >
> > ERROR: Improper whitespace around control block
> > #734 FILE: lib/netdev-offload-xdp.c:258:
> > FLOWMAP_FOR_EACH_INDEX(idx, mf->map) {
>
> Adding a whitespace like
>
> FLOWMAP_FOR_EACH_INDEX (idx, mf->map) {
>
> fixes the error, but as far as I can see, all existing usage of this macro
> does not have this kind of whitespace.
>
> Which is correct, need whitespace or not?
I think we need whitespace here.
Similar macros such as FLOWMAP_FOR_EACH_UNIT, FLOWMAP_FOR_EACH_MAP
also add whitespace
William
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 3/4] netdev-offload: Add xdp flow api provider
On Tue, Jul 7, 2020 at 2:07 AM Toshiaki Makita
wrote:
>
> On 2020/06/30 0:30, Toshiaki Makita wrote:
> ...
> > int netdev_afxdp_init(void)
> > {
> > libbpf_set_print(libbpf_print);
> > -return 0;
> > +return netdev_register_flow_api_provider(&netdev_offload_xdp);
>
> This causes duplicate flow api provider error because afxdp and afxdp-nonpmd
> are using
> the same init function, and afxdp-nonpmd netdev registration fails.
> Probably afxdp-nonpmd does not need to call this init function.
I think we just need to make sure it registers only once.
you can use
ovsthread_once_start(&once)
see example in netdev_dpdk_class_init(void)
Regards,
William
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 3/4] netdev-offload: Add xdp flow api provider
William Tu writes:
> On Tue, Jun 30, 2020 at 12:11 AM Toshiaki Makita
> wrote:
>>
>> On 2020/06/30 1:17, 0-day Robot wrote:
>> > Bleep bloop. Greetings Toshiaki Makita, 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:
>> > WARNING: Comment with 'xxx' marker
>> > #252 FILE: lib/netdev-afxdp.c:329:
>> > /* XXX: close output_map_fd somewhere? */
>> >
>> > ERROR: Improper whitespace around control block
>> > #734 FILE: lib/netdev-offload-xdp.c:258:
>> > FLOWMAP_FOR_EACH_INDEX(idx, mf->map) {
>>
>> Adding a whitespace like
>>
>> FLOWMAP_FOR_EACH_INDEX (idx, mf->map) {
>>
>> fixes the error, but as far as I can see, all existing usage of this macro
>> does not have this kind of whitespace.
>>
>> Which is correct, need whitespace or not?
Need whitespace. The usage of FLOWMAP_FOR_EACH throughout the codebase
predates automated checking. You'll note that when someone contributes
new versions (ex: tests/oss-fuzz/miniflow_target.c) they have the
whitespace.
Prior to a checkpatch utility, it was more difficult to catch instances
of style violations, and some snuck through. Even with checkpatch, some
make it through (though hopefully fewer).
> I think we need whitespace here.
Yes.
> Similar macros such as FLOWMAP_FOR_EACH_UNIT, FLOWMAP_FOR_EACH_MAP
> also add whitespace
> William
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 3/4] netdev-offload: Add xdp flow api provider
Thanks for the patch. My comments below:
On Mon, Jun 29, 2020 at 8:30 AM Toshiaki Makita
wrote:
>
> This provider offloads classifier to software XDP.
>
> It works only when a custom XDP object is loaded by afxdp netdev.
> The BPF program needs to implement classifier with array-of-maps for
> subtable hashmaps and arraymap for subtable masks. The flow api
> provider detects classifier support in the custom XDP program when
> loading it.
>
> The requirements for the BPF program is as below:
>
> - A struct named "meta_info" in ".ovs_meta" section
> inform vswitchd of supported keys, actions, and the max number of
> actions.
>
> - A map named "subtbl_masks"
> An array which implements a list. The entry contains struct
> xdp_subtables_mask provided by lib/netdev-offload-xdp.h followed by
> miniflow buf of any length for the mask.
>
> - A map named "subtbl_masks_hd"
> A one entry int array which contains the first entry index of the list
> implemented by subtbl_masks.
>
> - A map named "flow_table"
> An array-of-maps. Each entry points to subtable hash-map instantiated
> with the following "subtbl_template".
> The entry of each index of subtbl_masks has miniflow mask of subtable
> in the corresponding index of flow_table.
>
> - A map named "subtbl_template"
> A template for subtable, used for entries of "flow_table".
> The key must be miniflow buf (without leading map).
>
> - A map named "output_map"
> A devmap. Each entry represents odp port.
>
> - A map named "xsks_map"
> A xskmap. Used for upcall.
Maybe later on, we can add the above to Documentation/ and
explain the purpose of these maps there.
>
> For more details, refer to the reference BPF program in next commit.
>
> In the future it may be possible to offload classifier to SmartNIC
> through XDP, but currently it's not because map-in-map is not supported
> by XDP hw-offload.
I think it will be hard, given that only Netronome supports XDP offload today.
>
> Signed-off-by: Toshiaki Makita
> ---
> lib/automake.mk |6 +-
> lib/bpf-util.c| 38 +
> lib/bpf-util.h| 22 +
> lib/netdev-afxdp.c| 205 -
> lib/netdev-afxdp.h|3 +
> lib/netdev-linux-private.h|2 +
> lib/netdev-offload-provider.h |6 +
> lib/netdev-offload-xdp.c | 1315 +
> lib/netdev-offload-xdp.h | 49 ++
> lib/netdev-offload.c |6 +
> 10 files changed, 1650 insertions(+), 2 deletions(-)
> create mode 100644 lib/bpf-util.c
> create mode 100644 lib/bpf-util.h
> create mode 100644 lib/netdev-offload-xdp.c
> create mode 100644 lib/netdev-offload-xdp.h
>
> diff --git a/lib/automake.mk b/lib/automake.mk
> index 86940ccd2..1fa1371f3 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -421,10 +421,14 @@ endif
>
> if HAVE_AF_XDP
> lib_libopenvswitch_la_SOURCES += \
> + lib/bpf-util.c \
> + lib/bpf-util.h \
> lib/netdev-afxdp-pool.c \
> lib/netdev-afxdp-pool.h \
> lib/netdev-afxdp.c \
> - lib/netdev-afxdp.h
> + lib/netdev-afxdp.h \
> + lib/netdev-offload-xdp.c \
> + lib/netdev-offload-xdp.h
> endif
How about doing s.t like:
--enable-afxdp: the current one on master without the xdp offload program
--enable-afxdp-with-bpf: the afxdp one plus your xdp offload program
So that when users only --enable-afxdp, it doesn't need to compile in the
lib/netdev-offload-xdp.* and bpf/*
>
> if DPDK_NETDEV
> diff --git a/lib/bpf-util.c b/lib/bpf-util.c
> new file mode 100644
> index 0..324cfbe1d
> --- /dev/null
> +++ b/lib/bpf-util.c
> @@ -0,0 +1,38 @@
> +/*
> + * Copyright (c) 2020 NTT Corp.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include
> +
> +#include "bpf-util.h"
> +
> +#include
> +
> +#include "ovs-thread.h"
> +
> +DEFINE_STATIC_PER_THREAD_DATA(struct { char s[128]; },
> + libbpf_strerror_buffer,
> + { "" });
> +
> +const char *
> +ovs_libbpf_strerror(int err)
> +{
> +enum { BUFSIZE = sizeof libbpf_strerror_buffer_get()->s };
just curious:
what's the benefit of using enum {BUFSIZE ...} here?
> +char *buf = libbpf_strerror_buffer_get()->s;
> +
> +libbpf_strerror(err, buf, BUFSIZE);
> +
> +return buf;
> +}
> diff --git a/lib/bpf-util.h b/lib/bpf-util.h
> new file mode 100644
> index 0..6346935b3
> ---
Re: [ovs-dev] [PATCH v3 3/4] netdev-offload: Add xdp flow api provider
On 2020/07/22 0:38, Aaron Conole wrote:
William Tu writes:
On Tue, Jun 30, 2020 at 12:11 AM Toshiaki Makita
wrote:
On 2020/06/30 1:17, 0-day Robot wrote:
Bleep bloop. Greetings Toshiaki Makita, 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:
WARNING: Comment with 'xxx' marker
#252 FILE: lib/netdev-afxdp.c:329:
/* XXX: close output_map_fd somewhere? */
ERROR: Improper whitespace around control block
#734 FILE: lib/netdev-offload-xdp.c:258:
FLOWMAP_FOR_EACH_INDEX(idx, mf->map) {
Adding a whitespace like
FLOWMAP_FOR_EACH_INDEX (idx, mf->map) {
fixes the error, but as far as I can see, all existing usage of this macro
does not have this kind of whitespace.
Which is correct, need whitespace or not?
Need whitespace. The usage of FLOWMAP_FOR_EACH throughout the codebase
predates automated checking. You'll note that when someone contributes
new versions (ex: tests/oss-fuzz/miniflow_target.c) they have the
whitespace.
Prior to a checkpatch utility, it was more difficult to catch instances
of style violations, and some snuck through. Even with checkpatch, some
make it through (though hopefully fewer).
I think we need whitespace here.
Yes.
William, Aaron,
Thank you for the confirmation.
Will fix them in the next version.
Toshiaki Makita
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 3/4] netdev-offload: Add xdp flow api provider
On 2020/07/22 0:34, William Tu wrote:
On Tue, Jul 7, 2020 at 2:07 AM Toshiaki Makita
wrote:
On 2020/06/30 0:30, Toshiaki Makita wrote:
...
int netdev_afxdp_init(void)
{
libbpf_set_print(libbpf_print);
-return 0;
+return netdev_register_flow_api_provider(&netdev_offload_xdp);
This causes duplicate flow api provider error because afxdp and afxdp-nonpmd
are using
the same init function, and afxdp-nonpmd netdev registration fails.
Probably afxdp-nonpmd does not need to call this init function.
I think we just need to make sure it registers only once.
you can use
ovsthread_once_start(&once)
see example in netdev_dpdk_class_init(void)
OK, will try this.
Toshiaki Makita
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 3/4] netdev-offload: Add xdp flow api provider
On 2020/07/22 3:10, William Tu wrote:
Thanks for the patch. My comments below:
On Mon, Jun 29, 2020 at 8:30 AM Toshiaki Makita
wrote:
This provider offloads classifier to software XDP.
It works only when a custom XDP object is loaded by afxdp netdev.
The BPF program needs to implement classifier with array-of-maps for
subtable hashmaps and arraymap for subtable masks. The flow api
provider detects classifier support in the custom XDP program when
loading it.
The requirements for the BPF program is as below:
- A struct named "meta_info" in ".ovs_meta" section
inform vswitchd of supported keys, actions, and the max number of
actions.
- A map named "subtbl_masks"
An array which implements a list. The entry contains struct
xdp_subtables_mask provided by lib/netdev-offload-xdp.h followed by
miniflow buf of any length for the mask.
- A map named "subtbl_masks_hd"
A one entry int array which contains the first entry index of the list
implemented by subtbl_masks.
- A map named "flow_table"
An array-of-maps. Each entry points to subtable hash-map instantiated
with the following "subtbl_template".
The entry of each index of subtbl_masks has miniflow mask of subtable
in the corresponding index of flow_table.
- A map named "subtbl_template"
A template for subtable, used for entries of "flow_table".
The key must be miniflow buf (without leading map).
- A map named "output_map"
A devmap. Each entry represents odp port.
- A map named "xsks_map"
A xskmap. Used for upcall.
Maybe later on, we can add the above to Documentation/ and
explain the purpose of these maps there.
Sure.
For more details, refer to the reference BPF program in next commit.
In the future it may be possible to offload classifier to SmartNIC
through XDP, but currently it's not because map-in-map is not supported
by XDP hw-offload.
I think it will be hard, given that only Netronome supports XDP offload today.
I'll try it if I can get a netronome NIC.
Signed-off-by: Toshiaki Makita
---
lib/automake.mk |6 +-
lib/bpf-util.c| 38 +
lib/bpf-util.h| 22 +
lib/netdev-afxdp.c| 205 -
lib/netdev-afxdp.h|3 +
lib/netdev-linux-private.h|2 +
lib/netdev-offload-provider.h |6 +
lib/netdev-offload-xdp.c | 1315 +
lib/netdev-offload-xdp.h | 49 ++
lib/netdev-offload.c |6 +
10 files changed, 1650 insertions(+), 2 deletions(-)
create mode 100644 lib/bpf-util.c
create mode 100644 lib/bpf-util.h
create mode 100644 lib/netdev-offload-xdp.c
create mode 100644 lib/netdev-offload-xdp.h
diff --git a/lib/automake.mk b/lib/automake.mk
index 86940ccd2..1fa1371f3 100644
--- a/lib/automake.mk
+++ b/lib/automake.mk
@@ -421,10 +421,14 @@ endif
if HAVE_AF_XDP
lib_libopenvswitch_la_SOURCES += \
+ lib/bpf-util.c \
+ lib/bpf-util.h \
lib/netdev-afxdp-pool.c \
lib/netdev-afxdp-pool.h \
lib/netdev-afxdp.c \
- lib/netdev-afxdp.h
+ lib/netdev-afxdp.h \
+ lib/netdev-offload-xdp.c \
+ lib/netdev-offload-xdp.h
endif
How about doing s.t like:
--enable-afxdp: the current one on master without the xdp offload program
--enable-afxdp-with-bpf: the afxdp one plus your xdp offload program
So that when users only --enable-afxdp, it doesn't need to compile in the
lib/netdev-offload-xdp.* and bpf/*
Let me clarify it.
Currently,
--enable-afxdp: build lib/netdev-afxdp* and lib/netdev-offload-xdp*
--enable-bpf: build bpf/*
Do you sugguest this?
--enable-afxdp: build lib/netdev-afxdp*
--enable-afxdp-with-bpf: build lib/netdev-afxdp*, lib/netdev-offload-xdp*, and
bpf/*
if DPDK_NETDEV
diff --git a/lib/bpf-util.c b/lib/bpf-util.c
new file mode 100644
index 0..324cfbe1d
--- /dev/null
+++ b/lib/bpf-util.c
@@ -0,0 +1,38 @@
+/*
+ * Copyright (c) 2020 NTT Corp.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include
+
+#include "bpf-util.h"
+
+#include
+
+#include "ovs-thread.h"
+
+DEFINE_STATIC_PER_THREAD_DATA(struct { char s[128]; },
+ libbpf_strerror_buffer,
+ { "" });
+
+const char *
+ovs_libbpf_strerror(int err)
+{
+enum { BUFSIZE = sizeof libbpf_strerror_buffer_get()->s };
just curious:
what's the benefit of using enum {BUFSIZE ...} here?
Just imitated ovs_strerror().
Did not consider
Re: [ovs-dev] [PATCH v3 3/4] netdev-offload: Add xdp flow api provider
William,
On 2020/07/22 17:46, Toshiaki Makita wrote:
On 2020/07/22 3:10, William Tu wrote:
Thanks for the patch. My comments below:
On Mon, Jun 29, 2020 at 8:30 AM Toshiaki Makita
wrote:
...
diff --git a/lib/automake.mk b/lib/automake.mk
index 86940ccd2..1fa1371f3 100644
--- a/lib/automake.mk
+++ b/lib/automake.mk
@@ -421,10 +421,14 @@ endif
if HAVE_AF_XDP
lib_libopenvswitch_la_SOURCES += \
+ lib/bpf-util.c \
+ lib/bpf-util.h \
lib/netdev-afxdp-pool.c \
lib/netdev-afxdp-pool.h \
lib/netdev-afxdp.c \
- lib/netdev-afxdp.h
+ lib/netdev-afxdp.h \
+ lib/netdev-offload-xdp.c \
+ lib/netdev-offload-xdp.h
endif
How about doing s.t like:
--enable-afxdp: the current one on master without the xdp offload program
--enable-afxdp-with-bpf: the afxdp one plus your xdp offload program
So that when users only --enable-afxdp, it doesn't need to compile in the
lib/netdev-offload-xdp.* and bpf/*
Let me clarify it.
Currently,
--enable-afxdp: build lib/netdev-afxdp* and lib/netdev-offload-xdp*
--enable-bpf: build bpf/*
Do you sugguest this?
--enable-afxdp: build lib/netdev-afxdp*
--enable-afxdp-with-bpf: build lib/netdev-afxdp*, lib/netdev-offload-xdp*, and
bpf/*
Maybe we should not introduce this kind of overlapping?
(enable-afxdp-with-bpf should not include enable-afxdp)
How about this?
--enable-afxdp: build lib/netdev-afxdp*
--enable-xdp-offload: build lib/netdev-offload-xdp*
--enable-bpf: build bpf/*
I thought the following is also possible:
--enable-afxdp: build lib/netdev-afxdp*
--enable-afxdp=offload: build lib/netdev-afxdp* and lib/netdev-offload-xdp*
--enable-bpf: build bpf/*
But theoretically xdp offload can be enabled without afxdp in the future,
so I guess a different build switch, enable-xdp-offload, is better.
+static int
+xdp_preload(struct netdev *netdev, struct bpf_object *obj)
...
+ if (ovsthread_once_start(&output_map_once)) {
+ /* XXX: close output_map_fd somewhere? */
maybe at uninit_flow_api, we have to check whether there is
still a netdev using linux_xdp offload. And if not, close this map?
Then we need to cancel the ovsthread_once at that time?
I could not find such an api.
I feel like we should close the map when shutting down the ovs-vswitchd, but not so
important
as the process exits anyway.
I'll just drop this comment. This should not be necessary.
+static int
+netdev_xdp_flow_put(struct netdev *netdev, struct match *match_,
...
+ memcpy(pentry, entry, netdev_info->subtable_mask_size);
+ pidx = idx;
+ idx = entry->next;
question about this for loop:
why do we need to maintain ->next for the struct
xdp_subtable_mask_header *entry?
I thought we have a fixed-sized array of subtable.
And all we need to do is go over all the valid index of the array,
until finding a match.
I think you are right.
This code is ported from xdp_flow, where order is necessary.
I guess ovs-vswitchd does not care the order of each masks.
Will double check it.
After some more consideration, I'm thinking using ->next to maintain a list is
better.
As you say, indeed we can maintain an array to hold masks list.
But in that case we need to have a feature to invalidate an entry if we don't
replace the entire array on deletion of an entry.
Also to avoid too sparse array, we probably should implement deletion by swapping
the entry to be deleted and the last entry. Then, we need to invalidate an entry
temporarily, since we don't want to access the entry while it is being updated.
Think about something like the following entry to do invalidation:
struct table_entry {
struct entry_value value;
uint8_t valid;
}
We should update ->valid field and ->value field separately in order not to let CPU
reorder memory read (bpf does not provide memory barriers).
in vswitchd:
entry->valid = false;
bpf_map_update_elem(map, idx, entry);
entry->value.x = x;
entry->value.y = y;
...
bpf_map_update_elem(map, idx, entry);
entry->valid = true;
bpf_map_update_elem(map, idx, entry);
looks redundant?
Thanks,
Toshiaki Makita
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 3/4] netdev-offload: Add xdp flow api provider
On Sun, Jul 26, 2020 at 9:55 AM Toshiaki Makita
wrote:
>
snip
> >> How about doing s.t like:
> >> --enable-afxdp: the current one on master without the xdp offload program
> >> --enable-afxdp-with-bpf: the afxdp one plus your xdp offload program
> >>
> >> So that when users only --enable-afxdp, it doesn't need to compile in the
> >> lib/netdev-offload-xdp.* and bpf/*
> >
> > Let me clarify it.
> >
> > Currently,
> >
> > --enable-afxdp: build lib/netdev-afxdp* and lib/netdev-offload-xdp*
> > --enable-bpf: build bpf/*
> >
> > Do you sugguest this?
> >
> > --enable-afxdp: build lib/netdev-afxdp*
> > --enable-afxdp-with-bpf: build lib/netdev-afxdp*, lib/netdev-offload-xdp*,
> > and bpf/*
>
> Maybe we should not introduce this kind of overlapping?
> (enable-afxdp-with-bpf should not include enable-afxdp)
>
> How about this?
>
> --enable-afxdp: build lib/netdev-afxdp*
> --enable-xdp-offload: build lib/netdev-offload-xdp*
> --enable-bpf: build bpf/*
>
> I thought the following is also possible:
>
> --enable-afxdp: build lib/netdev-afxdp*
> --enable-afxdp=offload: build lib/netdev-afxdp* and lib/netdev-offload-xdp*
> --enable-bpf: build bpf/*
But isn't --enable-afxdp-offload has dependency on --enable-bpf?
Otherwise, if only doing "--enable-afxdp-offload", there is no BPF program
compiled and nothing is loaded.
>
> But theoretically xdp offload can be enabled without afxdp in the future,
> so I guess a different build switch, enable-xdp-offload, is better.
>
> >>> +static int
> >>> +xdp_preload(struct netdev *netdev, struct bpf_object *obj)
> ...
> >>> +if (ovsthread_once_start(&output_map_once)) {
> >>> +/* XXX: close output_map_fd somewhere? */
> >> maybe at uninit_flow_api, we have to check whether there is
> >> still a netdev using linux_xdp offload. And if not, close this map?
> >
> > Then we need to cancel the ovsthread_once at that time?
> > I could not find such an api.
> >
> > I feel like we should close the map when shutting down the ovs-vswitchd,
> > but not so
> > important
> > as the process exits anyway.
>
> I'll just drop this comment. This should not be necessary.
>
> >>> +static int
> >>> +netdev_xdp_flow_put(struct netdev *netdev, struct match *match_,
> ...
> >>> +memcpy(pentry, entry, netdev_info->subtable_mask_size);
> >>> +pidx = idx;
> >>> +idx = entry->next;
> >> question about this for loop:
> >> why do we need to maintain ->next for the struct
> >> xdp_subtable_mask_header *entry?
> >> I thought we have a fixed-sized array of subtable.
> >> And all we need to do is go over all the valid index of the array,
> >> until finding a match.
> >
> > I think you are right.
> > This code is ported from xdp_flow, where order is necessary.
> > I guess ovs-vswitchd does not care the order of each masks.
> > Will double check it.
>
> After some more consideration, I'm thinking using ->next to maintain a list
> is better.
>
> As you say, indeed we can maintain an array to hold masks list.
> But in that case we need to have a feature to invalidate an entry if we don't
> replace the entire array on deletion of an entry.
> Also to avoid too sparse array, we probably should implement deletion by
> swapping
> the entry to be deleted and the last entry. Then, we need to invalidate an
> entry
> temporarily, since we don't want to access the entry while it is being
> updated.
>
> Think about something like the following entry to do invalidation:
>
> struct table_entry {
> struct entry_value value;
> uint8_t valid;
> }
>
> We should update ->valid field and ->value field separately in order not to
> let CPU
> reorder memory read (bpf does not provide memory barriers).
yes, that's a big limitation.
>
> in vswitchd:
>
> entry->valid = false;
> bpf_map_update_elem(map, idx, entry);
> entry->value.x = x;
> entry->value.y = y;
> ...
> bpf_map_update_elem(map, idx, entry);
> entry->valid = true;
> bpf_map_update_elem(map, idx, entry);
I see, thanks for your explanation.
William
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 3/4] netdev-offload: Add xdp flow api provider
On 2020/07/28 3:14, William Tu wrote: On Sun, Jul 26, 2020 at 9:55 AM Toshiaki Makita wrote: snip How about doing s.t like: --enable-afxdp: the current one on master without the xdp offload program --enable-afxdp-with-bpf: the afxdp one plus your xdp offload program So that when users only --enable-afxdp, it doesn't need to compile in the lib/netdev-offload-xdp.* and bpf/* Let me clarify it. Currently, --enable-afxdp: build lib/netdev-afxdp* and lib/netdev-offload-xdp* --enable-bpf: build bpf/* Do you sugguest this? --enable-afxdp: build lib/netdev-afxdp* --enable-afxdp-with-bpf: build lib/netdev-afxdp*, lib/netdev-offload-xdp*, and bpf/* Maybe we should not introduce this kind of overlapping? (enable-afxdp-with-bpf should not include enable-afxdp) How about this? --enable-afxdp: build lib/netdev-afxdp* --enable-xdp-offload: build lib/netdev-offload-xdp* --enable-bpf: build bpf/* I thought the following is also possible: --enable-afxdp: build lib/netdev-afxdp* --enable-afxdp=offload: build lib/netdev-afxdp* and lib/netdev-offload-xdp* --enable-bpf: build bpf/* But isn't --enable-afxdp-offload has dependency on --enable-bpf? Strictly speaking, no. --enable-bpf just builds a reference program. Users can use it, but alternatively they can use their own programs built in their own build system as well. However, since that's probably rare, if you like I'll merge --enable-bpf into --enable-xdp-offload. If necessary, we can add --enable-bpf=no option later on. Toshiaki Makita ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
