Re: [ovs-dev] [PATCH v2 1/3] ct-dpif, dpif-netlink: Support conntrack flush by ct 5-tuple

2017-12-05 Thread Yi-Hung Wei 
>> On Nov 21, 2017, at 5:00 PM, Yi-Hung Wei  wrote:
>>
>> This patch adds support of flushing a conntrack entry specified by the
>> conntrack 5-tuple, and provides the implementation in dpif-netlink.
>> The implementation of dpif-netlink in the linux datapath utilizes the
>> NFNL_SUBSYS_CTNETLINK netlink subsystem to delete a conntrack entry in
>> nf_conntrack.
>
> It would be good to mention that this patch doesn't support the userspace or 
> Windows datapaths.  I'd like to add the following sentence to the commit 
> message:
>
> Future patches will add support for the userspace and Windows 
> datapaths.
Sure.

>
>> diff --git a/lib/netlink-conntrack.c b/lib/netlink-conntrack.c
>> index 1e1bb2f79d1d..1f0b9121036d 100644
>> --- a/lib/netlink-conntrack.c
>> +++ b/lib/netlink-conntrack.c
>>
>> ...
>> +int
>> +nl_ct_flush_tuple(const struct ct_dpif_tuple *tuple, uint16_t zone)
>> +{
>> +int err;
>> +struct ofpbuf buf;
>> +
>> +ofpbuf_init(, NL_DUMP_BUFSIZE);
>> +nl_msg_put_nfgenmsg(, 0, tuple->l3_type, NFNL_SUBSYS_CTNETLINK,
>> +IPCTNL_MSG_CT_DELETE, NLM_F_REQUEST);
>> +
>> +nl_msg_put_be16(, CTA_ZONE, htons(zone));
>
> When reviewing this patch, I noticed an issue with how Windows was handling 
> conntrack zones for flush.  I sent out a patch:
>
> 
> https://mail.openvswitch.org/pipermail/ovs-dev/2017-December/341610.html
>
> It's not a blocker for this patch, since this series doesn't add support for 
> flushing by 5-tuple on Windows.  However, I wanted to point out that it will 
> need to be fixed before that support is added.
>
> I thought a couple of the function descriptions could be clearer with a bit 
> more formatting and slight changes to the text.  I've appended those as a 
> patch to this message.
>
> If you agree with my suggestions, I'll commit this patch with those changes.
>
> Thanks,
>
> --Justin
Thanks for the review. Please fold in the changes, it makes the
function descriptions much clearer.

Thanks,

-Yi-Hung
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2 1/3] ct-dpif, dpif-netlink: Support conntrack flush by ct 5-tuple

2017-12-05 Thread Justin Pettit 


> On Nov 21, 2017, at 5:00 PM, Yi-Hung Wei  wrote:
> 
> This patch adds support of flushing a conntrack entry specified by the
> conntrack 5-tuple, and provides the implementation in dpif-netlink.
> The implementation of dpif-netlink in the linux datapath utilizes the
> NFNL_SUBSYS_CTNETLINK netlink subsystem to delete a conntrack entry in
> nf_conntrack.

It would be good to mention that this patch doesn't support the userspace or 
Windows datapaths.  I'd like to add the following sentence to the commit 
message:

Future patches will add support for the userspace and Windows datapaths.

> diff --git a/lib/netlink-conntrack.c b/lib/netlink-conntrack.c
> index 1e1bb2f79d1d..1f0b9121036d 100644
> --- a/lib/netlink-conntrack.c
> +++ b/lib/netlink-conntrack.c
> 
> ...
> +int
> +nl_ct_flush_tuple(const struct ct_dpif_tuple *tuple, uint16_t zone)
> +{
> +int err;
> +struct ofpbuf buf;
> +
> +ofpbuf_init(, NL_DUMP_BUFSIZE);
> +nl_msg_put_nfgenmsg(, 0, tuple->l3_type, NFNL_SUBSYS_CTNETLINK,
> +IPCTNL_MSG_CT_DELETE, NLM_F_REQUEST);
> +
> +nl_msg_put_be16(, CTA_ZONE, htons(zone));

When reviewing this patch, I noticed an issue with how Windows was handling 
conntrack zones for flush.  I sent out a patch:

https://mail.openvswitch.org/pipermail/ovs-dev/2017-December/341610.html

It's not a blocker for this patch, since this series doesn't add support for 
flushing by 5-tuple on Windows.  However, I wanted to point out that it will 
need to be fixed before that support is added.

I thought a couple of the function descriptions could be clearer with a bit 
more formatting and slight changes to the text.  I've appended those as a patch 
to this message.

If you agree with my suggestions, I'll commit this patch with those changes.

Thanks,

--Justin


-=-=-=-=-=-=-=-=-


diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
index 5cd6b5cfd870..cee4791565fb 100644
--- a/lib/ct-dpif.c
+++ b/lib/ct-dpif.c
@@ -110,13 +110,14 @@ ct_dpif_dump_done(struct ct_dpif_dump_state *dump)
 : EOPNOTSUPP);
 }
 ^L
