Re: [ovs-dev] [PATCH v4 4/9] ct-dpif, dpif-netlink: Add conntrack timeout policy support
On Tue, Aug 20, 2019 at 12:30 PM Yi-Hung Wei wrote:
> On Tue, Aug 20, 2019 at 12:46 AM Darrell Ball wrote:
> > After fixing a bug in my proposed incremental and adding tracking of an
> already removed sub timeout policy:
> > Pls double check.
>
> Thanks for the proposed incremental.
>
> I checked all the other logging places in dpif-netlink, we usually do
> not log the successfully cases in the INFO level. As the discussion
> in the e-mail thread, I think the successful cases does not provide
> much useful information, so I made some minor changes based on the
> proposed incremental. I will fold in the following diff.
>
Looks good
As mentioned earlier, tracking the timeout profile deletion timing at INFO
level is
not that important in general. So, as long as we don't spam the log, this
part should
be fine.
>
> Thanks,
>
> -Yi-Hung
>
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index 1d4ee60bd199..85827cd65503 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;
> }
> @@ -3318,32 +3317,32 @@ out:
> return err;
> }
>
> -/* Returns 0 if all the sub timeout policies are deleted or
> - * not exist in the kernel. */
> +/* Returns 0 if all the sub timeout policies are deleted or 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)
> {
> struct ds nl_tp_name = DS_EMPTY_INITIALIZER;
> -int err = 0;
> +int ret = 0;
>
> 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, _tp_name);
> -err = nl_ct_del_timeout_policy(ds_cstr(_tp_name));
> +int err = nl_ct_del_timeout_policy(ds_cstr(_tp_name));
> if (err == ENOENT) {
> err = 0;
> }
> if (err) {
> -VLOG_WARN_RL(_rl, "failed to delete timeout policy %s
> (%s)",
> +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(6, 6);
> +VLOG_INFO_RL(, "failed to delete timeout policy %s (%s)",
> ds_cstr(_tp_name), ovs_strerror(err));
> -goto out;
> +ret = 1;
> }
> }
>
> -out:
> ds_destroy(_tp_name);
> -return err;
> +return ret;
> }
>
> struct dpif_netlink_ct_timeout_policy_dump_state {
> @@ -3392,10 +3391,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(_state->nl_dump_state);
> +int err = nl_ct_timeout_policy_dump_start(_state->nl_dump_state);
> if (err) {
> free(dump_state);
> return err;
>
> <- end of diff
> -->
>
> >
> > 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, _tp_name);
> > -err = nl_ct_del_timeout_policy(ds_cstr(_tp_name));
> > -if (err == ENOENT) {
> > -err = 0;
> > -}
> > -if (err) {
> > -VLOG_WARN_RL(_rl, "failed to delete timeout policy %s
> (%s)",
> > -
Re: [ovs-dev] [PATCH v4 4/9] ct-dpif, dpif-netlink: Add conntrack timeout policy support
On Tue, Aug 20, 2019 at 12:46 AM Darrell Ball wrote:
> After fixing a bug in my proposed incremental and adding tracking of an
> already removed sub timeout policy:
> Pls double check.
Thanks for the proposed incremental.
I checked all the other logging places in dpif-netlink, we usually do
not log the successfully cases in the INFO level. As the discussion
in the e-mail thread, I think the successful cases does not provide
much useful information, so I made some minor changes based on the
proposed incremental. I will fold in the following diff.
Thanks,
-Yi-Hung
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index 1d4ee60bd199..85827cd65503 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;
}
@@ -3318,32 +3317,32 @@ out:
return err;
}
-/* Returns 0 if all the sub timeout policies are deleted or
- * not exist in the kernel. */
+/* Returns 0 if all the sub timeout policies are deleted or 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)
{
struct ds nl_tp_name = DS_EMPTY_INITIALIZER;
-int err = 0;
+int ret = 0;
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, _tp_name);
-err = nl_ct_del_timeout_policy(ds_cstr(_tp_name));
+int err = nl_ct_del_timeout_policy(ds_cstr(_tp_name));
if (err == ENOENT) {
err = 0;
}
if (err) {
-VLOG_WARN_RL(_rl, "failed to delete timeout policy %s (%s)",
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(6, 6);
+VLOG_INFO_RL(, "failed to delete timeout policy %s (%s)",
ds_cstr(_tp_name), ovs_strerror(err));
-goto out;
+ret = 1;
}
}
-out:
ds_destroy(_tp_name);
-return err;
+return ret;
}
struct dpif_netlink_ct_timeout_policy_dump_state {
@@ -3392,10 +3391,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(_state->nl_dump_state);
+int err = nl_ct_timeout_policy_dump_start(_state->nl_dump_state);
if (err) {
free(dump_state);
return err;
<- end of diff -->
>
> 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, _tp_name);
> -err = nl_ct_del_timeout_policy(ds_cstr(_tp_name));
> -if (err == ENOENT) {
> -err = 0;
> -}
> -if (err) {
> -VLOG_WARN_RL(_rl, "failed to delete timeout policy %s
> (%s)",
> - ds_cstr(_tp_name), ovs_strerror(err));
> -goto out;
> +int err2 = nl_ct_del_timeout_policy(ds_cstr(_tp_name));
> +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(6, 6);
> +VLOG_INFO_RL(, err2 == ENOENT
> + ? "timeout policy already removed %s (%s)"
> + : !err2 ? "deleted timeout policy %s (%s)"
> + : "failed to delete timeout policy %s (%s)",
> +
Re: [ovs-dev] [PATCH v4 4/9] ct-dpif, dpif-netlink: Add conntrack timeout policy support
On Mon, Aug 19, 2019 at 7:41 PM Darrell Ball wrote:
>
>
> On Mon, Aug 19, 2019 at 12:42 PM Darrell Ball wrote:
>
>>
>>
>> On Mon, Aug 19, 2019 at 10:52 AM Yi-Hung Wei
>> wrote:
>>
>>> On Fri, Aug 16, 2019 at 5:07 PM Darrell Ball 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(_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(, 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 +,12 @@ dpif_netlink_ct_del_timeout_policy(struct dpif
> *dpif OVS_UNUSED,
> if (err == ENOENT) {
> err = 0;
> }
> -if (err) {
> -VLOG_WARN_RL(_rl, "failed to delete timeout policy %s
> (%s)",
> - ds_cstr(_tp_name), ovs_strerror(err));
> -goto out;
> -}
> +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(6, 6);
> +VLOG_INFO_RL(, err ? "failed to delete timeout policy %s (%s)"
> + : "deleted timeout policy %s (%s)",
> + ds_cstr(_tp_name), ovs_strerror(err));
> }
>
> -out:
> ds_destroy(_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(_state->nl_dump_state);
> +int err = nl_ct_timeout_policy_dump_start(_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
Re: [ovs-dev] [PATCH v4 4/9] ct-dpif, dpif-netlink: Add conntrack timeout policy support
On Mon, Aug 19, 2019 at 12:42 PM Darrell Ball wrote:
>
>
> On Mon, Aug 19, 2019 at 10:52 AM Yi-Hung Wei wrote:
>
>> On Fri, Aug 16, 2019 at 5:07 PM Darrell Ball 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(_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(, 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 +,12 @@ dpif_netlink_ct_del_timeout_policy(struct dpif
*dpif OVS_UNUSED,
if (err == ENOENT) {
err = 0;
}
-if (err) {
-VLOG_WARN_RL(_rl, "failed to delete timeout policy %s
(%s)",
- ds_cstr(_tp_name), ovs_strerror(err));
-goto out;
-}
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(6, 6);
+VLOG_INFO_RL(, err ? "failed to delete timeout policy %s (%s)"
+ : "deleted timeout policy %s (%s)",
+ ds_cstr(_tp_name), ovs_strerror(err));
}
-out:
ds_destroy(_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(_state->nl_dump_state);
+int err = nl_ct_timeout_policy_dump_start(_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)
>
Re: [ovs-dev] [PATCH v4 4/9] ct-dpif, dpif-netlink: Add conntrack timeout policy support
On Mon, Aug 19, 2019 at 10:52 AM Yi-Hung Wei wrote:
> On Fri, Aug 16, 2019 at 5:07 PM Darrell Ball 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(_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(, 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.
>
> Thanks,
>
> -Yi-Hung
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4 4/9] ct-dpif, dpif-netlink: Add conntrack timeout policy support
On Fri, Aug 16, 2019 at 5:07 PM Darrell Ball 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(_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(, 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.
Thanks,
-Yi-Hung
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4 4/9] ct-dpif, dpif-netlink: Add conntrack timeout policy support
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(_rl, "failed to delete timeout policy %s
(%s)",
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
+VLOG_INFO_RL(, "failed to delete timeout policy %s (%s)",
ds_cstr(_tp_name), ovs_strerror(err));
goto out;
}
@@ -3392,10 +3392,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(_state->nl_dump_state);
+int err = nl_ct_timeout_policy_dump_start(_state->nl_dump_state);
if (err) {
free(dump_state);
return err;
On Thu, Aug 15, 2019 at 12:35 PM Yi-Hung Wei wrote:
> This patch first defines the dpif interface for a datapath to support
> adding, deleting, getting and dumping conntrack timeout policy.
> The timeout policy is identified by a 4 bytes unsigned integer in
> datapath, and it currently support timeout for TCP, UDP, and ICMP
> protocols.
>
> Moreover, this patch provides the implementation for Linux kernel
> datapath in dpif-netlink.
>
> In Linux kernel, the timeout policy is maintained per L3/L4 protocol,
> and it is identified by 32 bytes null terminated string. On the other
> hand, in vswitchd, the timeout policy is a generic one that consists of
> all the supported L4 protocols. Therefore, one of the main task in
> dpif-netlink is to break down the generic timeout policy into 6
> sub policies (ipv4 tcp, udp, icmp, and ipv6 tcp, udp, icmp),
> and push down the configuration using the netlink API in
> netlink-conntrack.c.
>
> This patch also adds missing symbols in the windows datapath so
> that the build on windows can pass.
>
> Appveyor CI:
> * https://ci.appveyor.com/project/YiHungWei/ovs/builds/26387754
>
> Signed-off-by: Yi-Hung Wei
> Acked-by: Alin Gabriel Serdean
> ---
> Documentation/faq/releases.rst | 3 +-
> datapath-windows/include/OvsDpInterfaceCtExt.h | 114 +
> datapath-windows/ovsext/Netlink/NetlinkProto.h | 8 +-
> include/windows/automake.mk| 1 +
> .../windows/linux/netfilter/nfnetlink_cttimeout.h | 0
> lib/ct-dpif.c | 102 +
> lib/ct-dpif.h | 56 +++
> lib/dpif-netdev.c | 6 +
> lib/dpif-netlink.c | 478
> +
> lib/dpif-netlink.h | 1 -
> lib/dpif-provider.h| 44 ++
> lib/netlink-conntrack.c| 301 +
> lib/netlink-conntrack.h| 27 +-
> lib/netlink-protocol.h | 8 +-
> 14 files changed, 1142 insertions(+), 7 deletions(-)
> create mode 100644 include/windows/linux/netfilter/nfnetlink_cttimeout.h
>
> diff --git a/Documentation/faq/releases.rst
> b/Documentation/faq/releases.rst
> index 8daa23bb2d0c..0b7eaab1b143 100644
> --- a/Documentation/faq/releases.rst
> +++ b/Documentation/faq/releases.rst
> @@ -110,8 +110,9 @@ Q: Are all features available with all datapaths?
> == == == =
> ===
> Connection tracking 4.3YES YES
> YES
> Conntrack Fragment Reass. 4.3YES YES
> YES
> +Conntrack Timeout Policies 5.2YES NO
> NO
> +Conntrack Zone Limit4.18 YES NO
> YES
> NAT 4.6YES YES
> YES
> -Conntrack zone limit4.18 YES NO
> YES
> Tunnel - LISP NO YES NO
> NO
> Tunnel - STTNO YES NO
> YES
> Tunnel - GRE3.11 YES YES
> YES
> diff --git a/datapath-windows/include/OvsDpInterfaceCtExt.h
>
