| Message ID | 1578555406-42564-1-git-send-email-xiangxia.m.yue@gmail.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [ovs-dev,ovs-dev,v3] dpif-netdev: Allow to set max capacity of flow on netdev. | expand |
On 09.01.2020 08:36, xiangxia.m.yue@gmail.com wrote: > From: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > For installing more than MAX_FLOWS (65536) flows to netdev datapath. > Add the ovs-appctl subcommand "dpif-netdev/pmd-set-max-flow" which > can change the flow number which netdev datapath support. > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> Hi. I'm wondering why we need the flow limit on the datapath level at all? MAX_FLOWS constant was there from the introduction of dpif-netdev, however, later new flow-limit mechanism was implemented that controls number of datapath flows in a dynamic way on ofproto level. So, maybe we can just remove the limit and fully rely on ofproto to decide what flow limit we need? There are no limitations for flow table size in dpif-netdev beside the artificial one. 'other_config:flow-limit' seems suitable to control this. Ben, what do you think? Best regards, Ilya Maximets.
On Thu, Jan 09, 2020 at 04:14:10PM +0100, Ilya Maximets wrote: > On 09.01.2020 08:36, xiangxia.m.yue@gmail.com wrote: > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > > > For installing more than MAX_FLOWS (65536) flows to netdev datapath. > > Add the ovs-appctl subcommand "dpif-netdev/pmd-set-max-flow" which > > can change the flow number which netdev datapath support. > > > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > Hi. > > I'm wondering why we need the flow limit on the datapath level at all? > > MAX_FLOWS constant was there from the introduction of dpif-netdev, > however, later new flow-limit mechanism was implemented that > controls number of datapath flows in a dynamic way on ofproto level. > > So, maybe we can just remove the limit and fully rely on ofproto > to decide what flow limit we need? There are no limitations for > flow table size in dpif-netdev beside the artificial one. > 'other_config:flow-limit' seems suitable to control this. > > Ben, what do you think? Hmm, that's a good point. Tonghao, do you have a good reason to want to limit flows at this level?
On Thu, Jan 9, 2020 at 11:14 PM Ilya Maximets <i.maximets@ovn.org> wrote: > > On 09.01.2020 08:36, xiangxia.m.yue@gmail.com wrote: > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > > > For installing more than MAX_FLOWS (65536) flows to netdev datapath. > > Add the ovs-appctl subcommand "dpif-netdev/pmd-set-max-flow" which > > can change the flow number which netdev datapath support. > > > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > Hi. > > I'm wondering why we need the flow limit on the datapath level at all? > > MAX_FLOWS constant was there from the introduction of dpif-netdev, > however, later new flow-limit mechanism was implemented that > controls number of datapath flows in a dynamic way on ofproto level. > > So, maybe we can just remove the limit and fully rely on ofproto > to decide what flow limit we need? There are no limitations for > flow table size in dpif-netdev beside the artificial one. > 'other_config:flow-limit' seems suitable to control this. Hi all, that is good idea. But the reason that I change MAX_FLOWS to a var is that we install the rule via "ovs-appctl dpctl/add-flow" in our product environment, and there may be too many rules(software limit for hardware flow limit). ofproto layer is complicated for some developers. Using the "ovs-appctl dpctl/add-flow" may be easy to control the flows as I know. And we do some work on dpctl, for example, revalidator thread does not delete the flow we installed via ovs-appctl. > Ben, what do you think? > > Best regards, Ilya Maximets.
On Fri, Jan 10, 2020 at 2:04 AM Ben Pfaff <blp@ovn.org> wrote: > > On Thu, Jan 09, 2020 at 04:14:10PM +0100, Ilya Maximets wrote: > > On 09.01.2020 08:36, xiangxia.m.yue@gmail.com wrote: > > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > > > > > For installing more than MAX_FLOWS (65536) flows to netdev datapath. > > > Add the ovs-appctl subcommand "dpif-netdev/pmd-set-max-flow" which > > > can change the flow number which netdev datapath support. > > > > > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > > > Hi. > > > > I'm wondering why we need the flow limit on the datapath level at all? > > > > MAX_FLOWS constant was there from the introduction of dpif-netdev, > > however, later new flow-limit mechanism was implemented that > > controls number of datapath flows in a dynamic way on ofproto level. > > > > So, maybe we can just remove the limit and fully rely on ofproto > > to decide what flow limit we need? There are no limitations for > > flow table size in dpif-netdev beside the artificial one. > > 'other_config:flow-limit' seems suitable to control this. > > > > Ben, what do you think? > > Hmm, that's a good point. > > Tonghao, do you have a good reason to want to limit flows at this level? Hi Ben, as I explained, we don't use the ofproto layer, and use the ovs-appctl to install flow and offload them to hardware. so the "other_config:flow-limit" and "should_install_flow" function is called when upcall install the rules, and can't limit it in dpcls layer as I know. I am not sure installing rule via ovs-appctl is welcome.
On 10.01.2020 03:16, Tonghao Zhang wrote: > On Fri, Jan 10, 2020 at 2:04 AM Ben Pfaff <blp@ovn.org> wrote: >> >> On Thu, Jan 09, 2020 at 04:14:10PM +0100, Ilya Maximets wrote: >>> On 09.01.2020 08:36, xiangxia.m.yue@gmail.com wrote: >>>> From: Tonghao Zhang <xiangxia.m.yue@gmail.com> >>>> >>>> For installing more than MAX_FLOWS (65536) flows to netdev datapath. >>>> Add the ovs-appctl subcommand "dpif-netdev/pmd-set-max-flow" which >>>> can change the flow number which netdev datapath support. >>>> >>>> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> >>> >>> Hi. >>> >>> I'm wondering why we need the flow limit on the datapath level at all? >>> >>> MAX_FLOWS constant was there from the introduction of dpif-netdev, >>> however, later new flow-limit mechanism was implemented that >>> controls number of datapath flows in a dynamic way on ofproto level. >>> >>> So, maybe we can just remove the limit and fully rely on ofproto >>> to decide what flow limit we need? There are no limitations for >>> flow table size in dpif-netdev beside the artificial one. >>> 'other_config:flow-limit' seems suitable to control this. >>> >>> Ben, what do you think? >> >> Hmm, that's a good point. >> >> Tonghao, do you have a good reason to want to limit flows at this level? > Hi Ben, as I explained, we don't use the ofproto layer, and use the > ovs-appctl to install flow and > offload them to hardware. so the "other_config:flow-limit" and > "should_install_flow" function is called > when upcall install the rules, and can't limit it in dpcls layer as I > know. I am not sure installing rule via ovs-appctl > is welcome. As far as I understand, revalidators will start flow eviction if flow-limit is reached while dumping datapath flows. So, you should start seeing flow deletion events if you'll reach ~200K flows installed in dpif-netdev. Anyway, why you need a flow limit on datapath level if you're managing them in so tricky way? Best regards, Ilya Maximets.
On Sat, Jan 11, 2020 at 12:44 AM Ilya Maximets <i.maximets@ovn.org> wrote: > > On 10.01.2020 03:16, Tonghao Zhang wrote: > > On Fri, Jan 10, 2020 at 2:04 AM Ben Pfaff <blp@ovn.org> wrote: > >> > >> On Thu, Jan 09, 2020 at 04:14:10PM +0100, Ilya Maximets wrote: > >>> On 09.01.2020 08:36, xiangxia.m.yue@gmail.com wrote: > >>>> From: Tonghao Zhang <xiangxia.m.yue@gmail.com> > >>>> > >>>> For installing more than MAX_FLOWS (65536) flows to netdev datapath. > >>>> Add the ovs-appctl subcommand "dpif-netdev/pmd-set-max-flow" which > >>>> can change the flow number which netdev datapath support. > >>>> > >>>> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> > >>> > >>> Hi. > >>> > >>> I'm wondering why we need the flow limit on the datapath level at all? > >>> > >>> MAX_FLOWS constant was there from the introduction of dpif-netdev, > >>> however, later new flow-limit mechanism was implemented that > >>> controls number of datapath flows in a dynamic way on ofproto level. > >>> > >>> So, maybe we can just remove the limit and fully rely on ofproto > >>> to decide what flow limit we need? There are no limitations for > >>> flow table size in dpif-netdev beside the artificial one. > >>> 'other_config:flow-limit' seems suitable to control this. > >>> > >>> Ben, what do you think? > >> > >> Hmm, that's a good point. > >> > >> Tonghao, do you have a good reason to want to limit flows at this level? > > Hi Ben, as I explained, we don't use the ofproto layer, and use the > > ovs-appctl to install flow and > > offload them to hardware. so the "other_config:flow-limit" and > > "should_install_flow" function is called > > when upcall install the rules, and can't limit it in dpcls layer as I > > know. I am not sure installing rule via ovs-appctl > > is welcome. > > As far as I understand, revalidators will start flow eviction if flow-limit > is reached while dumping datapath flows. So, you should start seeing > flow deletion events if you'll reach ~200K flows installed in dpif-netdev. As I said, we hope revalidators don't delete the rules if the rules is installed by ovs-appctl, but not ofproto. and the rules can be deleted by ovs-apply only. (Other patch can support this function.) And this patch support the limit for hardware limit, for example hardware support 100K flows, and we can set limit to 100K to dpif-netdev. This is easy to manage the flow for us, and that is why we use this way to install/delete flows. I am not sure ovs community can support it. and this patch is useful ? > Anyway, why you need a flow limit on datapath level if you're managing them > in so tricky way? > > Best regards, Ilya Maximets.
diff --git a/NEWS b/NEWS index 965facaf852d..55bd1255104d 100644 --- a/NEWS +++ b/NEWS @@ -8,6 +8,9 @@ Post-v2.12.0 * Add option to enable, disable and query TCP sequence checking in conntrack. * Add support for conntrack zone limits. + * New ovs-appctl "dpif-netdev/pmd-set-max-flow" and + "dpif-netdev/pmd-show-max-flow" commands for userspace datapath + change the max flows which support. - AF_XDP: * New option 'use-need-wakeup' for netdev-afxdp to control enabling of corresponding 'need_wakeup' flag in AF_XDP rings. Enabled by default diff --git a/lib/dpif-netdev-unixctl.man b/lib/dpif-netdev-unixctl.man index 6c54f6f9cc3b..92cbcd07e984 100644 --- a/lib/dpif-netdev-unixctl.man +++ b/lib/dpif-netdev-unixctl.man @@ -217,3 +217,7 @@ with port names, which this thread polls. . .IP "\fBdpif-netdev/pmd-rxq-rebalance\fR [\fIdp\fR]" Reassigns rxqs to pmds in the datapath \fIdp\fR based on their current usage. +.IP "\fBdpif-netdev/pmd-set-max-flow\fR [\fInumber\fR]" +Sets the max flow which netdev datapath supports (default 65536). +.IP "\fBdpif-netdev/pmd-show-max-flow" +Shows the current max flow which netdev datapath supports. diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 24218210d4a8..f789ccf6cc8b 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -97,7 +97,6 @@ DEFINE_STATIC_PER_THREAD_DATA(uint32_t, recirc_depth, 0) #define DEFAULT_TX_FLUSH_INTERVAL 0 /* Configuration parameters. */ -enum { MAX_FLOWS = 65536 }; /* Maximum number of flows in flow table. */ enum { MAX_METERS = 65536 }; /* Maximum number of meters. */ enum { MAX_BANDS = 8 }; /* Maximum number of bands / meter. */ enum { N_METER_LOCKS = 64 }; /* Maximum number of meters. */ @@ -116,6 +115,9 @@ COVERAGE_DEFINE(datapath_drop_rx_invalid_packet); /* Protects against changes to 'dp_netdevs'. */ static struct ovs_mutex dp_netdev_mutex = OVS_MUTEX_INITIALIZER; +/* Maximum number of flows in flow table. */ +static atomic_uint32_t netdev_max_flow = ATOMIC_VAR_INIT(65536); + /* Contains all 'struct dp_netdev's. */ static struct shash dp_netdevs OVS_GUARDED_BY(dp_netdev_mutex) = SHASH_INITIALIZER(&dp_netdevs); @@ -1123,6 +1125,39 @@ pmd_info_show_perf(struct ds *reply, } } +static void +dpif_netdev_set_max_flow(struct unixctl_conn *conn, + int argc OVS_UNUSED, + const char *argv[], + void *aux OVS_UNUSED) +{ + long long max_flow = atoll(argv[1]); + + if (max_flow <= 0 || max_flow >= UINT32_MAX) { + unixctl_command_reply_error(conn, + "max-flow should: > 0 and < UINT32_MAX"); + return; + } + + atomic_store_relaxed(&netdev_max_flow, max_flow); + unixctl_command_reply(conn, NULL); +} + +static void +dpif_netdev_show_max_flow(struct unixctl_conn *conn, + int argc OVS_UNUSED, + const char *argv[] OVS_UNUSED, + void *aux OVS_UNUSED) +{ + uint32_t max_flow; + + atomic_read_relaxed(&netdev_max_flow, &max_flow); + + char *reply = xasprintf("%u", max_flow); + unixctl_command_reply(conn, reply); + free(reply); +} + static int compare_poll_list(const void *a_, const void *b_) { @@ -1427,6 +1462,14 @@ dpif_netdev_init(void) "[-us usec] [-q qlen]", 0, 10, pmd_perf_log_set_cmd, NULL); + unixctl_command_register("dpif-netdev/pmd-set-max-flow", + "number", + 1, 1, dpif_netdev_set_max_flow, + NULL); + unixctl_command_register("dpif-netdev/pmd-show-max-flow", + "", + 0, 0, dpif_netdev_show_max_flow, + NULL); return 0; } @@ -3356,7 +3399,9 @@ flow_put_on_pmd(struct dp_netdev_pmd_thread *pmd, netdev_flow = dp_netdev_pmd_lookup_flow(pmd, key, NULL); if (!netdev_flow) { if (put->flags & DPIF_FP_CREATE) { - if (cmap_count(&pmd->flow_table) < MAX_FLOWS) { + uint32_t max_flow; + atomic_read_relaxed(&netdev_max_flow, &max_flow); + if (cmap_count(&pmd->flow_table) < max_flow) { dp_netdev_flow_add(pmd, match, ufid, put->actions, put->actions_len); error = 0;