-/* Flush the entries in the connection tracker used by 'dpif'.
- * If both 'zone' and 'tuple' are NULL, flush all the conntrack entries.
- * If 'zone' is not NULL, and 'tuple' is NULL, flush all the conntrack
- * entries in '*zone'.
- * If 'tuple' is not NULL, flush the conntrack entry specified by 'tuple'
- * in '*zone'. In this case, we use default zone (zone 0) if 'zone' is
- * NULL. */
+/* Flush the entries in the connection tracker used by 'dpif'.  The
+ * arguments have the following behavior:
+ *
+ *   - If both 'zone' and 'tuple' are NULL, flush all the conntrack entries.
+ *   - If 'zone' is not NULL, and 'tuple' is NULL, flush all the conntrack
+ * entries in '*zone'.
+ *   - If 'tuple' is not NULL, flush the conntrack entry specified by 'tuple'
+ * in '*zone'. If 'zone' is NULL, use the default zone (zone 0). */
 int
 ct_dpif_flush(struct dpif *dpif, const uint16_t *zone,
   const struct ct_dpif_tuple *tuple)
diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
index 33d7f2a64367..947bf5e31362 100644
--- a/lib/dpif-provider.h
+++ b/lib/dpif-provider.h
@@ -425,13 +425,16 @@ struct dpif_class {
 struct ct_dpif_entry *entry);
 int (*ct_dump_done)(struct dpif *, struct ct_dpif_dump_state *state);
 
-/* Flushes the connection tracking tables.
- * If both 'zone' and 'tuple' are NULL, flush all the conntrack entries.
- * If 'zone' is not NULL, and 'tuple' is NULL, flush all the conntrack
- * entries in '*zone'.
- * If 'tuple' is not NULL, flush the conntrack entry specified by 'tuple'
- * in '*zone'. In this case, we use default zone (zone 0) if 'zone' is
- * NULL. */
+/* Flushes the connection tracking tables.  The arguments have the
+ * following behavior:
+ *
+ *   - If both 'zone' and 'tuple' are NULL, flush all the conntrack
+ * entries.
+ *   - If 'zone' is not NULL, and 'tuple' is NULL, flush all the
+ * conntrack entries in '*zone'.
+ *   - If 'tuple' is not NULL, flush the conntrack entry specified by
+ * 'tuple' in '*zone'. If 'zone' is NULL, use the default zone
+ * (zone 0). */
 int (*ct_flush)(struct dpif *, const uint16_t *zone,
 const struct ct_dpif_tuple *tuple);
 


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v2 1/3] ct-dpif, dpif-netlink: Support conntrack flush by ct 5-tuple

2017-11-21 Thread Yi-Hung Wei 
This patch adds support of flushing a conntrack entry specified by the
conntrack 5-tuple, and provides the implementation in dpif-netlink.
The implementation of dpif-netlink in the linux datapath utilizes the
NFNL_SUBSYS_CTNETLINK netlink subsystem to delete a conntrack entry in
nf_conntrack.

VMWare-BZ: #1983178
Signed-off-by: Yi-Hung Wei 
---
 lib/ct-dpif.c   | 23 +---
 lib/ct-dpif.h   |  3 +-
 lib/dpctl.c |  2 +-
 lib/dpif-netdev.c   |  6 ++-
 lib/dpif-netlink.c  |  7 +++-
 lib/dpif-provider.h | 13 +--
 lib/netlink-conntrack.c | 97 +
 lib/netlink-conntrack.h |  1 +
 ofproto/ofproto-dpif.c  |  2 +-
 tests/ovs-ofctl.at  |  2 +-
 10 files changed, 140 insertions(+), 16 deletions(-)

diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
index c79e69e23970..5cd6b5cfd870 100644
--- a/lib/ct-dpif.c
+++ b/lib/ct-dpif.c
@@ -111,19 +111,30 @@ ct_dpif_dump_done(struct ct_dpif_dump_state *dump)
 }
 
 /* Flush the entries in the connection tracker used by 'dpif'.
- *
- * If 'zone' is not NULL, flush only the entries in '*zone'. */
+ * If both 'zone' and 'tuple' are NULL, flush all the conntrack entries.
+ * If 'zone' is not NULL, and 'tuple' is NULL, flush all the conntrack
+ * entries in '*zone'.
+ * If 'tuple' is not NULL, flush the conntrack entry specified by 'tuple'
+ * in '*zone'. In this case, we use default zone (zone 0) if 'zone' is
+ * NULL. */
 int
