diff mbox series

[ovs-dev,ovs-dev,v3] dpif-netdev: Allow to set max capacity of flow on netdev.

Message ID 1578555406-42564-1-git-send-email-xiangxia.m.yue@gmail.com
State New
Headers show
Series [ovs-dev,ovs-dev,v3] dpif-netdev: Allow to set max capacity of flow on netdev. | expand

Commit Message

Tonghao Zhang Jan. 9, 2020, 7:36 a.m. UTC
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>
---
v3:
* change the UINT_MAX to UINT32_MAX
* add information to NEWS/dpif-netdev-unixctl.man
* simply the codes and remove new-line at the end 
v2:
* change int type to atomic_uint32_t
* check max flow number is whether valid (0 < max-flow < UINT_MAX).
---
 NEWS                        |  3 +++
 lib/dpif-netdev-unixctl.man |  4 +++
 lib/dpif-netdev.c           | 49 +++++++++++++++++++++++++++++++++++--
 3 files changed, 54 insertions(+), 2 deletions(-)

Comments

Ilya Maximets Jan. 9, 2020, 3:14 p.m. UTC | #1
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.
Ben Pfaff Jan. 9, 2020, 6:04 p.m. UTC | #2
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?
Tonghao Zhang Jan. 10, 2020, 2:02 a.m. UTC | #3
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.
Tonghao Zhang Jan. 10, 2020, 2:16 a.m. UTC | #4
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.
Ilya Maximets Jan. 10, 2020, 4:44 p.m. UTC | #5
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.
Tonghao Zhang Jan. 13, 2020, 2:18 a.m. UTC | #6
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 mbox series

Patch

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;