| Message ID | 1569842139-21593-1-git-send-email-wenxu@ucloud.cn |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [ovs-dev] upcall: Configure datapath max-unkeep-op through ovs-vsctl. | expand |
Bleep bloop. Greetings wenxu, 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 wenxu <wenxu@ucloud.cn> needs to sign off.
WARNING: Line is 80 characters long (recommended limit is 79)
#135 FILE: vswitchd/vswitch.xml:228:
The maximum unkeep operations that for each dump in revalidator thread
Lines checked: 148, Warnings: 1, Errors: 1
Please check this out. If you feel there has been an error, please email aconole@redhat.com
Thanks,
0-day Robot
On Mon, Sep 30, 2019 at 07:15:39PM +0800, wenxu@ucloud.cn wrote: > From: wenxu <wenxu@ucloud.cn> > > This patch adds a new configuration option, "max-unkeep_op" to the > Open_vSwitch "other-config" column. This sets maximum allowed ravalidator > do the UNKEEP operations with limit in each dump. > In the hw-offload mode. The huge number of UNKEEP operationes will > make revalidator block long time. Thanks for working to make OVS perform better. It's best if OVS can perform well without requiring users to tune it by hand. This commit does not try to do that; instead, users have to notice that there is a performance problem, find this particular setting, and then experiment to find the best value. That's a lot of work. Can you try to make this self-tuning, so that users don't have to go through all of that? Second, when OVS does require manual tuning for best performance, we should try to document the settings' purpose and rationale and how to determine their values. The name for this setting is not informative (I do not understand it myself) and the documentation does not explain what it is for, how one should recognize that tuning it is necessary, or how to choose a useful value. Will you please take another look to see if you can improve the patch along those two axes? Thanks again for working to make OVS faster.
On 10/1/2019 4:29 AM, Ben Pfaff wrote: > > On Mon, Sep 30, 2019 at 07:15:39PM +0800, wenxu@ucloud.cn wrote: >> From: wenxu <wenxu@ucloud.cn> >> >> This patch adds a new configuration option, "max-unkeep_op" to the >> Open_vSwitch "other-config" column. This sets maximum allowed ravalidator >> do the UNKEEP operations with limit in each dump. >> In the hw-offload mode. The huge number of UNKEEP operationes will >> make revalidator block long time. > Thanks for working to make OVS perform better. > > It's best if OVS can perform well without requiring users to tune it by > hand. This commit does not try to do that; instead, users have to > notice that there is a performance problem, find this particular > setting, and then experiment to find the best value. That's a lot of > work. Can you try to make this self-tuning, so that users don't have to > go through all of that? > > Second, when OVS does require manual tuning for best performance, we > should try to document the settings' purpose and rationale and how to > determine their values. The name for this setting is not informative (I > do not understand it myself) and the documentation does not explain what > it is for, how one should recognize that tuning it is necessary, or how > to choose a useful value. > > Will you please take another look to see if you can improve the patch > along those two axes? Thanks again for working to make OVS faster. Hi ben, Thx. I found the root cause of the bad performance delete tc flower rule in revalidator threads. All the block is for the netdev_hmap_mutex mutex. In the netdev_ports_get the handler with compete with revlalidator. The netdev_ifindex_to_odp_port and netdev_ports_flow_del with this mutex will bolock revalidator each other. So Maybe replace the hmap to rcuhlist is much better? But there is no such rcuhlist utilitis in the lib
On Tue, Oct 08, 2019 at 03:01:44PM +0800, wenxu wrote: > Thx. I found the root cause of the bad performance delete tc flower > rule in revalidator threads. > > All the block is for the netdev_hmap_mutex mutex. In the > netdev_ports_get the handler with compete with revlalidator. The > netdev_ifindex_to_odp_port and netdev_ports_flow_del with this mutex > will bolock revalidator each other. So Maybe replace the hmap to > rcuhlist is much better? But there is no such rcuhlist utilitis in > the lib The cmap data structure might be better suited for this kind of use. Would it solve the problem, in your view?
On 10/9/2019 1:33 AM, Ben Pfaff wrote: > On Tue, Oct 08, 2019 at 03:01:44PM +0800, wenxu wrote: >> Thx. I found the root cause of the bad performance delete tc flower >> rule in revalidator threads. >> >> All the block is for the netdev_hmap_mutex mutex. In the >> netdev_ports_get the handler with compete with revlalidator. The >> netdev_ifindex_to_odp_port and netdev_ports_flow_del with this mutex >> will bolock revalidator each other. So Maybe replace the hmap to >> rcuhlist is much better? But there is no such rcuhlist utilitis in >> the lib > The cmap data structure might be better suited for this kind of use. > Would it solve the problem, in your view? yes cmap can this. But I find it is more easy to solve the problem through replacing the netdev_hmap_mutex to netdev_hmap_rwlock.
On Wed, Oct 09, 2019 at 02:50:39PM +0800, wenxu wrote: > > On 10/9/2019 1:33 AM, Ben Pfaff wrote: > > On Tue, Oct 08, 2019 at 03:01:44PM +0800, wenxu wrote: > >> Thx. I found the root cause of the bad performance delete tc flower > >> rule in revalidator threads. > >> > >> All the block is for the netdev_hmap_mutex mutex. In the > >> netdev_ports_get the handler with compete with revlalidator. The > >> netdev_ifindex_to_odp_port and netdev_ports_flow_del with this mutex > >> will bolock revalidator each other. So Maybe replace the hmap to > >> rcuhlist is much better? But there is no such rcuhlist utilitis in > >> the lib > > The cmap data structure might be better suited for this kind of use. > > Would it solve the problem, in your view? > > yes cmap can this. But I find it is more easy to solve the problem through replacing > > the netdev_hmap_mutex to netdev_hmap_rwlock. OK. That seems reasonable too.
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index 657aa7f..9423a50 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -2607,6 +2607,7 @@ revalidate(struct revalidator *revalidator) long long int now; size_t n_dp_flows; bool kill_them_all; + unsigned int counter = 0; n_dumped = dpif_flow_dump_next(dump_thread, flows, ARRAY_SIZE(flows)); if (!n_dumped) { @@ -2695,9 +2696,12 @@ revalidate(struct revalidator *revalidator) } if (result != UKEY_KEEP) { - /* Takes ownership of 'recircs'. */ - reval_op_init(&ops[n_ops++], result, udpif, ukey, &recircs, - &odp_actions); + if (!ofproto_max_unkeep_op || + ++counter <= ofproto_max_unkeep_op) { + /* Takes ownership of 'recircs'. */ + reval_op_init(&ops[n_ops++], result, udpif, ukey, &recircs, + &odp_actions); + } } ovs_mutex_unlock(&ukey->mutex); } diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h index c980e6b..bce194a 100644 --- a/ofproto/ofproto-provider.h +++ b/ofproto/ofproto-provider.h @@ -532,6 +532,9 @@ extern unsigned ofproto_max_revalidator; * duration exceeds half of max-revalidator config variable. */ extern unsigned ofproto_min_revalidate_pps; +/* Maximum unkeep op in revalidator.*/ +extern unsigned ofproto_max_unkeep_op; + /* Number of upcall handler and revalidator threads. Only affects the * ofproto-dpif implementation. */ extern size_t n_handlers, n_revalidators; diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index b4249b0..6a4cf2e 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -308,6 +308,7 @@ unsigned ofproto_flow_limit = OFPROTO_FLOW_LIMIT_DEFAULT; unsigned ofproto_max_idle = OFPROTO_MAX_IDLE_DEFAULT; unsigned ofproto_max_revalidator = OFPROTO_MAX_REVALIDATOR_DEFAULT; unsigned ofproto_min_revalidate_pps = OFPROTO_MIN_REVALIDATE_PPS_DEFAULT; +unsigned ofproto_max_unkeep_op = OFPROTO_MAX_UNKEEP_OP_DEFAULT; size_t n_handlers, n_revalidators; @@ -721,6 +722,14 @@ ofproto_set_min_revalidate_pps(unsigned min_revalidate_pps) ofproto_min_revalidate_pps = min_revalidate_pps ? min_revalidate_pps : 1; } +/* Sets the maximum allowed unkeep operation inrevalidator. */ +void +ofproto_set_max_unkeep_op(unsigned max_unkeep_op) +{ + ofproto_max_unkeep_op = max_unkeep_op; +} + + /* If forward_bpdu is true, the NORMAL action will forward frames with * reserved (e.g. STP) destination Ethernet addresses. if forward_bpdu is false, * the NORMAL action will drop these frames. */ diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h index 033c4cf..e1946ff 100644 --- a/ofproto/ofproto.h +++ b/ofproto/ofproto.h @@ -311,6 +311,7 @@ int ofproto_port_dump_done(struct ofproto_port_dump *); #define OFPROTO_MAX_IDLE_DEFAULT 10000 /* ms */ #define OFPROTO_MAX_REVALIDATOR_DEFAULT 500 /* ms */ #define OFPROTO_MIN_REVALIDATE_PPS_DEFAULT 5 +#define OFPROTO_MAX_UNKEEP_OP_DEFAULT 0 const char *ofproto_port_open_type(const struct ofproto *, const char *port_type); @@ -340,6 +341,7 @@ void ofproto_set_flow_limit(unsigned limit); void ofproto_set_max_idle(unsigned max_idle); void ofproto_set_max_revalidator(unsigned max_revalidator); void ofproto_set_min_revalidate_pps(unsigned min_revalidate_pps); +void ofproto_set_max_unkeep_op(unsigned max_unkeep_op); void ofproto_set_forward_bpdu(struct ofproto *, bool forward_bpdu); void ofproto_set_mac_table_config(struct ofproto *, unsigned idle_time, size_t max_entries); diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c index 9095ebf..5248926 100644 --- a/vswitchd/bridge.c +++ b/vswitchd/bridge.c @@ -804,6 +804,9 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg) ofproto_set_min_revalidate_pps( smap_get_int(&ovs_cfg->other_config, "min-revalidate-pps", OFPROTO_MIN_REVALIDATE_PPS_DEFAULT)); + ofproto_set_max_unkeep_op(smap_get_int(&ovs_cfg->other_config, + "max-ukeep-op", + OFPROTO_MAX_UNKEEP_OP_DEFAULT)); ofproto_set_vlan_limit(smap_get_int(&ovs_cfg->other_config, "vlan-limit", LEGACY_MAX_VLAN_HEADERS)); ofproto_set_bundle_idle_timeout(smap_get_int(&ovs_cfg->other_config, diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml index 01304a5..d13297a 100644 --- a/vswitchd/vswitch.xml +++ b/vswitchd/vswitch.xml @@ -222,6 +222,17 @@ </p> </column> + <column name="other_config" key="max-unkeep-op" + type='{"type": "integer", "minInteger": 0}'> + <p> + The maximum unkeep operations that for each dump in revalidator thread + If the vlaue is 0, there is no limitation for unkeep operation + </p> + <p> + The default is 0. + </p> + </column> + <column name="other_config" key="hw-offload" type='{"type": "boolean"}'> <p>
From: wenxu <wenxu@ucloud.cn> This patch adds a new configuration option, "max-unkeep_op" to the Open_vSwitch "other-config" column. This sets maximum allowed ravalidator do the UNKEEP operations with limit in each dump. In the hw-offload mode. The huge number of UNKEEP operationes will make revalidator block long time. --- ofproto/ofproto-dpif-upcall.c | 10 +++++++--- ofproto/ofproto-provider.h | 3 +++ ofproto/ofproto.c | 9 +++++++++ ofproto/ofproto.h | 2 ++ vswitchd/bridge.c | 3 +++ vswitchd/vswitch.xml | 11 +++++++++++ 6 files changed, 35 insertions(+), 3 deletions(-)