-ct_dpif_flush(struct dpif *dpif, const uint16_t *zone)
+ct_dpif_flush(struct dpif *dpif, const uint16_t *zone,
+  const struct ct_dpif_tuple *tuple)
 {
-if (zone) {
-VLOG_DBG("%s: ct_flush: %"PRIu16, dpif_name(dpif), *zone);
+if (tuple) {
+struct ds ds = DS_EMPTY_INITIALIZER;
+ct_dpif_format_tuple(, tuple);
+VLOG_DBG("%s: ct_flush: %s in zone %d", dpif_name(dpif), ds_cstr(),
+zone ? *zone : 0);
+ds_destroy();
+} else if (zone) {
+VLOG_DBG("%s: ct_flush: zone %"PRIu16, dpif_name(dpif), *zone);
 } else {
 VLOG_DBG("%s: ct_flush: ", dpif_name(dpif));
 }
 
 return (dpif->dpif_class->ct_flush
-? dpif->dpif_class->ct_flush(dpif, zone)
+? dpif->dpif_class->ct_flush(dpif, zone, tuple)
 : EOPNOTSUPP);
 }
 
diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
index d5f966175bc0..ef019050c78e 100644
--- a/lib/ct-dpif.h
+++ b/lib/ct-dpif.h
@@ -195,7 +195,8 @@ int ct_dpif_dump_start(struct dpif *, struct 
ct_dpif_dump_state **,
const uint16_t *zone, int *);
 int ct_dpif_dump_next(struct ct_dpif_dump_state *, struct ct_dpif_entry *);
 int ct_dpif_dump_done(struct ct_dpif_dump_state *);
-int ct_dpif_flush(struct dpif *, const uint16_t *zone);
+int ct_dpif_flush(struct dpif *, const uint16_t *zone,
+  const struct ct_dpif_tuple *);
 void ct_dpif_entry_uninit(struct ct_dpif_entry *);
 void ct_dpif_format_entry(const struct ct_dpif_entry *, struct ds *,
   bool verbose, bool print_stats);
diff --git a/lib/dpctl.c b/lib/dpctl.c
index 6520788aa428..7fc0e3afab37 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -1353,7 +1353,7 @@ dpctl_flush_conntrack(int argc, const char *argv[],
 return error;
 }
 
-error = ct_dpif_flush(dpif, pzone);
+error = ct_dpif_flush(dpif, pzone, NULL);
 
 dpif_close(dpif);
 return error;
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 0a62630c2712..b1ef9a6a5992 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -5734,10 +5734,14 @@ dpif_netdev_ct_dump_done(struct dpif *dpif OVS_UNUSED,
 }
 
 static int
-dpif_netdev_ct_flush(struct dpif *dpif, const uint16_t *zone)
+dpif_netdev_ct_flush(struct dpif *dpif, const uint16_t *zone,
+ const struct ct_dpif_tuple *tuple)
 {
 struct dp_netdev *dp = get_dp_netdev(dpif);
 
+if (tuple) {
+return EOPNOTSUPP;
+}
 return conntrack_flush(>conntrack, zone);
 }
 
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index c265909f8587..3f76128999d1 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -2892,9 +2892,12 @@ dpif_netlink_ct_dump_done(struct dpif *dpif OVS_UNUSED,
 }
 
 static int
-dpif_netlink_ct_flush(struct dpif *dpif OVS_UNUSED, const uint16_t *zone)
+dpif_netlink_ct_flush(struct dpif *dpif OVS_UNUSED, const uint16_t *zone,
+  const struct ct_dpif_tuple *tuple)
 {
-if (zone) {
+if (tuple) {
+return nl_ct_flush_tuple(tuple, zone ? *zone : 0);
+} else if (zone) {
 return nl_ct_flush_zone(*zone);
 } else {
 return nl_ct_flush();
diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
index 1d82a0939aa1..33d7f2a64367 100644
--- a/lib/dpif-provider.h
+++ b/lib/dpif-provider.h
@@ -75,6 +75,7 @@ dpif_flow_dump_thread_init(struct dpif_flow_dump_thread 
*thread,
 
 struct ct_dpif_dump_state;
 struct ct_dpif_entry;
+struct