Re: [ovs-dev] [PATCH v3 4/9] ct-dpif, dpif-netlink: Add conntrack timeout policy support
On Wed, Aug 14, 2019 at 2:35 PM Darrell Ball wrote:
> On Mon, Aug 12, 2019 at 5:54 PM Yi-Hung Wei wrote:
>> --- a/lib/dpif-netlink.c
>> +++ b/lib/dpif-netlink.c
>> +static int
>> +dpif_netlink_ct_set_timeout_policy(struct dpif *dpif OVS_UNUSED,
>> + const struct ct_dpif_timeout_policy *tp)
>> +{
>> +struct nl_ct_timeout_policy nl_tp;
>> +struct ds nl_tp_name = DS_EMPTY_INITIALIZER;
>> +int i, err = 0;
>> +
>> +for (i = 0; i < ARRAY_SIZE(tp_protos); ++i) {
>
>
> for (int i = 0; i < ARRAY_SIZE(tp_protos); ++i) {
>
> there are several other cases as well
> I not going to mention all
Thanks for the review. I make all the similar code changes in
ct-dpif.c, dpif-netlink.c and netlink-conntrack.c.
>> +static int
>> +dpif_netlink_ct_timeout_policy_dump_next(struct dpif *dpif OVS_UNUSED,
>> + void *state,
>> + struct ct_dpif_timeout_policy *tp)
>
> I think it would be super helpful to add some comments to this function.
>
Sure, I added some comments to dpif_netlink_ct_timeout_policy_dump_next() in v4.
>> +tp_dump_node = get_dpif_netlink_tp_dump_node_by_tp_id(
>> +tp_id, _state->tp_dump_map);
>> +if (!tp_dump_node) {
>> +tp_dump_node = xzalloc(sizeof *tp_dump_node);
>> +tp_dump_node->tp = xzalloc(sizeof *tp_dump_node->tp);
>> +tp_dump_node->tp->id = tp_id;
>> +hmap_insert(_state->tp_dump_map, _dump_node->hmap_node,
>> +hash_int(tp_id, 0));
>> +}
>> +
>> +update_dpif_netlink_tp_dump_node(_tp, tp_dump_node);
>> +if (tp_dump_node->l3_l4_present == DPIF_NL_ALL_TP) {
>
>>
>> +hmap_remove(_state->tp_dump_map, _dump_node->hmap_node);
>> +*tp = *tp_dump_node->tp;
>> +free(tp_dump_node->tp);
>> +free(tp_dump_node);
>
> common block; write once
Sure, I move the common block to a new function in v4.
static void
get_and_cleanup_tp_dump_node(struct hmap *hamp,
struct dpif_netlink_tp_dump_node *tp_dump_node,
struct ct_dpif_timeout_policy *tp)
{
hmap_remove(hmap, _dump_node->hmap_node);
*tp = *tp_dump_node->tp;
free(tp_dump_node->tp);
free(tp_dump_node);
}
Thanks,
-Yi-Hung
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 4/9] ct-dpif, dpif-netlink: Add conntrack timeout policy support
Thanks for the patch
mostly minor comments
On Mon, Aug 12, 2019 at 5:54 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
> ---
> 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 | 104 +
> lib/ct-dpif.h | 56 +++
> lib/dpif-netdev.c | 6 +
> lib/dpif-netlink.c | 469
> +
> lib/dpif-netlink.h | 1 -
> lib/dpif-provider.h| 44 ++
> lib/netlink-conntrack.c| 308 ++
> 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
> b/datapath-windows/include/OvsDpInterfaceCtExt.h
> index 3b947782e90c..4379855bb8dd 100644
> --- a/datapath-windows/include/OvsDpInterfaceCtExt.h
> +++ b/datapath-windows/include/OvsDpInterfaceCtExt.h
> @@ -421,4 +421,118 @@ struct nf_ct_tcp_flags {
> UINT8 mask;
> };
>
> +/* File: nfnetlink_cttimeout.h */
> +enum ctnl_timeout_msg_types {
> +IPCTNL_MSG_TIMEOUT_NEW,
> +IPCTNL_MSG_TIMEOUT_GET,
> +IPCTNL_MSG_TIMEOUT_DELETE,
> +IPCTNL_MSG_TIMEOUT_DEFAULT_SET,
> +IPCTNL_MSG_TIMEOUT_DEFAULT_GET,
> +
> +IPCTNL_MSG_TIMEOUT_MAX
> +};
> +
> +enum ctattr_timeout {
> +CTA_TIMEOUT_UNSPEC,
> +CTA_TIMEOUT_NAME,
> +CTA_TIMEOUT_L3PROTO,
> +CTA_TIMEOUT_L4PROTO,
> +CTA_TIMEOUT_DATA,
> +CTA_TIMEOUT_USE,
> +__CTA_TIMEOUT_MAX
> +};
> +#define CTA_TIMEOUT_MAX (__CTA_TIMEOUT_MAX - 1)
> +
> +enum ctattr_timeout_generic {
> +CTA_TIMEOUT_GENERIC_UNSPEC,
> +CTA_TIMEOUT_GENERIC_TIMEOUT,
> +__CTA_TIMEOUT_GENERIC_MAX
> +};
> +#define CTA_TIMEOUT_GENERIC_MAX (__CTA_TIMEOUT_GENERIC_MAX - 1)
> +
> +enum ctattr_timeout_tcp {
> +CTA_TIMEOUT_TCP_UNSPEC,
> +CTA_TIMEOUT_TCP_SYN_SENT,
> +CTA_TIMEOUT_TCP_SYN_RECV,
> +CTA_TIMEOUT_TCP_ESTABLISHED,
> +CTA_TIMEOUT_TCP_FIN_WAIT,
> +CTA_TIMEOUT_TCP_CLOSE_WAIT,
> +CTA_TIMEOUT_TCP_LAST_ACK,
> +CTA_TIMEOUT_TCP_TIME_WAIT,
> +CTA_TIMEOUT_TCP_CLOSE,
> +CTA_TIMEOUT_TCP_SYN_SENT2,
> +CTA_TIMEOUT_TCP_RETRANS,
> +CTA_TIMEOUT_TCP_UNACK,
> +__CTA_TIMEOUT_TCP_MAX
> +};
> +#define CTA_TIMEOUT_TCP_MAX (__CTA_TIMEOUT_TCP_MAX - 1)
> +
> +enum ctattr_timeout_udp {
> +
Re: [ovs-dev] [PATCH v3 4/9] ct-dpif, dpif-netlink: Add conntrack timeout policy support
On Tue, Aug 13, 2019 at 4:25 AM wrote:
> > ---
> > 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 | 104 +
> > lib/ct-dpif.h | 56 +++
> > lib/dpif-netdev.c | 6 +
> > lib/dpif-netlink.c | 469
> +
> > lib/dpif-netlink.h | 1 -
> > lib/dpif-provider.h| 44 ++
> > lib/netlink-conntrack.c| 308 ++
> > 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
> >
> [Alin] This is not an actual review.
>
> I'm okay with the Windows changes.
>
> I also tested the series and things look good.
>
> Do you mind folding in the following:
> diff --git a/datapath-windows/include/OvsDpInterfaceCtExt.h
> b/datapath-windows/include/OvsDpInterfaceCtExt.h
> index 4379855bb..3379f0a25 100644
> --- a/datapath-windows/include/OvsDpInterfaceCtExt.h
> +++ b/datapath-windows/include/OvsDpInterfaceCtExt.h
> @@ -421,7 +421,7 @@ struct nf_ct_tcp_flags {
> UINT8 mask;
> };
>
> -/* File: nfnetlink_cttimeout.h */
> +/* File: nfnetlink_cttimeout.h. XXX: the following are not implemented */
> enum ctnl_timeout_msg_types {
> IPCTNL_MSG_TIMEOUT_NEW,
> IPCTNL_MSG_TIMEOUT_GET,
>
>
> Acked-by: Alin Gabriel Serdean
>
Thanks Alin! Will fold in your diff into OvsDpInterfaceCtExt.h
Thanks,
-Yi-Hung
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 4/9] ct-dpif, dpif-netlink: Add conntrack timeout policy support
> -Original Message-
> From: ovs-dev-boun...@openvswitch.org boun...@openvswitch.org> On Behalf Of Yi-Hung Wei
> Sent: Tuesday, August 13, 2019 3:52 AM
> To: d...@openvswitch.org
> Subject: [ovs-dev] [PATCH v3 4/9] ct-dpif, dpif-netlink: Add conntrack
> timeout policy support
>
> 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
> ---
> 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 | 104 +
> lib/ct-dpif.h | 56 +++
> lib/dpif-netdev.c | 6 +
> lib/dpif-netlink.c | 469
+
> lib/dpif-netlink.h | 1 -
> lib/dpif-provider.h| 44 ++
> lib/netlink-conntrack.c| 308 ++
> 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
>
[Alin] This is not an actual review.
I'm okay with the Windows changes.
I also tested the series and things look good.
Do you mind folding in the following:
diff --git a/datapath-windows/include/OvsDpInterfaceCtExt.h
b/datapath-windows/include/OvsDpInterfaceCtExt.h
index 4379855bb..3379f0a25 100644
--- a/datapath-windows/include/OvsDpInterfaceCtExt.h
+++ b/datapath-windows/include/OvsDpInterfaceCtExt.h
@@ -421,7 +421,7 @@ struct nf_ct_tcp_flags {
UINT8 mask;
};
-/* File: nfnetlink_cttimeout.h */
+/* File: nfnetlink_cttimeout.h. XXX: the following are not implemented */
enum ctnl_timeout_msg_types {
IPCTNL_MSG_TIMEOUT_NEW,
IPCTNL_MSG_TIMEOUT_GET,
Acked-by: Alin Gabriel Serdean
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v3 4/9] ct-dpif, dpif-netlink: Add conntrack timeout policy support
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
---
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 | 104 +
lib/ct-dpif.h | 56 +++
lib/dpif-netdev.c | 6 +
lib/dpif-netlink.c | 469 +
lib/dpif-netlink.h | 1 -
lib/dpif-provider.h| 44 ++
lib/netlink-conntrack.c| 308 ++
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
b/datapath-windows/include/OvsDpInterfaceCtExt.h
index 3b947782e90c..4379855bb8dd 100644
--- a/datapath-windows/include/OvsDpInterfaceCtExt.h
+++ b/datapath-windows/include/OvsDpInterfaceCtExt.h
@@ -421,4 +421,118 @@ struct nf_ct_tcp_flags {
UINT8 mask;
};
+/* File: nfnetlink_cttimeout.h */
+enum ctnl_timeout_msg_types {
+IPCTNL_MSG_TIMEOUT_NEW,
+IPCTNL_MSG_TIMEOUT_GET,
+IPCTNL_MSG_TIMEOUT_DELETE,
+IPCTNL_MSG_TIMEOUT_DEFAULT_SET,
+IPCTNL_MSG_TIMEOUT_DEFAULT_GET,
+
+IPCTNL_MSG_TIMEOUT_MAX
+};
+
+enum ctattr_timeout {
+CTA_TIMEOUT_UNSPEC,
+CTA_TIMEOUT_NAME,
+CTA_TIMEOUT_L3PROTO,
+CTA_TIMEOUT_L4PROTO,
+CTA_TIMEOUT_DATA,
+CTA_TIMEOUT_USE,
+__CTA_TIMEOUT_MAX
+};
+#define CTA_TIMEOUT_MAX (__CTA_TIMEOUT_MAX - 1)
+
+enum ctattr_timeout_generic {
+CTA_TIMEOUT_GENERIC_UNSPEC,
+CTA_TIMEOUT_GENERIC_TIMEOUT,
+__CTA_TIMEOUT_GENERIC_MAX
+};
+#define CTA_TIMEOUT_GENERIC_MAX (__CTA_TIMEOUT_GENERIC_MAX - 1)
+
+enum ctattr_timeout_tcp {
+CTA_TIMEOUT_TCP_UNSPEC,
+CTA_TIMEOUT_TCP_SYN_SENT,
+CTA_TIMEOUT_TCP_SYN_RECV,
+CTA_TIMEOUT_TCP_ESTABLISHED,
+CTA_TIMEOUT_TCP_FIN_WAIT,
+CTA_TIMEOUT_TCP_CLOSE_WAIT,
+CTA_TIMEOUT_TCP_LAST_ACK,
+CTA_TIMEOUT_TCP_TIME_WAIT,
+CTA_TIMEOUT_TCP_CLOSE,
+CTA_TIMEOUT_TCP_SYN_SENT2,
+CTA_TIMEOUT_TCP_RETRANS,
+CTA_TIMEOUT_TCP_UNACK,
+__CTA_TIMEOUT_TCP_MAX
+};
+#define CTA_TIMEOUT_TCP_MAX (__CTA_TIMEOUT_TCP_MAX - 1)
+
+enum ctattr_timeout_udp {
+CTA_TIMEOUT_UDP_UNSPEC,
+CTA_TIMEOUT_UDP_UNREPLIED,
+CTA_TIMEOUT_UDP_REPLIED,
+__CTA_TIMEOUT_UDP_MAX
+};
+#define CTA_TIMEOUT_UDP_MAX (__CTA_TIMEOUT_UDP_MAX - 1)
+
+enum ctattr_timeout_udplite {
+CTA_TIMEOUT_UDPLITE_UNSPEC,
+CTA_TIMEOUT_UDPLITE_UNREPLIED,
+CTA_TIMEOUT_UDPLITE_REPLIED,
+
