[ovs-dev] [PATCH v4 4/9] ct-dpif, dpif-netlink: Add conntrack timeout policy support

Darrell Ball dlu998 at gmail.com
Tue Aug 20 07:46:16 UTC 2019


On Mon, Aug 19, 2019 at 7:41 PM Darrell Ball <dlu998 at gmail.com> wrote:

>
>
> On Mon, Aug 19, 2019 at 12:42 PM Darrell Ball <dlu998 at gmail.com> wrote:
>
>>
>>
>> On Mon, Aug 19, 2019 at 10:52 AM Yi-Hung Wei <yihung.wei at gmail.com>
>> wrote:
>>
>>> On Fri, Aug 16, 2019 at 5:07 PM Darrell Ball <dlu998 at gmail.com> wrote:
>>> >
>>> > Thanks for the patch
>>> >
>>> > Pls let me know if this incremental works for you.
>>> > Main change is logging fix for timeout policy deletion.
>>> >
>>> > Darrell
>>> >
>>> > diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
>>> > index 1d4ee60..00d957b 100644
>>> > --- a/lib/dpif-netlink.c
>>> > +++ b/lib/dpif-netlink.c
>>> > @@ -2822,11 +2822,10 @@ dpif_netlink_ct_dump_done(struct dpif *dpif
>>> OVS_UNUSED,
>>> >                            struct ct_dpif_dump_state *dump_)
>>> >  {
>>> >      struct dpif_netlink_ct_dump_state *dump;
>>> > -    int err;
>>> >
>>> >      INIT_CONTAINER(dump, dump_, up);
>>> >
>>> > -    err = nl_ct_dump_done(dump->nl_ct_dump);
>>> > +    int err = nl_ct_dump_done(dump->nl_ct_dump);
>>> >      free(dump);
>>> >      return err;
>>> >  }
>>> > @@ -3335,7 +3334,8 @@ dpif_netlink_ct_del_timeout_policy(struct dpif
>>> *dpif OVS_UNUSED,
>>> >              err = 0;
>>> >          }
>>> >          if (err) {
>>> > -            VLOG_WARN_RL(&error_rl, "failed to delete timeout policy
>>> %s (%s)",
>>> > +            static struct vlog_rate_limit rl =
>>> VLOG_RATE_LIMIT_INIT(1, 1);
>>>
>>> Thanks for the diff.  It looks good in general.
>>>
>>> I agree on the main concern of the proposed diff which is the original
>>> rate limit in dpif-netlink (VLOG_RATE_LIMIT_INIT(9999, 5)) may log too
>>> much duplicated information.  However, since we may delete more than
>>> one one timeout policy in a minute, so lowering the rate limit to
>>> VLOG_RATE_LIMIT_INIT(1, 1) may miss some useful information.   I would
>>> use somewhere in the between (VLOG_RATE_LIMIT_INIT(5, 5)) in the next
>>> version.
>>>
>>
>> TBH, I am not sure we care lots about this information. I was even
>> debating changing it debug level.
>> We have 4 billion datapath timeout profile IDs, so it is unlikely we will
>> run out.
>> Eventually, they will get cleaned up by the retry thingy.
>>
>> Also, I am not sure what action we will take by seeing these logs anyhow.
>>
>> Spamming the log is more of a concern.
>>
>
> After more testing, I noticed there are a couple other aspects we might
> have overlooked.
>
> 1/ We don't have a log for a successful deletion attempt.
>
> 2/ When we try to delete 1 of the 6 associated Netfilter timeout policies
> and it fails, we don't try to delete
>  the remaining ones and bail out of the deletion loop early.
>
> Keeping the INFO level change (from WARN), addressing '1' and '2' above
> and also folding in
> your idea to keep the overall log generation rate a little higher, I ended
> up with:
>
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index 1d4ee60..3926cfd 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -2822,11 +2822,10 @@ dpif_netlink_ct_dump_done(struct dpif *dpif
> OVS_UNUSED,
>                            struct ct_dpif_dump_state *dump_)
>  {
>      struct dpif_netlink_ct_dump_state *dump;
> -    int err;
>
>      INIT_CONTAINER(dump, dump_, up);
>
> -    err = nl_ct_dump_done(dump->nl_ct_dump);
> +    int err = nl_ct_dump_done(dump->nl_ct_dump);
>      free(dump);
>      return err;
>  }
> @@ -3334,14 +3333,12 @@ dpif_netlink_ct_del_timeout_policy(struct dpif
> *dpif OVS_UNUSED,
>          if (err == ENOENT) {
>              err = 0;
>          }
> -        if (err) {
> -            VLOG_WARN_RL(&error_rl, "failed to delete timeout policy %s
> (%s)",
> -                         ds_cstr(&nl_tp_name), ovs_strerror(err));
> -            goto out;
> -        }
> +        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(6, 6);
> +        VLOG_INFO_RL(&rl, err ? "failed to delete timeout policy %s (%s)"
> +                              : "deleted timeout policy %s (%s)",
> +                     ds_cstr(&nl_tp_name), ovs_strerror(err));
>      }
>
> -out:
>      ds_destroy(&nl_tp_name);
>      return err;
>  }
> @@ -3392,10 +3389,9 @@ dpif_netlink_ct_timeout_policy_dump_start(struct
> dpif *dpif OVS_UNUSED,
>                                            void **statep)
>  {
>      struct dpif_netlink_ct_timeout_policy_dump_state *dump_state;
> -    int err;
>
>      *statep = dump_state = xzalloc(sizeof *dump_state);
> -    err = nl_ct_timeout_policy_dump_start(&dump_state->nl_dump_state);
> +    int err = nl_ct_timeout_policy_dump_start(&dump_state->nl_dump_state);
>      if (err) {
>          free(dump_state);
>          return err;
>

> Running the system test (from Patch 9) and adding a deletion request for
> the in-use timeout policy yields
>
> > 2019-08-20T01:28:41.004Z|00203|dpif_netlink|INFO|deleted timeout policy
> ovs_tp_0_tcp4 (Success)
> > 2019-08-20T01:28:41.004Z|00204|dpif_netlink|INFO|failed to delete
> timeout policy ovs_tp_0_udp4 (Device or resource busy)
> > 2019-08-20T01:28:41.004Z|00205|dpif_netlink|INFO|failed to delete
> timeout policy ovs_tp_0_icmp4 (Device or resource busy)
> > 2019-08-20T01:28:41.004Z|00206|dpif_netlink|INFO|deleted timeout policy
> ovs_tp_0_tcp6 (Success)
> > 2019-08-20T01:28:41.004Z|00207|dpif_netlink|INFO|deleted timeout policy
> ovs_tp_0_udp6 (Success)
> > 2019-08-20T01:28:41.004Z|00208|dpif_netlink|INFO|deleted timeout policy
> ovs_tp_0_icmpv6 (Success)
>

After fixing a bug in my proposed incremental and adding tracking of an
already removed sub timeout policy:
Pls double check.

diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index 1d4ee60..cba4432 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -2822,11 +2822,10 @@ dpif_netlink_ct_dump_done(struct dpif *dpif
OVS_UNUSED,
                           struct ct_dpif_dump_state *dump_)
 {
     struct dpif_netlink_ct_dump_state *dump;
-    int err;

     INIT_CONTAINER(dump, dump_, up);

-    err = nl_ct_dump_done(dump->nl_ct_dump);
+    int err = nl_ct_dump_done(dump->nl_ct_dump);
     free(dump);
     return err;
 }
@@ -3319,7 +3318,8 @@ out:
 }

 /* Returns 0 if all the sub timeout policies are deleted or
- * not exist in the kernel. */
+ * not exist in the kernel; returns 1 if any sub timeout policy deletion
+ * failed. */
 static int
 dpif_netlink_ct_del_timeout_policy(struct dpif *dpif OVS_UNUSED,
                                    uint32_t tp_id)
@@ -3330,18 +3330,19 @@ dpif_netlink_ct_del_timeout_policy(struct dpif
*dpif OVS_UNUSED,
     for (int i = 0; i < ARRAY_SIZE(tp_protos); ++i) {
         dpif_netlink_format_tp_name(tp_id, tp_protos[i].l3num,
                                     tp_protos[i].l4num, &nl_tp_name);
-        err = nl_ct_del_timeout_policy(ds_cstr(&nl_tp_name));
-        if (err == ENOENT) {
-            err = 0;
-        }
-        if (err) {
-            VLOG_WARN_RL(&error_rl, "failed to delete timeout policy %s
(%s)",
-                         ds_cstr(&nl_tp_name), ovs_strerror(err));
-            goto out;
+        int err2 = nl_ct_del_timeout_policy(ds_cstr(&nl_tp_name));
+        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(6, 6);
+        VLOG_INFO_RL(&rl, err2 == ENOENT
+                     ? "timeout policy already removed %s (%s)"
+                     : !err2 ? "deleted timeout policy %s (%s)"
+                             : "failed to delete timeout policy %s (%s)",
+                     ds_cstr(&nl_tp_name), ovs_strerror(err2));
+        if (err2 == ENOENT) {
+            err2 = 0;
         }
+        err = err || err2;
     }

-out:
     ds_destroy(&nl_tp_name);
     return err;
 }
@@ -3392,10 +3393,9 @@ dpif_netlink_ct_timeout_policy_dump_start(struct
dpif *dpif OVS_UNUSED,
                                           void **statep)
 {
     struct dpif_netlink_ct_timeout_policy_dump_state *dump_state;
-    int err;

     *statep = dump_state = xzalloc(sizeof *dump_state);
-    err = nl_ct_timeout_policy_dump_start(&dump_state->nl_dump_state);
+    int err = nl_ct_timeout_policy_dump_start(&dump_state->nl_dump_state);
     if (err) {
         free(dump_state);
         return err;
(END)

Running the system test (from Patch 9) and adding a deletion request for
the in-use timeout policy yields

> 2019-08-20T07:39:41.016Z|00211|dpif_netlink|INFO|deleted timeout policy
ovs_tp_0_tcp4 (Success)
> 2019-08-20T07:39:41.016Z|00212|dpif_netlink|INFO|failed to delete timeout
policy ovs_tp_0_udp4 (Device or resource busy)
> 2019-08-20T07:39:41.016Z|00213|dpif_netlink|INFO|failed to delete timeout
policy ovs_tp_0_icmp4 (Device or resource busy)
> 2019-08-20T07:39:41.016Z|00214|dpif_netlink|INFO|deleted timeout policy
ovs_tp_0_tcp6 (Success)
> 2019-08-20T07:39:41.016Z|00215|dpif_netlink|INFO|deleted timeout policy
ovs_tp_0_udp6 (Success)
> 2019-08-20T07:39:41.016Z|00216|dpif_netlink|INFO|deleted timeout policy
ovs_tp_0_icmpv6 (Success)


>
>
>
>
>>
>>
>>>
>>> Thanks,
>>>
>>> -Yi-Hung
>>>
>>


More information about the dev mailing list