Re: [ovs-dev] [PATCH v2 09/15] dpif-netdev: Execute conntrack action.
On 26/04/2016 16:37, "Flavio Leitner" wrote:
>On Fri, Apr 15, 2016 at 05:02:41PM -0700, Daniele Di Proietto wrote:
>> This commit implements the OVS_ACTION_ATTR_CT action in dpif-netdev.
>>
>> To allow ofproto-dpif to detect the conntrack feature, flow_put will not
>> discard anymore flows with ct_* fields set. We still shouldn't allow
>> flows with NAT bits set, since there is no support for NAT.
>>
>> Signed-off-by: Daniele Di Proietto
>> ---
>> lib/dpif-netdev.c | 68
>> +--
>> tests/dpif-netdev.at | 14 +--
>> tests/ofproto-dpif.at | 20 +++
>> 3 files changed, 77 insertions(+), 25 deletions(-)
>>
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>> index 1e8a37c..436359a 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>> @@ -33,6 +33,7 @@
>>
>> #include "bitmap.h"
>> #include "cmap.h"
>> +#include "conntrack.h"
>> #include "coverage.h"
>> #include "csum.h"
>> #include "dp-packet.h"
>> @@ -89,9 +90,17 @@ static struct shash dp_netdevs
>> OVS_GUARDED_BY(dp_netdev_mutex)
>>
>> static struct vlog_rate_limit upcall_rl = VLOG_RATE_LIMIT_INIT(600, 600);
>>
>> +#define DP_NETDEV_CS_SUPPORTED_MASK (CS_NEW | CS_ESTABLISHED | CS_RELATED \
>> + | CS_INVALID | CS_REPLY_DIR |
>> CS_TRACKED)
>> +#define DP_NETDEV_CS_UNSUPPORTED_MASK
>> (~(uint32_t)DP_NETDEV_CS_SUPPORTED_MASK)
>> +
>> static struct odp_support dp_netdev_support = {
>> .max_mpls_depth = SIZE_MAX,
>> .recirc = true,
>> +.ct_state = true,
>> +.ct_zone = true,
>> +.ct_mark = true,
>> +.ct_label = true,
>> };
>>
>> /* Stores a miniflow with inline values */
>> @@ -224,6 +233,8 @@ struct dp_netdev {
>> /* Cpu mask for pin of pmd threads. */
>> char *pmd_cmask;
>> uint64_t last_tnl_conf_seq;
>> +
>> +struct conntrack conntrack;
>> };
>>
>> static struct dp_netdev_port *dp_netdev_lookup_port(const struct dp_netdev
>> *dp,
>> @@ -906,6 +917,8 @@ create_dp_netdev(const char *name, const struct
>> dpif_class *class,
>> dp->upcall_aux = NULL;
>> dp->upcall_cb = NULL;
>>
>> +conntrack_init(&dp->conntrack);
>> +
>> cmap_init(&dp->poll_threads);
>> ovs_mutex_init_recursive(&dp->non_pmd_mutex);
>> ovsthread_key_create(&dp->per_pmd_key, NULL);
>> @@ -976,6 +989,8 @@ dp_netdev_free(struct dp_netdev *dp)
>> ovs_mutex_destroy(&dp->non_pmd_mutex);
>> ovsthread_key_delete(dp->per_pmd_key);
>>
>> +conntrack_destroy(&dp->conntrack);
>> +
>> ovs_mutex_lock(&dp->port_mutex);
>> CMAP_FOR_EACH (port, node, &dp->ports) {
>> /* PMD threads are destroyed here. do_del_port() cannot quiesce */
>> @@ -1965,9 +1980,7 @@ dpif_netdev_flow_from_nlattrs(const struct nlattr
>> *key, uint32_t key_len,
>> return EINVAL;
>> }
>>
>> -/* Userspace datapath doesn't support conntrack. */
>> -if (flow->ct_state || flow->ct_zone || flow->ct_mark
>> -|| !ovs_u128_is_zero(&flow->ct_label)) {
>> +if (flow->ct_state & DP_NETDEV_CS_UNSUPPORTED_MASK) {
>> return EINVAL;
>> }
>>
>> @@ -2604,6 +2617,9 @@ dpif_netdev_run(struct dpif *dpif)
>> ovs_mutex_unlock(&dp->non_pmd_mutex);
>> dp_netdev_pmd_unref(non_pmd);
>>
>> +/* XXX: If workload is too heavy we could add a separate thread. */
>> +conntrack_run(&dp->conntrack);
>> +
>
>Do you have any results to share? Just curious.
>
>Acked-by: Flavio Leitner
As also pointed out by Joe this probably need to be revisited.
I'll do more testing. Thanks!
>
>> tnl_neigh_cache_run();
>> tnl_port_map_run();
>> new_tnl_seq = seq_read(tnl_conf_seq);
>> @@ -3850,12 +3866,48 @@ dp_execute_cb(void *aux_, struct dp_packet
>> **packets, int cnt,
>> VLOG_WARN("Packet dropped. Max recirculation depth exceeded.");
>> break;
>>
>> -case OVS_ACTION_ATTR_CT:
>> -/* If a flow with this action is slow-pathed, datapath assistance is
>> - * required to implement it. However, we don't support this action
>> - * in the userspace datapath. */
>> -VLOG_WARN("Cannot execute conntrack action in userspace.");
>> +case OVS_ACTION_ATTR_CT: {
>> +const struct nlattr *b;
>> +bool commit = false;
>> +unsigned int left;
>> +uint16_t zone = 0;
>> +const char *helper = NULL;
>> +const uint32_t *setmark = NULL;
>> +const struct ovs_key_ct_labels *setlabel = NULL;
>> +
>> +
>> +/* XXX parsing this everytime is expensive. We should do like
>> kernel
>> + * does and create a structure. */
>> +NL_ATTR_FOR_EACH_UNSAFE (b, left, nl_attr_get(a),
>> nl_attr_get_size(a)) {
>> +enum ovs_ct_attr sub_type = nl_attr_type(b);
>> +
>> +switch(sub_type) {
>> +case OVS_CT_ATTR_COMMIT:
>> +commit = true;
>> +break;
>> +case OVS_CT_ATTR_ZONE
Re: [ovs-dev] [PATCH v2 09/15] dpif-netdev: Execute conntrack action.
On 19/04/2016 14:52, "Joe Stringer" wrote:
>On 15 April 2016 at 17:02, Daniele Di Proietto wrote:
>> This commit implements the OVS_ACTION_ATTR_CT action in dpif-netdev.
>>
>> To allow ofproto-dpif to detect the conntrack feature, flow_put will not
>> discard anymore flows with ct_* fields set. We still shouldn't allow
>> flows with NAT bits set, since there is no support for NAT.
>>
>> Signed-off-by: Daniele Di Proietto
>> ---
>
>...
>
>> @@ -2604,6 +2617,9 @@ dpif_netdev_run(struct dpif *dpif)
>> ovs_mutex_unlock(&dp->non_pmd_mutex);
>> dp_netdev_pmd_unref(non_pmd);
>>
>> +/* XXX: If workload is too heavy we could add a separate thread. */
>> +conntrack_run(&dp->conntrack);
>> +
>
>Have you tried, eg, portscanning with several threads against the
>implementation to see how bad it gets?
Good point, I'm doing some testing and I'll revisit this.
>
>
>> @@ -3850,12 +3866,48 @@ dp_execute_cb(void *aux_, struct dp_packet
>> **packets, int cnt,
>> VLOG_WARN("Packet dropped. Max recirculation depth exceeded.");
>> break;
>>
>> -case OVS_ACTION_ATTR_CT:
>> -/* If a flow with this action is slow-pathed, datapath assistance is
>> - * required to implement it. However, we don't support this action
>> - * in the userspace datapath. */
>> -VLOG_WARN("Cannot execute conntrack action in userspace.");
>> +case OVS_ACTION_ATTR_CT: {
>> +const struct nlattr *b;
>> +bool commit = false;
>> +unsigned int left;
>> +uint16_t zone = 0;
>> +const char *helper = NULL;
>> +const uint32_t *setmark = NULL;
>> +const struct ovs_key_ct_labels *setlabel = NULL;
>> +
>> +
>> +/* XXX parsing this everytime is expensive. We should do like
>> kernel
>> + * does and create a structure. */
>
>Seems reasonable. You say it's expensive (how expensive?), but it also
>seems a little cleaner to store it in a more accessible manner.
I tried creating a structure, but it doesn't seem to make a difference in
performance. It is a little cleaner, but the I guess the complexity of
maintaining a (not so different) action format doesn't seem worth for now.
I think I'll remove this comment.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2 09/15] dpif-netdev: Execute conntrack action.
On Fri, Apr 15, 2016 at 05:02:41PM -0700, Daniele Di Proietto wrote:
> This commit implements the OVS_ACTION_ATTR_CT action in dpif-netdev.
>
> To allow ofproto-dpif to detect the conntrack feature, flow_put will not
> discard anymore flows with ct_* fields set. We still shouldn't allow
> flows with NAT bits set, since there is no support for NAT.
>
> Signed-off-by: Daniele Di Proietto
> ---
> lib/dpif-netdev.c | 68
> +--
> tests/dpif-netdev.at | 14 +--
> tests/ofproto-dpif.at | 20 +++
> 3 files changed, 77 insertions(+), 25 deletions(-)
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 1e8a37c..436359a 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -33,6 +33,7 @@
>
> #include "bitmap.h"
> #include "cmap.h"
> +#include "conntrack.h"
> #include "coverage.h"
> #include "csum.h"
> #include "dp-packet.h"
> @@ -89,9 +90,17 @@ static struct shash dp_netdevs
> OVS_GUARDED_BY(dp_netdev_mutex)
>
> static struct vlog_rate_limit upcall_rl = VLOG_RATE_LIMIT_INIT(600, 600);
>
> +#define DP_NETDEV_CS_SUPPORTED_MASK (CS_NEW | CS_ESTABLISHED | CS_RELATED \
> + | CS_INVALID | CS_REPLY_DIR |
> CS_TRACKED)
> +#define DP_NETDEV_CS_UNSUPPORTED_MASK
> (~(uint32_t)DP_NETDEV_CS_SUPPORTED_MASK)
> +
> static struct odp_support dp_netdev_support = {
> .max_mpls_depth = SIZE_MAX,
> .recirc = true,
> +.ct_state = true,
> +.ct_zone = true,
> +.ct_mark = true,
> +.ct_label = true,
> };
>
> /* Stores a miniflow with inline values */
> @@ -224,6 +233,8 @@ struct dp_netdev {
> /* Cpu mask for pin of pmd threads. */
> char *pmd_cmask;
> uint64_t last_tnl_conf_seq;
> +
> +struct conntrack conntrack;
> };
>
> static struct dp_netdev_port *dp_netdev_lookup_port(const struct dp_netdev
> *dp,
> @@ -906,6 +917,8 @@ create_dp_netdev(const char *name, const struct
> dpif_class *class,
> dp->upcall_aux = NULL;
> dp->upcall_cb = NULL;
>
> +conntrack_init(&dp->conntrack);
> +
> cmap_init(&dp->poll_threads);
> ovs_mutex_init_recursive(&dp->non_pmd_mutex);
> ovsthread_key_create(&dp->per_pmd_key, NULL);
> @@ -976,6 +989,8 @@ dp_netdev_free(struct dp_netdev *dp)
> ovs_mutex_destroy(&dp->non_pmd_mutex);
> ovsthread_key_delete(dp->per_pmd_key);
>
> +conntrack_destroy(&dp->conntrack);
> +
> ovs_mutex_lock(&dp->port_mutex);
> CMAP_FOR_EACH (port, node, &dp->ports) {
> /* PMD threads are destroyed here. do_del_port() cannot quiesce */
> @@ -1965,9 +1980,7 @@ dpif_netdev_flow_from_nlattrs(const struct nlattr *key,
> uint32_t key_len,
> return EINVAL;
> }
>
> -/* Userspace datapath doesn't support conntrack. */
> -if (flow->ct_state || flow->ct_zone || flow->ct_mark
> -|| !ovs_u128_is_zero(&flow->ct_label)) {
> +if (flow->ct_state & DP_NETDEV_CS_UNSUPPORTED_MASK) {
> return EINVAL;
> }
>
> @@ -2604,6 +2617,9 @@ dpif_netdev_run(struct dpif *dpif)
> ovs_mutex_unlock(&dp->non_pmd_mutex);
> dp_netdev_pmd_unref(non_pmd);
>
> +/* XXX: If workload is too heavy we could add a separate thread. */
> +conntrack_run(&dp->conntrack);
> +
Do you have any results to share? Just curious.
Acked-by: Flavio Leitner
> tnl_neigh_cache_run();
> tnl_port_map_run();
> new_tnl_seq = seq_read(tnl_conf_seq);
> @@ -3850,12 +3866,48 @@ dp_execute_cb(void *aux_, struct dp_packet **packets,
> int cnt,
> VLOG_WARN("Packet dropped. Max recirculation depth exceeded.");
> break;
>
> -case OVS_ACTION_ATTR_CT:
> -/* If a flow with this action is slow-pathed, datapath assistance is
> - * required to implement it. However, we don't support this action
> - * in the userspace datapath. */
> -VLOG_WARN("Cannot execute conntrack action in userspace.");
> +case OVS_ACTION_ATTR_CT: {
> +const struct nlattr *b;
> +bool commit = false;
> +unsigned int left;
> +uint16_t zone = 0;
> +const char *helper = NULL;
> +const uint32_t *setmark = NULL;
> +const struct ovs_key_ct_labels *setlabel = NULL;
> +
> +
> +/* XXX parsing this everytime is expensive. We should do like kernel
> + * does and create a structure. */
> +NL_ATTR_FOR_EACH_UNSAFE (b, left, nl_attr_get(a),
> nl_attr_get_size(a)) {
> +enum ovs_ct_attr sub_type = nl_attr_type(b);
> +
> +switch(sub_type) {
> +case OVS_CT_ATTR_COMMIT:
> +commit = true;
> +break;
> +case OVS_CT_ATTR_ZONE:
> +zone = nl_attr_get_u16(b);
> +break;
> +case OVS_CT_ATTR_HELPER:
> +helper = nl_attr_get_string(b);
> +break;
> +case OVS_CT_ATTR_MARK:
> +setmark = nl_attr_get(b);
> +
Re: [ovs-dev] [PATCH v2 09/15] dpif-netdev: Execute conntrack action.
On 15 April 2016 at 17:02, Daniele Di Proietto wrote:
> This commit implements the OVS_ACTION_ATTR_CT action in dpif-netdev.
>
> To allow ofproto-dpif to detect the conntrack feature, flow_put will not
> discard anymore flows with ct_* fields set. We still shouldn't allow
> flows with NAT bits set, since there is no support for NAT.
>
> Signed-off-by: Daniele Di Proietto
> ---
...
> @@ -2604,6 +2617,9 @@ dpif_netdev_run(struct dpif *dpif)
> ovs_mutex_unlock(&dp->non_pmd_mutex);
> dp_netdev_pmd_unref(non_pmd);
>
> +/* XXX: If workload is too heavy we could add a separate thread. */
> +conntrack_run(&dp->conntrack);
> +
Have you tried, eg, portscanning with several threads against the
implementation to see how bad it gets?
> @@ -3850,12 +3866,48 @@ dp_execute_cb(void *aux_, struct dp_packet **packets,
> int cnt,
> VLOG_WARN("Packet dropped. Max recirculation depth exceeded.");
> break;
>
> -case OVS_ACTION_ATTR_CT:
> -/* If a flow with this action is slow-pathed, datapath assistance is
> - * required to implement it. However, we don't support this action
> - * in the userspace datapath. */
> -VLOG_WARN("Cannot execute conntrack action in userspace.");
> +case OVS_ACTION_ATTR_CT: {
> +const struct nlattr *b;
> +bool commit = false;
> +unsigned int left;
> +uint16_t zone = 0;
> +const char *helper = NULL;
> +const uint32_t *setmark = NULL;
> +const struct ovs_key_ct_labels *setlabel = NULL;
> +
> +
> +/* XXX parsing this everytime is expensive. We should do like kernel
> + * does and create a structure. */
Seems reasonable. You say it's expensive (how expensive?), but it also
seems a little cleaner to store it in a more accessible manner.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v2 09/15] dpif-netdev: Execute conntrack action.
This commit implements the OVS_ACTION_ATTR_CT action in dpif-netdev.
To allow ofproto-dpif to detect the conntrack feature, flow_put will not
discard anymore flows with ct_* fields set. We still shouldn't allow
flows with NAT bits set, since there is no support for NAT.
Signed-off-by: Daniele Di Proietto
---
lib/dpif-netdev.c | 68 +--
tests/dpif-netdev.at | 14 +--
tests/ofproto-dpif.at | 20 +++
3 files changed, 77 insertions(+), 25 deletions(-)
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 1e8a37c..436359a 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -33,6 +33,7 @@
#include "bitmap.h"
#include "cmap.h"
+#include "conntrack.h"
#include "coverage.h"
#include "csum.h"
#include "dp-packet.h"
@@ -89,9 +90,17 @@ static struct shash dp_netdevs
OVS_GUARDED_BY(dp_netdev_mutex)
static struct vlog_rate_limit upcall_rl = VLOG_RATE_LIMIT_INIT(600, 600);
+#define DP_NETDEV_CS_SUPPORTED_MASK (CS_NEW | CS_ESTABLISHED | CS_RELATED \
+ | CS_INVALID | CS_REPLY_DIR | CS_TRACKED)
+#define DP_NETDEV_CS_UNSUPPORTED_MASK (~(uint32_t)DP_NETDEV_CS_SUPPORTED_MASK)
+
static struct odp_support dp_netdev_support = {
.max_mpls_depth = SIZE_MAX,
.recirc = true,
+.ct_state = true,
+.ct_zone = true,
+.ct_mark = true,
+.ct_label = true,
};
/* Stores a miniflow with inline values */
@@ -224,6 +233,8 @@ struct dp_netdev {
/* Cpu mask for pin of pmd threads. */
char *pmd_cmask;
uint64_t last_tnl_conf_seq;
+
+struct conntrack conntrack;
};
static struct dp_netdev_port *dp_netdev_lookup_port(const struct dp_netdev *dp,
@@ -906,6 +917,8 @@ create_dp_netdev(const char *name, const struct dpif_class
*class,
dp->upcall_aux = NULL;
dp->upcall_cb = NULL;
+conntrack_init(&dp->conntrack);
+
cmap_init(&dp->poll_threads);
ovs_mutex_init_recursive(&dp->non_pmd_mutex);
ovsthread_key_create(&dp->per_pmd_key, NULL);
@@ -976,6 +989,8 @@ dp_netdev_free(struct dp_netdev *dp)
ovs_mutex_destroy(&dp->non_pmd_mutex);
ovsthread_key_delete(dp->per_pmd_key);
+conntrack_destroy(&dp->conntrack);
+
ovs_mutex_lock(&dp->port_mutex);
CMAP_FOR_EACH (port, node, &dp->ports) {
/* PMD threads are destroyed here. do_del_port() cannot quiesce */
@@ -1965,9 +1980,7 @@ dpif_netdev_flow_from_nlattrs(const struct nlattr *key,
uint32_t key_len,
return EINVAL;
}
-/* Userspace datapath doesn't support conntrack. */
-if (flow->ct_state || flow->ct_zone || flow->ct_mark
-|| !ovs_u128_is_zero(&flow->ct_label)) {
+if (flow->ct_state & DP_NETDEV_CS_UNSUPPORTED_MASK) {
return EINVAL;
}
@@ -2604,6 +2617,9 @@ dpif_netdev_run(struct dpif *dpif)
ovs_mutex_unlock(&dp->non_pmd_mutex);
dp_netdev_pmd_unref(non_pmd);
+/* XXX: If workload is too heavy we could add a separate thread. */
+conntrack_run(&dp->conntrack);
+
tnl_neigh_cache_run();
tnl_port_map_run();
new_tnl_seq = seq_read(tnl_conf_seq);
@@ -3850,12 +3866,48 @@ dp_execute_cb(void *aux_, struct dp_packet **packets,
int cnt,
VLOG_WARN("Packet dropped. Max recirculation depth exceeded.");
break;
-case OVS_ACTION_ATTR_CT:
-/* If a flow with this action is slow-pathed, datapath assistance is
- * required to implement it. However, we don't support this action
- * in the userspace datapath. */
-VLOG_WARN("Cannot execute conntrack action in userspace.");
+case OVS_ACTION_ATTR_CT: {
+const struct nlattr *b;
+bool commit = false;
+unsigned int left;
+uint16_t zone = 0;
+const char *helper = NULL;
+const uint32_t *setmark = NULL;
+const struct ovs_key_ct_labels *setlabel = NULL;
+
+
+/* XXX parsing this everytime is expensive. We should do like kernel
+ * does and create a structure. */
+NL_ATTR_FOR_EACH_UNSAFE (b, left, nl_attr_get(a), nl_attr_get_size(a))
{
+enum ovs_ct_attr sub_type = nl_attr_type(b);
+
+switch(sub_type) {
+case OVS_CT_ATTR_COMMIT:
+commit = true;
+break;
+case OVS_CT_ATTR_ZONE:
+zone = nl_attr_get_u16(b);
+break;
+case OVS_CT_ATTR_HELPER:
+helper = nl_attr_get_string(b);
+break;
+case OVS_CT_ATTR_MARK:
+setmark = nl_attr_get(b);
+break;
+case OVS_CT_ATTR_LABELS:
+setlabel = nl_attr_get(b);
+break;
+case OVS_CT_ATTR_NAT:
+case OVS_CT_ATTR_UNSPEC:
+case __OVS_CT_ATTR_MAX:
+OVS_NOT_REACHED();
+}
+}
+
+conntrack_execute(&dp->conntrack, packets, cnt, commit, zone,
+ setmark, setlabel, helper);
