| Message ID | 20200225014632.18722-1-Yanqin.Wei@arm.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v1] netdev: fix partial offloading test cases failure | expand |
On 2/25/20 2:46 AM, Yanqin Wei wrote: > Some partial offloading test cases are failing inconsistently. The root > cause is that dummy netdev is assigned with incorrect offloading flow API. > dpif-netdev - partial hw offload - dummy > dpif-netdev - partial hw offload - dummy-pmd > dpif-netdev - partial hw offload with packet modifications - dummy > dpif-netdev - partial hw offload with packet modifications - dummy-pmd > > This patch fixes this issue by adding a specified flow api type in netdev. > Dummy netdev class can specify flow type in construct function. All of the > above cases can pass consistently. Could you, please, clarify which offload provider is assigned to dummy ports in your case and why this happens? In general, we need to fix offload providers to only accept ports that are usable for them instead of hardcoding the type. Best regards, Ilya Maximets.
Hi Ilya, > -----Original Message----- > From: Ilya Maximets <i.maximets@ovn.org> > Sent: Tuesday, February 25, 2020 5:25 PM > To: Yanqin Wei <Yanqin.Wei@arm.com>; dev@openvswitch.org > Cc: nd <nd@arm.com>; Lijian Zhang <Lijian.Zhang@arm.com>; Gavin Hu > <Gavin.Hu@arm.com>; i.maximets@ovn.org > Subject: Re: [ovs-dev] [PATCH v1] netdev: fix partial offloading test cases > failure > > On 2/25/20 2:46 AM, Yanqin Wei wrote: > > Some partial offloading test cases are failing inconsistently. The > > root cause is that dummy netdev is assigned with incorrect offloading flow > API. > > dpif-netdev - partial hw offload - dummy dpif-netdev - partial hw > > offload - dummy-pmd dpif-netdev - partial hw offload with packet > > modifications - dummy dpif-netdev - partial hw offload with packet > > modifications - dummy-pmd > > > > This patch fixes this issue by adding a specified flow api type in netdev. > > Dummy netdev class can specify flow type in construct function. All of > > the above cases can pass consistently. > > Could you, please, clarify which offload provider is assigned to dummy ports in > your case and why this happens? > > In general, we need to fix offload providers to only accept ports that are usable > for them instead of hardcoding the type. > [Yanqin] Sometimes " linux_tc" is assigned to dummy netdev by mistake. Currently, dpif traverses all offloading provider and select the first provider with successful initialization. It makes the result uncertain and random. I think the problem is who should take responsible for offloading providers assignment. Both dpif and netdev class impact the provider selection Firstly, netdev may specify offloading provider if the option is unique. Secondly, dpif should determine if netdev has more than one option (instead of traverse all providers). This patch implement the first one. The second one could be implemented in netdev_init_flow_api by means of dpif class and netdev class. What do you think of this? Best Regards, Wei Yanqin > Best regards, Ilya Maximets.
On 2/25/20 10:59 AM, Yanqin Wei wrote: > Hi Ilya, > > >> -----Original Message----- >> From: Ilya Maximets <i.maximets@ovn.org> >> Sent: Tuesday, February 25, 2020 5:25 PM >> To: Yanqin Wei <Yanqin.Wei@arm.com>; dev@openvswitch.org >> Cc: nd <nd@arm.com>; Lijian Zhang <Lijian.Zhang@arm.com>; Gavin Hu >> <Gavin.Hu@arm.com>; i.maximets@ovn.org >> Subject: Re: [ovs-dev] [PATCH v1] netdev: fix partial offloading test cases >> failure >> >> On 2/25/20 2:46 AM, Yanqin Wei wrote: >>> Some partial offloading test cases are failing inconsistently. The >>> root cause is that dummy netdev is assigned with incorrect offloading flow >> API. >>> dpif-netdev - partial hw offload - dummy dpif-netdev - partial hw >>> offload - dummy-pmd dpif-netdev - partial hw offload with packet >>> modifications - dummy dpif-netdev - partial hw offload with packet >>> modifications - dummy-pmd >>> >>> This patch fixes this issue by adding a specified flow api type in netdev. >>> Dummy netdev class can specify flow type in construct function. All of >>> the above cases can pass consistently. >> >> Could you, please, clarify which offload provider is assigned to dummy ports in >> your case and why this happens? >> >> In general, we need to fix offload providers to only accept ports that are usable >> for them instead of hardcoding the type. >> > [Yanqin] Sometimes " linux_tc" is assigned to dummy netdev by mistake. Currently, dpif traverses all offloading provider and select the first provider with successful initialization. It makes the result uncertain and random. > I think the problem is who should take responsible for offloading providers assignment. Both dpif and netdev class impact the provider selection > Firstly, netdev may specify offloading provider if the option is unique. Secondly, dpif should determine if netdev has more than one option (instead of traverse all providers). > This patch implement the first one. The second one could be implemented in netdev_init_flow_api by means of dpif class and netdev class. > What do you think of this? I think that current implementation of dynamic flow API assignment is somewhat OK and there is no significant issues that we should address. For this particular issue, I think we just need to be a little bit more careful with tests. I believe that removing of 'options:ifindex=1' from the test along with applying of the following patch: https://patchwork.ozlabs.org/patch/1226013/ should fix the occasional assignment of tinux_tc flow API for dummy port. For now, as a workaround, to fix this particular case we could change 'options:ifindex=1' to some fairly big value instead of using '1', which is likely used by some of the existing system interfaces. Best regards, Ilya Maximets.
> -----Original Message----- > From: Ilya Maximets <i.maximets@ovn.org> > Sent: Tuesday, February 25, 2020 6:31 PM > To: Yanqin Wei <Yanqin.Wei@arm.com>; Ilya Maximets > <i.maximets@ovn.org>; dev@openvswitch.org > Cc: nd <nd@arm.com>; Lijian Zhang <Lijian.Zhang@arm.com>; Gavin Hu > <Gavin.Hu@arm.com> > Subject: Re: [ovs-dev] [PATCH v1] netdev: fix partial offloading test cases > failure > > On 2/25/20 10:59 AM, Yanqin Wei wrote: > > Hi Ilya, > > > > > >> -----Original Message----- > >> From: Ilya Maximets <i.maximets@ovn.org> > >> Sent: Tuesday, February 25, 2020 5:25 PM > >> To: Yanqin Wei <Yanqin.Wei@arm.com>; dev@openvswitch.org > >> Cc: nd <nd@arm.com>; Lijian Zhang <Lijian.Zhang@arm.com>; Gavin Hu > >> <Gavin.Hu@arm.com>; i.maximets@ovn.org > >> Subject: Re: [ovs-dev] [PATCH v1] netdev: fix partial offloading test > >> cases failure > >> > >> On 2/25/20 2:46 AM, Yanqin Wei wrote: > >>> Some partial offloading test cases are failing inconsistently. The > >>> root cause is that dummy netdev is assigned with incorrect > >>> offloading flow > >> API. > >>> dpif-netdev - partial hw offload - dummy dpif-netdev - partial hw > >>> offload - dummy-pmd dpif-netdev - partial hw offload with packet > >>> modifications - dummy dpif-netdev - partial hw offload with packet > >>> modifications - dummy-pmd > >>> > >>> This patch fixes this issue by adding a specified flow api type in netdev. > >>> Dummy netdev class can specify flow type in construct function. All > >>> of the above cases can pass consistently. > >> > >> Could you, please, clarify which offload provider is assigned to > >> dummy ports in your case and why this happens? > >> > >> In general, we need to fix offload providers to only accept ports > >> that are usable for them instead of hardcoding the type. > >> > > [Yanqin] Sometimes " linux_tc" is assigned to dummy netdev by mistake. > Currently, dpif traverses all offloading provider and select the first provider > with successful initialization. It makes the result uncertain and random. > > I think the problem is who should take responsible for offloading > > providers assignment. Both dpif and netdev class impact the provider > selection Firstly, netdev may specify offloading provider if the option is unique. > Secondly, dpif should determine if netdev has more than one option (instead > of traverse all providers). > > This patch implement the first one. The second one could be implemented in > netdev_init_flow_api by means of dpif class and netdev class. > > What do you think of this? > > I think that current implementation of dynamic flow API assignment is > somewhat OK and there is no significant issues that we should address. > > For this particular issue, I think we just need to be a little bit more careful with > tests. I believe that removing of 'options:ifindex=1' from the test along with > applying of the following patch: > https://patchwork.ozlabs.org/patch/1226013/ > should fix the occasional assignment of tinux_tc flow API for dummy port. > > For now, as a workaround, to fix this particular case we could change > 'options:ifindex=1' to some fairly big value instead of using '1', which is likely > used by some of the existing system interfaces. > [Yanqin] Using ifindex to determine offloading provider is a little tricky. If more netdev need support offloading (which have valid ifindex but not support tc offloading), the logic needs to change. But as you said, there is no significant issue so far. We could use the workaround you suggest and refine the provider assignment when required. I will update v2 based on this workaround. > Best regards, Ilya Maximets.
diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c index 71df29184..252b9d802 100644 --- a/lib/netdev-dummy.c +++ b/lib/netdev-dummy.c @@ -50,6 +50,8 @@ VLOG_DEFINE_THIS_MODULE(netdev_dummy); #define C_STATS_SIZE 2 +#define DUMMY_OFFLOAD_TYPE "dummy" + struct reconnect; struct dummy_packet_stream { @@ -709,6 +711,7 @@ netdev_dummy_construct(struct netdev *netdev_) dummy_packet_conn_init(&netdev->conn); + netdev_->flow_api_type = DUMMY_OFFLOAD_TYPE; ovs_list_init(&netdev->rxes); hmap_init(&netdev->offloaded_flows); ovs_mutex_unlock(&netdev->mutex); @@ -1588,7 +1591,7 @@ netdev_dummy_offloads_init_flow_api(struct netdev *netdev) } static const struct netdev_flow_api netdev_offload_dummy = { - .type = "dummy", + .type = DUMMY_OFFLOAD_TYPE, .flow_put = netdev_dummy_flow_put, .flow_del = netdev_dummy_flow_del, .init_flow_api = netdev_dummy_offloads_init_flow_api, diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c index 32eab5910..c4bc71618 100644 --- a/lib/netdev-offload.c +++ b/lib/netdev-offload.c @@ -173,6 +173,17 @@ netdev_assign_flow_api(struct netdev *netdev) { struct netdev_registered_flow_api *rfa; + if (netdev->flow_api_type && + (rfa = netdev_lookup_flow_api(netdev->flow_api_type))) { + if (!rfa->flow_api->init_flow_api(netdev)) { + ovs_refcount_ref(&rfa->refcnt); + ovsrcu_set(&netdev->flow_api, rfa->flow_api); + VLOG_INFO("%s: Sepecified flow API '%s'.", + netdev_get_name(netdev), rfa->flow_api->type); + return 0; + } + } + CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) { if (!rfa->flow_api->init_flow_api(netdev)) { ovs_refcount_ref(&rfa->refcnt); diff --git a/lib/netdev-provider.h b/lib/netdev-provider.h index 22f4cde33..5942166a8 100644 --- a/lib/netdev-provider.h +++ b/lib/netdev-provider.h @@ -93,6 +93,7 @@ struct netdev { struct ovs_list saved_flags_list; /* Contains "struct netdev_saved_flags". */ /* Functions to control flow offloading. */ + char *flow_api_type; OVSRCU_TYPE(const struct netdev_flow_api *) flow_api; struct netdev_hw_info hw_info; /* offload-capable netdev info */ }; diff --git a/lib/netdev.c b/lib/netdev.c index f95b19af4..5c799f854 100644 --- a/lib/netdev.c +++ b/lib/netdev.c @@ -423,6 +423,7 @@ netdev_open(const char *name, const char *type, struct netdev **netdevp) netdev->reconfigure_seq = seq_create(); netdev->last_reconfigure_seq = seq_read(netdev->reconfigure_seq); + netdev->flow_api_type = NULL; ovsrcu_set(&netdev->flow_api, NULL); netdev->hw_info.oor = false; netdev->node = shash_add(&netdev_shash, name, netdev);