[ovs-dev] [PATCH v9 3/3] conntrack: limit port clash resolution attempts
From: wenxu
In case almost or all available ports are taken, clash resolution can
take a very long time, resulting in pmd hang in conntrack.
This can happen when many to-be-natted hosts connect to same
destination:port (e.g. a proxy) and all connections pass the same SNAT.
Pick a random offset in the acceptable range, then try ever smaller
number of adjacent port numbers, until either the limit is reached or a
useable port was found. This results in at most 248 attempts
(128 + 64 + 32 + 16 + 8, i.e. 4 restarts with new search offset)
instead of 64000+.
And if thenumber of ip address will limit the max attempts and which
will lead the total attempts under 248.
Signed-off-by: wenxu
---
lib/conntrack.c | 60 ++---
1 file changed, 53 insertions(+), 7 deletions(-)
diff --git a/lib/conntrack.c b/lib/conntrack.c
index dc29c9b..4079a35 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -2400,13 +2400,18 @@ next_addr_in_range_guarded(union ct_addr *curr, union
ct_addr *min,
static bool
nat_get_unique_l4(struct conntrack *ct, struct conn *nat_conn,
ovs_be16 *port, uint16_t curr, uint16_t min,
- uint16_t max)
+ uint16_t max, unsigned int *round,
+ unsigned int attempts)
{
FOR_EACH_PORT_IN_RANGE(curr, min, max) {
-*port = htons(curr);
-if (!conn_lookup(ct, _conn->rev_key,
- time_msec(), NULL, NULL)) {
-return true;
+if ((*round)++ < attempts) {
+*port = htons(curr);
+if (!conn_lookup(ct, _conn->rev_key,
+ time_msec(), NULL, NULL)) {
+return true;
+}
+} else {
+break;
}
}
@@ -2446,6 +2451,10 @@ nat_get_unique_tuple(struct conntrack *ct, const struct
conn *conn,
uint16_t min_dport, max_dport, curr_dport, orig_dport;
bool pat_proto = conn->key.nw_proto == IPPROTO_TCP ||
conn->key.nw_proto == IPPROTO_UDP;
+unsigned int attempts, max_attempts, min_attempts;
+uint16_t range_src, range_dst, range_max;
+uint32_t range_addr;
+unsigned int i;
min_addr = nat_info->min_addr;
max_addr = nat_info->max_addr;
@@ -2462,6 +2471,24 @@ nat_get_unique_tuple(struct conntrack *ct, const struct
conn *conn,
set_dport_range(nat_info, >key, hash, _dport,
_dport, _dport);
+range_src = max_sport - min_sport + 1;
+range_dst = max_dport - min_dport + 1;
+range_max = range_src > range_dst ? range_src : range_dst;
+if (conn->key.dl_type == htons(ETH_TYPE_IP)) {
+range_addr = ntohl(max_addr.ipv4) - ntohl(min_addr.ipv4) + 1;
+} else {
+range_addr = nat_ipv6_addrs_delta(_info->min_addr.ipv6,
+ _info->max_addr.ipv6) + 1;
+}
+max_attempts = 128 / range_addr;
+if (max_attempts < 1) {
+max_attempts = 1;
+}
+min_attempts = 16 / range_addr;
+if (min_attempts < 2) {
+min_attempts = 2;
+}
+
another_round:
store_addr_to_key(_addr, _conn->rev_key,
nat_info->nat_action);
@@ -2481,24 +2508,43 @@ another_round:
nat_conn->rev_key.src.port = htons(curr_dport);
nat_conn->rev_key.dst.port = htons(curr_sport);
+attempts = range_max;
+if (attempts > max_attempts) {
+attempts = max_attempts;
+}
+
+another_port_round:
+i = 0;
+
bool found = false;
if (nat_info->nat_action & NAT_ACTION_DST_PORT) {
found = nat_get_unique_l4(ct, nat_conn, _conn->rev_key.src.port,
- curr_dport, min_dport, max_dport);
+ curr_dport, min_dport, max_dport, ,
+ attempts);
if (!found) {
+i = 0;
nat_conn->rev_key.src.port = htons(orig_dport);
}
}
if (!found) {
found = nat_get_unique_l4(ct, nat_conn, _conn->rev_key.dst.port,
- curr_sport, min_sport, max_sport);
+ curr_sport, min_sport, max_sport, ,
+ attempts);
}
if (found) {
return true;
}
+if (attempts < range_max && attempts >= min_attempts) {
+attempts /= 2;
+curr_dport = min_dport + (random_uint32() % range_dst);
+curr_sport = min_sport + (random_uint32() % range_src);
+
+goto another_port_round;
+}
+
/* Check if next IP is in range and respin. Otherwise, notify
* exhaustion to the caller. */
next_addr:
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v9 2/3] conntrack: prefer dst port range during unique tuple search
From: wenxu
This commit splits the nested loop used to search the unique ports for
the reverse tuple.
It affects only the dnat action, giving more precedence to the dnat
range, similarly to the kernel dp, instead of searching through the
default ephemeral source range for each destination port.
Signed-off-by: wenxu
---
lib/conntrack.c | 61 +++--
1 file changed, 46 insertions(+), 15 deletions(-)
diff --git a/lib/conntrack.c b/lib/conntrack.c
index 44f99f3..dc29c9b 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -2397,6 +2397,22 @@ next_addr_in_range_guarded(union ct_addr *curr, union
ct_addr *min,
return exhausted;
}
+static bool
+nat_get_unique_l4(struct conntrack *ct, struct conn *nat_conn,
+ ovs_be16 *port, uint16_t curr, uint16_t min,
+ uint16_t max)
+{
+FOR_EACH_PORT_IN_RANGE(curr, min, max) {
+*port = htons(curr);
+if (!conn_lookup(ct, _conn->rev_key,
+ time_msec(), NULL, NULL)) {
+return true;
+}
+}
+
+return false;
+}
+
/* This function tries to get a unique tuple.
* Every iteration checks that the reverse tuple doesn't
* collide with any existing one.
@@ -2411,9 +2427,11 @@ next_addr_in_range_guarded(union ct_addr *curr, union
ct_addr *min,
*
* In case of DNAT:
*- For each dst IP address in the range (if any).
- *- For each dport in range (if any).
- * - Try to find a source port in the ephemeral range
- * (after testing the port used by the sender).
+ *- For each dport in range (if any) tries to find
+ * an unique tuple.
+ *- Eventually, if the previous attempt fails,
+ * tries to find a source port in the ephemeral
+ * range (after testing the port used by the sender).
*
* If none can be found, return exhaustion to the caller. */
static bool
@@ -2424,10 +2442,10 @@ nat_get_unique_tuple(struct conntrack *ct, const struct
conn *conn,
union ct_addr min_addr = {0}, max_addr = {0}, curr_addr = {0},
guard_addr = {0};
uint32_t hash = nat_range_hash(conn, ct->hash_basis, nat_info);
+uint16_t min_sport, max_sport, curr_sport, orig_sport;
+uint16_t min_dport, max_dport, curr_dport, orig_dport;
bool pat_proto = conn->key.nw_proto == IPPROTO_TCP ||
conn->key.nw_proto == IPPROTO_UDP;
-uint16_t min_dport, max_dport, curr_dport;
-uint16_t min_sport, max_sport, curr_sport;
min_addr = nat_info->min_addr;
max_addr = nat_info->max_addr;
@@ -2439,9 +2457,9 @@ nat_get_unique_tuple(struct conntrack *ct, const struct
conn *conn,
* we can stop once we reach it. */
guard_addr = curr_addr;
-set_sport_range(nat_info, >key, hash, _sport,
+set_sport_range(nat_info, >key, hash, _sport,
_sport, _sport);
-set_dport_range(nat_info, >key, hash, _dport,
+set_dport_range(nat_info, >key, hash, _dport,
_dport, _dport);
another_round:
@@ -2457,17 +2475,30 @@ another_round:
goto next_addr;
}
-FOR_EACH_PORT_IN_RANGE(curr_dport, min_dport, max_dport) {
-nat_conn->rev_key.src.port = htons(curr_dport);
-FOR_EACH_PORT_IN_RANGE(curr_sport, min_sport, max_sport) {
-nat_conn->rev_key.dst.port = htons(curr_sport);
-if (!conn_lookup(ct, _conn->rev_key,
- time_msec(), NULL, NULL)) {
-return true;
-}
+curr_sport = orig_sport;
+curr_dport = orig_dport;
+
+nat_conn->rev_key.src.port = htons(curr_dport);
+nat_conn->rev_key.dst.port = htons(curr_sport);
+
+bool found = false;
+if (nat_info->nat_action & NAT_ACTION_DST_PORT) {
+found = nat_get_unique_l4(ct, nat_conn, _conn->rev_key.src.port,
+ curr_dport, min_dport, max_dport);
+if (!found) {
+nat_conn->rev_key.src.port = htons(orig_dport);
}
}
+if (!found) {
+found = nat_get_unique_l4(ct, nat_conn, _conn->rev_key.dst.port,
+ curr_sport, min_sport, max_sport);
+}
+
+if (found) {
+return true;
+}
+
/* Check if next IP is in range and respin. Otherwise, notify
* exhaustion to the caller. */
next_addr:
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v9 1/3] conntrack: select correct sport range for well-known origin sport
From: wenxu
Like the kernel datapath. The sport nat range for well-konwn origin
sport should limit in the well-known ports.
Signed-off-by: wenxu
Acked-by: Paolo Valerio
---
lib/conntrack.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/lib/conntrack.c b/lib/conntrack.c
index 33a1a92..44f99f3 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -2265,8 +2265,16 @@ set_sport_range(const struct nat_action_info_t *ni,
const struct conn_key *k,
if (((ni->nat_action & NAT_ACTION_SNAT_ALL) == NAT_ACTION_SRC) ||
((ni->nat_action & NAT_ACTION_DST))) {
*curr = ntohs(k->src.port);
-*min = MIN_NAT_EPHEMERAL_PORT;
-*max = MAX_NAT_EPHEMERAL_PORT;
+if (*curr < 512) {
+*min = 1;
+*max = 511;
+} else if (*curr < 1024) {
+*min = 600;
+*max = 1023;
+} else {
+*min = MIN_NAT_EPHEMERAL_PORT;
+*max = MAX_NAT_EPHEMERAL_PORT;
+}
} else {
*min = ni->min_port;
*max = ni->max_port;
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ofproto-dpif-upcall:Fix for OVS route cache re-add
>Problem Statement:
>OVS flushes and subsequently repopulates its route
>cache whenever it receives a netlink notification
>about kernel interface change. At the same time the
>port addition triggers a revalidation of all
>datapath flow cache entries. The revalidation of
>egress tunnel flows depends on correct routing
>information and can fail during the rebuild of
>the route cache, which leads to temporary insertion
>of drop flows.
>
>Solution:
>There is already a route_table_mutex that OVS seizes
>when it rebuilds the route cache.
>To ensure that flow revalidation cannot collide with
>route cache rebuild, seize that route_table_mutex also
>during revalidation. Since revalidation is multi-threaded,
>we seize the lock only on the leader thread. As a
>revalidation run can last hundreds of milliseconds,
>replace the lock() with trylock() in the route table code
>to avoid blocking the main OVS thread during revalidation.
>
>Signed-off-by: Cheekatamarla Eswara Venkata Pavan Kumar
>
>---
> lib/route-table.c | 34 --
> lib/route-table.h | 2 ++
> ofproto/ofproto-dpif-upcall.c | 14 ++
> 3 files changed, 40 insertions(+), 10 deletions(-)
>
>diff --git a/lib/route-table.c b/lib/route-table.c
>index ac82cf262..e1b17f1b8 100644
>--- a/lib/route-table.c
>+++ b/lib/route-table.c
>@@ -88,6 +88,21 @@ static void route_map_clear(void);
> static void name_table_init(void);
> static void name_table_change(const struct rtnetlink_change *, void *);
>
>+inline void route_table_lock(void)
>+{
>+ ovs_mutex_lock(_table_mutex);
>+}
>+
>+inline void route_table_unlock(void)
>+{
>+ ovs_mutex_unlock(_table_mutex);
>+}
>+
>+inline int route_table_trylock(void)
>+{
>+ return ovs_mutex_trylock(_table_mutex);
>+}
>+
> uint64_t
> route_table_get_change_seq(void)
> {
>@@ -100,7 +115,7 @@ void
> route_table_init(void)
> OVS_EXCLUDED(route_table_mutex)
> {
>-ovs_mutex_lock(_table_mutex);
>+route_table_lock();
> ovs_assert(!nln);
> ovs_assert(!route_notifier);
> ovs_assert(!route6_notifier);
>@@ -119,7 +134,7 @@ route_table_init(void)
> route_table_reset();
> name_table_init();
>
>-ovs_mutex_unlock(_table_mutex);
>+route_table_unlock();
> }
>
> /* Run periodically to update the locally maintained routing table. */
>@@ -127,7 +142,11 @@ void
> route_table_run(void)
> OVS_EXCLUDED(route_table_mutex)
> {
>-ovs_mutex_lock(_table_mutex);
>+/* Skip route table updates when the table is locked. */
>+if (route_table_trylock()) {
>+return;
>+}
>+
> if (nln) {
> rtnetlink_run();
> nln_run(nln);
>@@ -136,7 +155,7 @@ route_table_run(void)
> route_table_reset();
> }
> }
>-ovs_mutex_unlock(_table_mutex);
>+route_table_unlock();
> }
>
> /* Causes poll_block() to wake up when route_table updates are required. */
>@@ -144,12 +163,15 @@ void
> route_table_wait(void)
> OVS_EXCLUDED(route_table_mutex)
> {
>-ovs_mutex_lock(_table_mutex);
>+if (route_table_trylock()) {
>+return;
>+}
>+
> if (nln) {
> rtnetlink_wait();
> nln_wait(nln);
> }
>-ovs_mutex_unlock(_table_mutex);
>+route_table_unlock();
> }
>
> static int
>diff --git a/lib/route-table.h b/lib/route-table.h
>index 3a02d737a..58418bd3f 100644
>--- a/lib/route-table.h
>+++ b/lib/route-table.h
>@@ -33,4 +33,6 @@ void route_table_wait(void);
> bool route_table_fallback_lookup(const struct in6_addr *ip6_dst,
> char name[],
> struct in6_addr *gw6);
>+void route_table_lock(void);
>+void route_table_unlock(void);
> #endif /* route-table.h */
>diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
>index 1c9c720f0..ab2d20833 100644
>--- a/ofproto/ofproto-dpif-upcall.c
>+++ b/ofproto/ofproto-dpif-upcall.c
>@@ -23,6 +23,7 @@
> #include "coverage.h"
> #include "cmap.h"
> #include "lib/dpif-provider.h"
>+#include "lib/route-table.h"
> #include "dpif.h"
> #include "openvswitch/dynamic-string.h"
> #include "fail-open.h"
>@@ -587,7 +588,8 @@ udpif_start_threads(struct udpif *udpif, uint32_t
>n_handlers_,
> dpif_enable_upcall(udpif->dpif);
>
> ovs_barrier_init(>reval_barrier, udpif->n_revalidators);
>-ovs_barrier_init(>pause_barrier, udpif->n_revalidators + 1);
>+/* For main thread and leader*/
>+ovs_barrier_init(>pause_barrier, 2);
> udpif->reval_exit = false;
> udpif->pause = false;
> udpif->offload_rebalance_time = time_msec();
>@@ -953,6 +955,10 @@ udpif_revalidator(void *arg)
> * on the pause_barrier */
> udpif->pause = latch_is_set(>pause_latch);
>
>+if (udpif->pause) {
>+revalidator_pause(revalidator);
>+}
>+
> /* Only the leader checks the exit latch to prevent a race where
> * some threads think it's true and exit and others
Re: [ovs-dev] [PATCH v8 3/3] conntrack: limit port clash resolution attempts
From: Paolo Valerio
Date: 2022-01-12 18:19:25
To: we...@ucloud.cn,i.maxim...@ovn.org
Cc: d...@openvswitch.org
Subject: Re: [PATCH v8 3/3] conntrack: limit port clash resolution
attempts>Hello wenxu,
>
>I tested a bit more the patch, and it seems to effectively limit the
>number of attempts. There is a case with a sufficiently large port range
>that will always tries the same ports.
>E.g. (incresing the IPs you can reduce the port range):
>
>actions=ct(commit,nat(dst=10.1.1.100-10.1.1.101:80-144)
>
>in this case the source port will never get the chance to resolve the
>clash and the only IPs/ports tested would be the ones above.
Yes , for dnat case the source port resolve should be restore attempts and
dport
>
>we...@ucloud.cn writes:
>
>> From: wenxu
>>
>> In case almost or all available ports are taken, clash resolution can
>> take a very long time, resulting in pmd hang in conntrack.
>>
>> This can happen when many to-be-natted hosts connect to same
>> destination:port (e.g. a proxy) and all connections pass the same SNAT.
>>
>> Pick a random offset in the acceptable range, then try ever smaller
>> number of adjacent port numbers, until either the limit is reached or a
>> useable port was found. This results in at most 248 attempts
>> (128 + 64 + 32 + 16 + 8, i.e. 4 restarts with new search offset)
>> instead of 64000+.
>>
>> And if thenumber of ip address will limit the max attempts and which
>> will lead the total attempts under 248.
>>
>> Signed-off-by: wenxu
>> ---
>> lib/conntrack.c | 65
>> +
>> 1 file changed, 56 insertions(+), 9 deletions(-)
>>
>> diff --git a/lib/conntrack.c b/lib/conntrack.c
>> index 2a5d72a..dae8dd7 100644
>> --- a/lib/conntrack.c
>> +++ b/lib/conntrack.c
>> @@ -2426,10 +2426,14 @@ nat_get_unique_tuple(struct conntrack *ct, const
>> struct conn *conn,
>> union ct_addr min_addr = {0}, max_addr = {0}, curr_addr = {0},
>>guard_addr = {0};
>> uint32_t hash = nat_range_hash(conn, ct->hash_basis, nat_info);
>> +uint16_t min_sport, max_sport, curr_sport, orig_sport;
>> bool pat_proto = conn->key.nw_proto == IPPROTO_TCP ||
>> conn->key.nw_proto == IPPROTO_UDP;
>> +unsigned int attempts, max_attempts, min_attempts;
>> uint16_t min_dport, max_dport, curr_dport;
>> -uint16_t min_sport, max_sport, curr_sport;
>> +uint16_t range_src, range_dst, range_max;
>> +uint32_t range_addr;
>> +unsigned int i;
>>
>> min_addr = nat_info->min_addr;
>> max_addr = nat_info->max_addr;
>> @@ -2441,11 +2445,29 @@ nat_get_unique_tuple(struct conntrack *ct, const
>> struct conn *conn,
>> * we can stop once we reach it. */
>> guard_addr = curr_addr;
>>
>> -set_sport_range(nat_info, >key, hash, _sport,
>> +set_sport_range(nat_info, >key, hash, _sport,
>> _sport, _sport);
>> set_dport_range(nat_info, >key, hash, _dport,
>> _dport, _dport);
>>
>> +range_src = max_sport - min_sport + 1;
>> +range_dst = max_dport - min_dport + 1;
>> +range_max = range_src > range_dst ? range_src : range_dst;
>> +if (conn->key.dl_type == htons(ETH_TYPE_IP)) {
>> +range_addr = ntohl(max_addr.ipv4) - ntohl(min_addr.ipv4) + 1;
>> +} else {
>> +range_addr = nat_ipv6_addrs_delta(_info->min_addr.ipv6,
>> + _info->max_addr.ipv6) + 1;
>> +}
>> +max_attempts = 128 / range_addr;
>> +if (max_attempts < 1) {
>> +max_attempts = 1;
>> +}
>> +min_attempts = 16 / range_addr;
>> +if (min_attempts < 2) {
>> +min_attempts = 2;
>> +}
>> +
>> another_round:
>> store_addr_to_key(_addr, _conn->rev_key,
>>nat_info->nat_action);
>> @@ -2459,22 +2481,47 @@ another_round:
>> goto next_addr;
>> }
>>
>> +curr_sport = orig_sport;
>
>I think that you should restore the dport as well, right?
>
>> +
>> +attempts = range_max;
>> +if (attempts > max_attempts) {
>> +attempts = max_attempts;
>> +}
>> +
>> +another_port_round:
>> +i = 0;
>> if (nat_info->nat_action & NAT_ACTION_DST_PORT) {
>> FOR_EACH_PORT_IN_RANGE(curr_dport, min_dport, max_dport) {
>> -nat_conn->rev_key.src.port = htons(curr_dport);
>> +if (i++ < attempts) {
>> +nat_conn->rev_key.src.port = htons(curr_dport);
>> +if (!conn_lookup(ct, _conn->rev_key,
>> + time_msec(), NULL, NULL)) {
>> +return true;
>> +}
>> +} else {
>> +break;
>
>I don't know if it's really a problem (and maybe you noticed
>already), but breaking before you go through the whole range will change
>the dport (that is, it will not use the initial destination port) during
>the the next clash resolution (based on the source port).
>
>All in
Re: [ovs-dev] [PATCH v5 2/2] dpif-netdev/mfex: Optimize packet hash and enable autovalidator
> From: Kumar Amber
>
> This patch adds error checking of packet hashes to the mfex
> autovalidator infrastructure, ensuring that hashes calculated by
> optimized mfex implementations is identical to the scalar code.
>
> This patch avoids calculating the software hash of the packet again
> if the optimized miniflow-extract hit and has already calculated the
> packet hash. In cases of scalar miniflow extract, the normal hashing
> calculation is performed.
>
> Signed-off-by: Kumar Amber
> Signed-off-by: Harry van Haaren
Thanks for the patch Harry/Amber, few queries below.
>
> ---
>
> v5:
> - Always use SW hashing to validate optimized hash implementations
> ---
> lib/dpif-netdev-avx512.c | 6 +++---
> lib/dpif-netdev-private-extract.c | 19 +++
> 2 files changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/lib/dpif-netdev-avx512.c b/lib/dpif-netdev-avx512.c
> index b7131ba3f..c68b79f6b 100644
> --- a/lib/dpif-netdev-avx512.c
> +++ b/lib/dpif-netdev-avx512.c
> @@ -212,15 +212,15 @@ dp_netdev_input_outer_avx512(struct
> dp_netdev_pmd_thread *pmd,
> if (!mfex_hit) {
> /* Do a scalar miniflow extract into keys. */
> miniflow_extract(packet, >mf);
> +key->len = netdev_flow_key_size(miniflow_n_values(>mf));
> +key->hash = dpif_netdev_packet_get_rss_hash_orig_pkt(packet,
> + >mf);
So I'm not sure, but has there been any investigation into the effect of only
storing this info when !mfex_hit occurs?
Prior to this these values were stored regardless. My concern here is that is
there a case where this info is needed even if the mfex_hit is true?
> }
>
> /* Cache TCP and byte values for all packets. */
> pkt_meta[i].bytes = dp_packet_size(packet);
> pkt_meta[i].tcp_flags = miniflow_get_tcp_flags(>mf);
>
> -key->len = netdev_flow_key_size(miniflow_n_values(>mf));
> -key->hash = dpif_netdev_packet_get_rss_hash_orig_pkt(packet,
> >mf);
> -
> if (emc_enabled) {
> f = emc_lookup(>emc_cache, key);
>
> diff --git a/lib/dpif-netdev-private-extract.c
> b/lib/dpif-netdev-private-extract.c
> index a29bdcfa7..2957c0172 100644
> --- a/lib/dpif-netdev-private-extract.c
> +++ b/lib/dpif-netdev-private-extract.c
> @@ -252,8 +252,15 @@ dpif_miniflow_extract_autovalidator(struct
> dp_packet_batch *packets,
>
> /* Run scalar miniflow_extract to get default result. */
> DP_PACKET_BATCH_FOR_EACH (i, packet, packets) {
> +
> +/* remove the NIC RSS bit to force SW hashing for validation. */
Minor, Capitalize Remove.
> +dp_packet_reset_offload(packet);
> +
Is there any performance penalty for forcing this reset each time?
Ian
> pkt_metadata_init(>md, in_port);
> miniflow_extract(packet, [i].mf);
> +keys[i].len = netdev_flow_key_size(miniflow_n_values([i].mf));
> +keys[i].hash = dpif_netdev_packet_get_rss_hash_orig_pkt(packet,
> +[i].mf);
>
> /* Store known good metadata to compare with optimized metadata. */
> good_l2_5_ofs[i] = packet->l2_5_ofs;
> @@ -271,7 +278,10 @@ dpif_miniflow_extract_autovalidator(struct
> dp_packet_batch *packets,
> /* Reset keys and offsets before each implementation. */
> memset(test_keys, 0, keys_size * sizeof(struct netdev_flow_key));
> DP_PACKET_BATCH_FOR_EACH (i, packet, packets) {
> +/* Ensure offsets is set by the opt impl. */
> dp_packet_reset_offsets(packet);
> +/* Ensure packet hash is re-calculated by opt impl. */
> +dp_packet_reset_offload(packet);
> }
> /* Call optimized miniflow for each batch of packet. */
> uint32_t hit_mask = mfex_impls[j].extract_func(packets, test_keys,
> @@ -303,6 +313,15 @@ dpif_miniflow_extract_autovalidator(struct
> dp_packet_batch *packets,
> failed = 1;
> }
>
> +/* Check hashes are equal. */
> +if ((keys[i].hash != test_keys[i].hash) ||
> +(keys[i].len != test_keys[i].len)) {
> +ds_put_format(_msg, "Good hash: %d len: %d\tTest hash:%d"
> + " len:%d\n", keys[i].hash, keys[i].len,
> + test_keys[i].hash, test_keys[i].len);
> +failed = 1;
> +}
> +
> if (!miniflow_equal([i].mf, _keys[i].mf)) {
> uint32_t block_cnt = miniflow_n_values([i].mf);
> uint32_t test_block_cnt =
> miniflow_n_values(_keys[i].mf);
> --
> 2.25.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [RFC ovn] Add LTS section to release documentation.
OVN LTS releases have a lot of ambiguity, so this is intended to codify LTS support and cadence. Signed-off-by: Mark Michelson --- Documentation/internals/release-process.rst | 28 + 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/Documentation/internals/release-process.rst b/Documentation/internals/release-process.rst index f37c09e51..1ad43f3cc 100644 --- a/Documentation/internals/release-process.rst +++ b/Documentation/internals/release-process.rst @@ -75,16 +75,24 @@ Scheduling`_ for the timing of each stage: 2019.10.2, and so on. The process is the same for these additional release as for a .0 release. -At most two release branches are formally maintained at any given time: the -latest release and the latest release designed as LTS. An LTS release is one -that the OVN project has designated as being maintained for a longer period of -time. Currently, an LTS release is maintained until the next LTS is chosen. -There is not currently a strict guideline on how often a new LTS release is -chosen, but so far it has been about every 2 years. That could change based on -the current state of OVN development. For example, we do not want to designate -a new release as LTS that includes disruptive internal changes, as that may -make it harder to support for a longer period of time. Discussion about -choosing the next LTS release occurs on the OVS development mailing list. +Long-term Support Releases +-- + +The OVN project will periodically designate a release as "long-term support" or +LTS for short. An LTS release has the distinction of being maintained for +longer than a standard release. + +LTS releases will receive bug fixes until the point that another LTS is +released. At that point, the old LTS will receive an additional year of +critical and security fixes. Critical fixes are those that are required to +ensure basic operation (e.g. memory leak fixes, crash fixes). Security fixes +are those that address concerns about exploitable flaws in OVN and that have a +corresponding CVE report. + +LTS releases are scheduled to be released once every three years. This means +that any given LTS will receive bug fix support for three years, followed by +one year of critical bug fixes and security fixes. + Release Numbering - -- 2.31.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v5 8/8] odp-execute: Add ISA implementation of push_vlan action.
> This commit adds the AVX512 implementation of the push_vlan action.
> The implementation here is auto-validated by the miniflow
> extract autovalidator, hence its correctness can be easily
> tested and verified.
>
> Signed-off-by: Emma Finn
Hi Emma, thanks for the patch.
I think given the conversation on patch 7 of this series would help me work
through this better. Overall I understand the majority of the new action logic
and design but just have a few questions that we can discuss on patch 7 that
will no doubt help me complete review here.
Thanks
Ian
> ---
> lib/odp-execute-avx512.c | 62 +++
> lib/odp-execute-private.c | 1 +
> lib/odp-execute.c | 24 +--
> 3 files changed, 72 insertions(+), 15 deletions(-)
>
> diff --git a/lib/odp-execute-avx512.c b/lib/odp-execute-avx512.c
> index fcf27f070..03c0fd446 100644
> --- a/lib/odp-execute-avx512.c
> +++ b/lib/odp-execute-avx512.c
> @@ -43,6 +43,13 @@ static inline void ALWAYS_INLINE
> avx512_dp_packet_resize_l2(struct dp_packet *b, int resize_by_bytes)
> {
> /* update packet size/data pointers */
> +if (resize_by_bytes >= 0) {
> +dp_packet_prealloc_headroom(b, resize_by_bytes);
> +} else {
> +ovs_assert(dp_packet_size(b) - dp_packet_l2_pad_size(b) >=
> +-resize_by_bytes);
> +}
> +
> dp_packet_set_data(b, (char *) dp_packet_data(b) - resize_by_bytes);
> dp_packet_set_size(b, dp_packet_size(b) + resize_by_bytes);
>
> @@ -50,9 +57,9 @@ avx512_dp_packet_resize_l2(struct dp_packet *b, int
> resize_by_bytes)
> const __m128i v_zeros = _mm_setzero_si128();
> const __m128i v_u16_max = _mm_cmpeq_epi16(v_zeros, v_zeros);
>
> -/* Only these lanes can be incremented for push-VLAN action. */
> +/* Only these lanes can be incremented/decremented for L2. */
> const uint8_t k_lanes = 0b1110;
> -__m128i v_offset = _mm_set1_epi16(VLAN_HEADER_LEN);
> +__m128i v_offset = _mm_set1_epi16(abs(resize_by_bytes));
>
> /* Load packet and compare with UINT16_MAX */
> void *adjust_ptr = >l2_pad_size;
> @@ -60,9 +67,17 @@ avx512_dp_packet_resize_l2(struct dp_packet *b, int
> resize_by_bytes)
> __mmask8 k_cmp = _mm_mask_cmpneq_epu16_mask(k_lanes, v_adjust_src,
> v_u16_max);
>
> -/* Add VLAN_HEADER_LEN using compare mask, store results. */
> -__m128i v_adjust_wip = _mm_mask_sub_epi16(v_adjust_src, k_cmp,
> - v_adjust_src, v_offset);
> +/* Update VLAN_HEADER_LEN using compare mask, store results. */
> +__m128i v_adjust_wip;
> +
> +if (resize_by_bytes >= 0) {
> +v_adjust_wip = _mm_mask_add_epi16(v_adjust_src, k_cmp,
> +v_adjust_src, v_offset);
> +} else {
> +v_adjust_wip = _mm_mask_sub_epi16(v_adjust_src, k_cmp,
> +v_adjust_src, v_offset);
> +}
> +
> _mm_storeu_si128(adjust_ptr, v_adjust_wip);
>
> }
> @@ -80,7 +95,6 @@ avx512_eth_pop_vlan(struct dp_packet *packet)
> 16 - VLAN_HEADER_LEN);
> _mm_storeu_si128((void *) veh, v_realign);
> avx512_dp_packet_resize_l2(packet, -VLAN_HEADER_LEN);
> -
> }
> }
>
> @@ -96,6 +110,41 @@ action_avx512_pop_vlan(void *dp OVS_UNUSED, struct
> dp_packet_batch *batch,
> }
> }
>
> +static inline void ALWAYS_INLINE
> +avx512_eth_push_vlan(struct dp_packet *packet, ovs_be16 tpid, ovs_be16 tci)
> +{
> +avx512_dp_packet_resize_l2(packet, VLAN_HEADER_LEN);
> +
> +/* Build up the VLAN TCI/TPID, and merge with the moving of Ether. */
> +char *pkt_data = (char *) dp_packet_data(packet);
> +const uint16_t tci_proc = tci & htons(~VLAN_CFI);
> +const uint32_t tpid_tci = (tci_proc << 16) | tpid;
> +
> +static const uint8_t vlan_push_shuffle_mask[16] = {
> +4, 5, 6, 7, 8, 9, 10, 11,
> +12, 13, 14, 15, 0xFF, 0xFF, 0xFF, 0xFF
> +};
> +
> +__m128i v_ether = _mm_loadu_si128((void *) pkt_data);
> +__m128i v_index = _mm_loadu_si128((void *) vlan_push_shuffle_mask);
> +__m128i v_shift = _mm_shuffle_epi8(v_ether, v_index);
> +__m128i v_vlan_hdr = _mm_insert_epi32(v_shift, tpid_tci, 3);
> + _mm_storeu_si128((void *) pkt_data, v_vlan_hdr);
> +}
> +
> +static void
> +action_avx512_push_vlan(void *dp OVS_UNUSED, struct dp_packet_batch
> *batch,
> + const struct nlattr *a,
> + bool should_steal OVS_UNUSED)
> +{
> +struct dp_packet *packet;
> +const struct ovs_action_push_vlan *vlan = nl_attr_get(a);
> +
> +DP_PACKET_BATCH_FOR_EACH (i, packet, batch) {
> +avx512_eth_push_vlan(packet, vlan->vlan_tpid, vlan->vlan_tci);
> +}
> +}
> +
> /* Probe functions to check ISA requirements. */
> static int32_t
> avx512_isa_probe(uint32_t needs_vbmi)
> @@ -136,6 +185,7 @@
Re: [ovs-dev] [PATCH v5 7/8] odp-execute: Add ISA implementation of pop_vlan action.
> This commit adds the AVX512 implementation of the pop_vlan action.
> The implementation here is auto-validated by the miniflow
> extract autovalidator, hence its correctness can be easily
> tested and verified.
>
> Signed-off-by: Emma Finn
Hi Emma, some comments below.
> ---
> lib/odp-execute-avx512.c | 77 ++-
> lib/odp-execute-private.c | 2 +-
> lib/odp-execute-private.h | 2 +-
> 3 files changed, 78 insertions(+), 3 deletions(-)
>
> diff --git a/lib/odp-execute-avx512.c b/lib/odp-execute-avx512.c
> index aa71faa1c..fcf27f070 100644
> --- a/lib/odp-execute-avx512.c
> +++ b/lib/odp-execute-avx512.c
> @@ -14,6 +14,11 @@
> * limitations under the License.
> */
>
> +#ifdef __x86_64__
> +/* Sparse cannot handle the AVX512 instructions. */
> +#if !defined(__CHECKER__)
> +
> +
> #include
> #include
>
> @@ -25,6 +30,71 @@
>
> #include "immintrin.h"
>
> +VLOG_DEFINE_THIS_MODULE(odp_execute_avx512);
> +BUILD_ASSERT_DECL(offsetof(struct dp_packet, l2_5_ofs) +
> + MEMBER_SIZEOF(struct dp_packet, l2_5_ofs) ==
> + offsetof(struct dp_packet, l3_ofs));
> +
> +BUILD_ASSERT_DECL(offsetof(struct dp_packet, l3_ofs) +
> + MEMBER_SIZEOF(struct dp_packet, l3_ofs) ==
> + offsetof(struct dp_packet, l4_ofs));
> +
> +static inline void ALWAYS_INLINE
> +avx512_dp_packet_resize_l2(struct dp_packet *b, int resize_by_bytes)
> +{
> +/* update packet size/data pointers */
Minor, Capitalize start of comment, missing period (goes for a few of the
comments in the rest of this function also).
> +dp_packet_set_data(b, (char *) dp_packet_data(b) - resize_by_bytes);
> +dp_packet_set_size(b, dp_packet_size(b) + resize_by_bytes);
> +
> +/* Increment u16 packet offset values */
> +const __m128i v_zeros = _mm_setzero_si128();
> +const __m128i v_u16_max = _mm_cmpeq_epi16(v_zeros, v_zeros);
> +
> +/* Only these lanes can be incremented for push-VLAN action. */
> +const uint8_t k_lanes = 0b1110;
> +__m128i v_offset = _mm_set1_epi16(VLAN_HEADER_LEN);
Can you walk through the above logic, as this is the pop use cases, are you
saying you don't want to use these lanes as they should only be used for the
push vlan case?
> +
> +/* Load packet and compare with UINT16_MAX */
> +void *adjust_ptr = >l2_pad_size;
> +__m128i v_adjust_src = _mm_loadu_si128(adjust_ptr);
> +__mmask8 k_cmp = _mm_mask_cmpneq_epu16_mask(k_lanes,
> v_adjust_src,
> +v_u16_max);
> +
> +/* Add VLAN_HEADER_LEN using compare mask, store results. */
Again I'm confused here, if the operation being added is to pop_vlan why are we
adding a vlan header?
If you could give a general run down of the expected operations here and logic
it would be appreciated.
Thanks
Ian
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v5 6/8] odp-execute: Add ISA implementation of actions.
> -Original Message- > From: Finn, Emma > Sent: Wednesday, January 12, 2022 9:43 AM > To: d...@openvswitch.org; Van Haaren, Harry ; > Amber, Kumar ; Stokes, Ian ; > i.maxim...@ovn.org > Cc: Finn, Emma > Subject: [PATCH v5 6/8] odp-execute: Add ISA implementation of actions. > > This commit adds the AVX512 implementation of the action functionality. > > Usage: > $ ovs-appctl dpif-netdev/action-impl-set avx512 > > Signed-off-by: Emma Finn > Acked-by: Harry van Haaren HI Emma, few minor comments below, but other than those LGTM. > --- > Documentation/topics/dpdk/bridge.rst | 25 ++ > Documentation/topics/testing.rst | 20 +--- > NEWS | 1 + > lib/automake.mk | 4 +- > lib/cpu.c| 1 + > lib/cpu.h| 1 + > lib/odp-execute-avx512.c | 69 > lib/odp-execute-private.c| 9 > lib/odp-execute-private.h| 9 > 9 files changed, 131 insertions(+), 8 deletions(-) > create mode 100644 lib/odp-execute-avx512.c > > diff --git a/Documentation/topics/dpdk/bridge.rst > b/Documentation/topics/dpdk/bridge.rst > index ceee91015..67089e08f 100644 > --- a/Documentation/topics/dpdk/bridge.rst > +++ b/Documentation/topics/dpdk/bridge.rst > @@ -321,3 +321,28 @@ following command:: > ``scalar`` can be selected on core ``3`` by the following command:: > > $ ovs-appctl dpif-netdev/miniflow-parser-set -pmd 3 scalar > + > +Actions Performance > +--- > + > +Actions are used in OpenFlow flows to describe what to do when the flow > +matches a packet. Just like with the datapath interface, SIMD instructions > +can be applied to the action implementation to improve performance. > + > +OVS provides multiple implementations of the actions. > +Available implementations can be listed with the following command:: > + > +$ ovs-appctl dpif-netdev/action-impl-get > +Available Actions implementations: > +scalar (available: True, active: True) > +autovalidator (available: True, active: False) > +avx512 (available: True, active: False) > + > +By default, ``scalar`` is used. Implementations can be selected by > +name:: > + > +$ ovs-appctl dpif-netdev/action-impl-set avx512 > +action implementation set to avx512. > + > +$ ovs-appctl dpif-netdev/action-impl-set scalar > +action implementation set to scalar. > diff --git a/Documentation/topics/testing.rst > b/Documentation/topics/testing.rst > index c15d5b38f..10d0ecc48 100644 > --- a/Documentation/topics/testing.rst > +++ b/Documentation/topics/testing.rst > @@ -361,12 +361,12 @@ testsuite. > Userspace datapath: Testing and Validation of CPU-specific Optimizations > > > -As multiple versions of the datapath classifier and packet parsing functions > -can co-exist, each with different CPU ISA optimizations, it is important to > -validate that they all give the exact same results. To easily test all the > -implementations, an ``autovalidator`` implementation of them exists. This > -implementation runs all other available implementations, and verifies that > the > -results are identical. > +As multiple versions of the datapath classifier, packet parsing functions and > +actions can co-exist, each with different CPU ISA optimizations, it is > +important to validate that they all give the exact same results. To easily > +test all the implementations, an ``autovalidator`` implementation of them > +exists. This implementation runs all other available implementations, and > +verifies that the results are identical. > > Running the OVS unit tests with the autovalidator enabled ensures all > implementations provide the same results. Note that the performance of the > @@ -382,18 +382,24 @@ To set the autovalidator for the packet parser, use > this command:: > > $ ovs-appctl dpif-netdev/miniflow-parser-set autovalidator > > +To set the autovalidator for actions, use this command:: > + > +$ ovs-appctl dpif-netdev/action-impl-set autovalidator > + > To run the OVS unit test suite with the autovalidator as the default > implementation, it is required to recompile OVS. During the recompilation, > the default priority of the `autovalidator` implementation is set to the > maximum priority, ensuring every test will be run with every implementation:: > > -$ ./configure --enable-autovalidator --enable-mfex-default-autovalidator > +$ ./configure --enable-autovalidator --enable-mfex-default-autovalidator > \ > +--enable-actions-default-autovalidator > > The following line should be seen in the configuration log when the above > options are used:: > > checking whether DPCLS Autovalidator is default implementation... yes > checking whether MFEX Autovalidator is default implementation... yes >
Re: [ovs-dev] [PATCH v5 4/8] odp-execute: Add command to switch action implementation.
> This commit adds a new command to allow the user to switch
> the active action implementation at runtime. A probe function
> is executed before switching the implementation, to ensure
> the CPU is capable of running the ISA required.
>
> Usage:
> $ ovs-appctl dpif-netdev/action-impl-set scalar
>
> This commit also adds a new command to retrieve the list of available
> action implementations. This can be used by to check what implementations
> of actions are available and what implementation is active during runtime.
>
> Usage:
>$ ovs-appctl dpif-netdev/action-impl-get
Hi Emma,
In general this look straight forward enough. General query, I assume that
scalar is always the default used, and that the user does not actually have to
set it to use it by default?
The only time the user should have to set it to scalar would be if they were
using a different implementation and wished to revert to scalar?
Thanks
Ian
>
> Added separate test-case for ovs-actions get/set commands:
> 1023: PMD - ovs-actions configuration
>
> Signed-off-by: Emma Finn
> Co-authored-by: Kumar Amber
> Signed-off-by: Kumar Amber
> Acked-by: Harry van Haaren
> ---
> NEWS| 2 ++
> lib/dpif-netdev-unixctl.man | 6 ++
> lib/dpif-netdev.c | 39 +
> lib/odp-execute-private.c | 14 +
> lib/odp-execute.h | 3 +++
> tests/pmd.at| 21
> 6 files changed, 85 insertions(+)
>
> diff --git a/NEWS b/NEWS
> index 26be454df..42bb876da 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -21,6 +21,8 @@ Post-v2.16.0
> * Add support for running threads on cores >= RTE_MAX_LCORE.
> * Add actions auto-validator function to compare different actions
> implementations against default implementation.
> + * Add command line option to switch between different actions
> + implementations available at run time.
> - Python:
> * For SSL support, the use of the pyOpenSSL library has been replaced
> with the native 'ssl' module.
> diff --git a/lib/dpif-netdev-unixctl.man b/lib/dpif-netdev-unixctl.man
> index 8cd847416..500daf4de 100644
> --- a/lib/dpif-netdev-unixctl.man
> +++ b/lib/dpif-netdev-unixctl.man
> @@ -262,3 +262,9 @@ PMDs in the case where no value is specified. By default
> "scalar" is used.
> \fIstudy_cnt\fR defaults to 128 and indicates the number of packets that the
> "study" miniflow implementation must parse before choosing an optimal
> implementation.
> +
> +.IP "\fBdpif-netdev/action-impl-get\fR
> +Lists the actions implementations that are available.
> +.
> +.IP "\fBdpif-netdev/action-impl-set\fR \fIaction_impl\fR"
> +Sets the action to be used to \fIaction_impl\fR. By default "scalar" is used.
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index eada4fcd7..f6cc779ef 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -60,6 +60,7 @@
> #include "netdev-vport.h"
> #include "netlink.h"
> #include "odp-execute.h"
> +#include "odp-execute-private.h"
> #include "odp-util.h"
> #include "openvswitch/dynamic-string.h"
> #include "openvswitch/list.h"
> @@ -1330,6 +1331,38 @@ error:
> ds_destroy();
> }
>
> +static void
> +action_impl_set(struct unixctl_conn *conn, int argc OVS_UNUSED,
> +const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
> +{
> +struct ds reply = DS_EMPTY_INITIALIZER;
> +
> +int32_t err = odp_actions_impl_set(argv[1]);
> +if (err) {
> +ds_put_format(, "action implementation %s not found.\n",
> + argv[1]);
> +const char *reply_str = ds_cstr();
> +unixctl_command_reply_error(conn, reply_str);
> +VLOG_ERR("%s", reply_str);
> +ds_destroy();
> +return;
> +}
> +
> +ds_put_format(, "action implementation set to %s.\n", argv[1]);
> +unixctl_command_reply(conn, ds_cstr());
> +ds_destroy();
> +}
> +
> +static void
> +action_impl_get(struct unixctl_conn *conn, int argc OVS_UNUSED,
> +const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
> +{
> +struct ds reply = DS_EMPTY_INITIALIZER;
> +odp_execute_action_get();
> +unixctl_command_reply(conn, ds_cstr());
> +ds_destroy();
> +}
> +
> static void
> dpif_netdev_pmd_rebalance(struct unixctl_conn *conn, int argc,
>const char *argv[], void *aux OVS_UNUSED)
> @@ -1567,6 +1600,12 @@ dpif_netdev_init(void)
> unixctl_command_register("dpif-netdev/miniflow-parser-get", "",
> 0, 0, dpif_miniflow_extract_impl_get,
> NULL);
> +unixctl_command_register("dpif-netdev/action-impl-set", "name",
> + 1, 1, action_impl_set,
> + NULL);
> +unixctl_command_register("dpif-netdev/action-impl-get", "",
> + 0, 0, action_impl_get,
> + NULL);
> return
Re: [ovs-dev] OVS DPDK MFEX Unit Tests Failing
> -Original Message- > From: David Marchand > Sent: Wednesday 12 January 2022 17:05 > To: Ferriter, Cian > Cc: Phelan, Michael ; d...@openvswitch.org; Ilya > Maximets > Subject: Re: [ovs-dev] OVS DPDK MFEX Unit Tests Failing > > On Wed, Jan 12, 2022 at 5:54 PM Ferriter, Cian > wrote: > > I tested your fix and it works, but I had to modify the port number from 0 > > to 1. If I leave it at 0, > the tests still fail. > > It depends on what DPDK ports get initialised on your system. > On mine, I had no pci device bound to vfio-pci (and unloaded mlx5 > drivers), so the net/pcap port got the 0 dpdk portid. > > > > > > I put the modifications I made inline below. > > > > Perhaps we need to wildcard this number? > > Yes, like what I did for mlx5 testing: > https://patchwork.ozlabs.org/project/openvswitch/patch/20220103141552.27060-2- > david.march...@redhat.com/ > > I updated my repo and I'll send a fix on the ml. > https://github.com/david-marchand/ovs/commit/system-dpdk > > > -- > David Marchand The fix on your updated repo works for me. I'll give my ack here even though this isn't the patch. Happy to test the patch once it's on the mailing list and ack it too. Acked-by: Cian Ferriter ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4 0/9] Actions Infrastructure + Optimizations
On 1/6/22 14:11, Van Haaren, Harry wrote: >> -Original Message- >> From: Finn, Emma >> Sent: Wednesday, January 5, 2022 4:54 PM >> To: d...@openvswitch.org; Van Haaren, Harry ; >> Amber, Kumar >> Cc: Finn, Emma >> Subject: [PATCH v4 0/9] Actions Infrastructure + Optimizations >> >> --- >> v4: >> - Rebase to master >> - Add ISA implementation of push_vlan action > > Thanks for the updated patchset Emma & Amber. > > Overall, this is working as expected and I've only had some minor > comments throughout the patchset. I've added my Acked-by to most > patches, some small open questions remain to be addressed in a v5. > > +CC Ian/Ilya , I'd like to see the v5 get merged, so let's continue to work > towards that. Hi, Harry, Ian, others. Following up from a brief conversation during today's upstream meeting. It was brought to my attention that you're expecting this series and the 'hash' one to be accepted into 2.17. Though there are few issues with that: 1. This review for v4 was actually very first review of the patch set. The other one as of today doesn't have any reviews at all. Looking at the change log for this patch set it doesn't seem that internal reviews behind the closed doors (if there were any) requested any significant changes. In any case, internal reviews is not the way how open-source projects work. 2. The soft freeze for 2.17 began on Jan 3 in accordance with our release schedule (even a bit later), and as you know, during the soft freeze we're not normally accepting patches that wasn't already reviewed before the soft freeze begun. https://mail.openvswitch.org/pipermail/ovs-dev/2022-January/390487.html That's not the end of a world, but you need to request an exception in reply to the email linked above. But I have a few high-level concerns regarding the patch set itself, and that's a bigger problem for me: 1. What are the benefits of these patch sets? A lot of infrastructure changes are made, but the benefits of them are unclear. Why these changes are needed in the end? I believe, that was the main reason why community had no interest in reviewing these patches. 2.17 is supposed to be a new LTS, so infrastructure changes without clear benefits might not be a good fit taking into account time constraints and lack of reviews. 2. The same concern that I already brought to you attention in other conversations, e.g. on the ovs-security list about a month ago. It's related to all developments in that area: why this is tied to the userspace datapath? i.e. why execution of actions depends on the datapath? This seems artificial and complicates testing a lot. Like current autovalidator is not able to test most of the packet parsing cases, the same way autovalidator will not be able to test execution of actions. I have some more comments, but they are more related to the actual code and above 2 are the most important for now. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] OVS DPDK MFEX Unit Tests Failing
On Wed, Jan 12, 2022 at 5:54 PM Ferriter, Cian wrote: > I tested your fix and it works, but I had to modify the port number from 0 to > 1. If I leave it at 0, the tests still fail. It depends on what DPDK ports get initialised on your system. On mine, I had no pci device bound to vfio-pci (and unloaded mlx5 drivers), so the net/pcap port got the 0 dpdk portid. > > I put the modifications I made inline below. > > Perhaps we need to wildcard this number? Yes, like what I did for mlx5 testing: https://patchwork.ozlabs.org/project/openvswitch/patch/20220103141552.27060-2-david.march...@redhat.com/ I updated my repo and I'll send a fix on the ml. https://github.com/david-marchand/ovs/commit/system-dpdk -- David Marchand ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] OVS DPDK MFEX Unit Tests Failing
Hi David,
I tested your fix and it works, but I had to modify the port number from 0 to
1. If I leave it at 0, the tests still fail.
I put the modifications I made inline below.
Perhaps we need to wildcard this number?
Thanks,
Cian
> -Original Message-
> From: dev On Behalf Of David Marchand
> Sent: Wednesday 12 January 2022 16:17
> To: Phelan, Michael
> Cc: d...@openvswitch.org; Ilya Maximets
> Subject: Re: [ovs-dev] OVS DPDK MFEX Unit Tests Failing
>
> On Wed, Jan 12, 2022 at 4:48 PM Phelan, Michael
> wrote:
> >
> > Hi,
> >
> > During internal testing of the AVX-512 CI, a bug was picked up on the OVS
> > master branch which
> resulted in the MFEX unit tests consistently failing. I believe the bug was
> introduced by commit
> d446dcb7e03fc7bd4e3050c83c22233b0a46d364 “system-dpdk: Refactor common logs
> matching”. It looks like
> this commit changed the logs which in turn meant that the expected output
> became outdated causing the
> unit tests to fail. The unit tests are still failing with the most recent
> updates to master so this
> bug does not seem to have been fixed with any commits that have come after
> the one specified.
>
> Thanks for reporting.
> I did not recheck my series after the dpdk upgrade to 21.11 (with only
> the first patch applied and the whole series does not have the issue
> since it does not rely on net/pcap anymore).
>
> Could you try the following diff:
>
> diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at
> index 1dd7aae1b..922f27032 100644
> --- a/tests/system-dpdk.at
> +++ b/tests/system-dpdk.at
> @@ -243,7 +243,10 @@ OVS_WAIT_UNTIL([test `ovs-vsctl get interface p1
> statistics | grep -oP 'rx_packe
>
> dnl Clean up
> AT_CHECK([ovs-vsctl del-port br0 p1], [], [stdout], [stderr])
> -OVS_VSWITCHD_STOP("[SYSTEM_DPDK_ALLOWED_LOGS]")
> +OVS_VSWITCHD_STOP("m4_join([], [SYSTEM_DPDK_ALLOWED_LOGS], [
> +\@Interface p1 does not support MTU configuration, max packet size
> supported is 1500.@d
> +\@Rx checksum offload is not supported on port 0@d
Here, I have:
\@Rx checksum offload is not supported on port 1@d
> +])")
> AT_CLEANUP
> dnl
> --
>
> @@ -271,7 +274,10 @@ OVS_WAIT_UNTIL([test `ovs-vsctl get interface p1
> statistics | grep -oP 'rx_packe
>
> dnl Clean up
> AT_CHECK([ovs-vsctl del-port br0 p1], [], [stdout], [stderr])
> -OVS_VSWITCHD_STOP("[SYSTEM_DPDK_ALLOWED_LOGS]")
> +OVS_VSWITCHD_STOP("m4_join([], [SYSTEM_DPDK_ALLOWED_LOGS], [
> +\@Interface p1 does not support MTU configuration, max packet size
> supported is 1500.@d
> +\@Rx checksum offload is not supported on port 0@d
Here I have:
\@Rx checksum offload is not supported on port 1@d
> +])")
> AT_CLEANUP
> dnl
> --
>
> @@ -389,6 +395,8 @@ OVS_VSWITCHD_STOP("m4_join([],
> [SYSTEM_DPDK_ALLOWED_LOGS], [
> \@Error: no miniflow extract name provided. Output of
> miniflow-parser-get shows implementation list.@d
> \@Error: unknown miniflow extract implementation superstudy.@d
> \@Error: invalid study_pkt_cnt value: -pmd.@d
> +\@Interface p1 does not support MTU configuration, max packet size
> supported is 1500.@d
> +\@Rx checksum offload is not supported on port 0@d
Here I have:
\@Rx checksum offload is not supported on port 1@d
> ])")
> AT_CLEANUP dnl
> dnl
> --
>
>
> --
> David Marchand
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] OVS DPDK MFEX Unit Tests Failing
On 1/12/22 17:31, David Marchand wrote: > On Wed, Jan 12, 2022 at 5:17 PM David Marchand > wrote: >> >> On Wed, Jan 12, 2022 at 4:48 PM Phelan, Michael >> wrote: >>> >>> Hi, >>> >>> During internal testing of the AVX-512 CI, a bug was picked up on the OVS >>> master branch which resulted in the MFEX unit tests consistently failing. I >>> believe the bug was introduced by commit >>> d446dcb7e03fc7bd4e3050c83c22233b0a46d364 “system-dpdk: Refactor common logs >>> matching”. It looks like this commit changed the logs which in turn meant >>> that the expected output became outdated causing the unit tests to fail. >>> The unit tests are still failing with the most recent updates to master so >>> this bug does not seem to have been fixed with any commits that have come >>> after the one specified. >> >> Thanks for reporting. >> I did not recheck my series after the dpdk upgrade to 21.11 (with only >> the first patch applied and the whole series does not have the issue >> since it does not rely on net/pcap anymore). >> >> Could you try the following diff: > > Sorry, forgot about gmail mangling stuff. > > Please try: https://github.com/david-marchand/ovs/commit/system-dpdk > > Thanks for the report. And thanks, David, for checking, I can not verify the change right now, I just wanted to rant for a bit about system-dpdk testsuite being highly unstable. It depends on the system configuration too much. Today I touched my iommu configuration and now all the tests are failing due to tons of virtual address hint warnings: +2022-01-12T16:25:40.190Z|00023|dpdk|WARN|EAL: WARNING! Base virtual address hint (0x15000 != 0x7f7855c9) not respected! +2022-01-12T16:25:40.190Z|00024|dpdk|WARN|EAL:This may cause issues with mapping memory into secondary processes +2022-01-12T16:25:40.191Z|00025|dpdk|WARN|EAL: WARNING! Base virtual address hint (0x1b000 != 0x7f7855e0c000) not respected! +2022-01-12T16:25:40.191Z|00026|dpdk|WARN|EAL:This may cause issues with mapping memory into secondary processes +2022-01-12T16:25:40.191Z|00027|dpdk|WARN|EAL: WARNING! Base virtual address hint (0x24000c000 != 0x7f704000) not respected! +2022-01-12T16:25:40.191Z|00028|dpdk|WARN|EAL:This may cause issues with mapping memory into secondary processes Maybe we should just disable all DPDK warnings? BTW, DPDK should probably not emit a warning about secondary process for applications with disabled multiprocessing. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] OVS DPDK MFEX Unit Tests Failing
Hi, During internal testing of the AVX-512 CI, a bug was picked up on the OVS master branch which resulted in the MFEX unit tests consistently failing. I believe the bug was introduced by commit d446dcb7e03fc7bd4e3050c83c22233b0a46d364 "system-dpdk: Refactor common logs matching". It looks like this commit changed the logs which in turn meant that the expected output became outdated causing the unit tests to fail. The unit tests are still failing with the most recent updates to master so this bug does not seem to have been fixed with any commits that have come after the one specified. The CI is still in testing so the issue was only reported internally. We will investigate and resolve this issue ASAP as it is impacting git master right now, but wanted to inform OVS community of useful (internal) results of the CI! Kind Regards, Michael. -- Intel Research and Development Ireland Limited Registered in Ireland Registered Office: Collinstown Industrial Park, Leixlip, County Kildare Registered Number: 308263 This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v1 1/1] datapath-windows: Pickup Ct tuple as CT lookup key in function OvsCtSetupLookupCtx
From: Wilson Peng
CT marks which are loaded in non-first commit will be lost in ovs-windows.In
linux OVS,
the CT mark setting with same flow could be set successfully.
Currenlty Ovs-windows will create one new CT with the flowKey(Extracted from
the packet itself)
If the packet is already done DNAT action after the 1st round flow processing.
So the ct-mark
Set on previous Conntrack will be lost.In the fix, it will make use of CT tuple
src/dst address
stored in the flowKey if the value is not zero and zone in the flowKey is same
as the input zone.
In the fix, it is also to adjust function OvsProcessDeferredActions to make it
clear.
//DNAT flow
cookie=0x10400, duration=950.326s, table=EndpointDNAT, n_packets=0,
n_bytes=0, priority=200,tcp,reg3=0xc0a8fa2b,reg4=0x20050/0x7
actions=ct(commit,table=AntreaPolicyEgressRule,zone=65520,nat(dst=192.168.250.43:80),exec(load:0x1->NXM_NX_CT_MARK[2])
// Append ct_mark flow
cookie=0x1, duration=11980.701s, table=SNATConntrackCommit,
n_packets=6, n_bytes=396, priority=200,ct_state=+new+trk,ip,reg0=0x2
00/0x200,reg4=0/0xc0
actions=load:0x3->NXM_NX_REG4[22..23],ct(commit,table=SNATConntrackCommit,zone=65520,exec(load:0x1->NXM_NX_CT_MARK[4
],load:0x1->NXM_NX_CT_MARK[5]))
// SNAT flow
cookie=0x1, duration=11980.701s, table=SNATConntrackCommit,
n_packets=6, n_bytes=396, priority=200,ct_state=+new+trk,ip,reg0=0x6
00/0x600,reg4=0xc0/0xc0
actions=ct(commit,table=L2Forwarding,zone=65521,nat(src=192.168.250.1),exec(load:0x1->NXM_NX_CT_MARK[2]))
Reported-at:https://github.com/openvswitch/ovs-issues/issues/237
Signed-off-by: Wilson Peng
---
datapath-windows/ovsext/Actions.c | 2 +-
datapath-windows/ovsext/Conntrack.c | 31 +
datapath-windows/ovsext/Recirc.c| 9 ++---
datapath-windows/ovsext/Recirc.h| 3 +--
4 files changed, 35 insertions(+), 10 deletions(-)
diff --git a/datapath-windows/ovsext/Actions.c
b/datapath-windows/ovsext/Actions.c
index 0c18c6254..70ac0a0e5 100644
--- a/datapath-windows/ovsext/Actions.c
+++ b/datapath-windows/ovsext/Actions.c
@@ -2366,7 +2366,7 @@ OvsActionsExecute(POVS_SWITCH_CONTEXT switchContext,
if (status == STATUS_SUCCESS) {
status = OvsProcessDeferredActions(switchContext, completionList,
- portNo, sendFlags, NULL);
+ portNo, sendFlags);
}
return status;
diff --git a/datapath-windows/ovsext/Conntrack.c
b/datapath-windows/ovsext/Conntrack.c
index fd6f3bae0..a5d2a4858 100644
--- a/datapath-windows/ovsext/Conntrack.c
+++ b/datapath-windows/ovsext/Conntrack.c
@@ -626,6 +626,32 @@ OvsReverseIcmpType(UINT8 type)
}
}
+static __inline NDIS_STATUS
+OvsPickupCtTupleAsLookupKey(POVS_CT_KEY ctKey, UINT16 zone, OvsFlowKey
*flowKey)
+{
+UINT32 ipAddrSrc = 0, ipAddrDst = 0;
+
+if (!flowKey || !ctKey) return NDIS_STATUS_SUCCESS;
+
+if (flowKey->l2.dlType == htons(ETH_TYPE_IPV4)) {
+ipAddrSrc = flowKey->ct.tuple_ipv4.ipv4_src;
+ipAddrDst = flowKey->ct.tuple_ipv4.ipv4_dst;
+
+if ((ipAddrSrc > 0 && ipAddrDst > 0) &&
+(zone == flowKey->ct.zone)) {
+/* if the ct tuple_ipv4 in flowKey is not null and ct.zone is same
with
+ * zone parameter pickup the tuple_ipv4 value as the lookup key
+ */
+ctKey->src.addr.ipv4 = flowKey->ct.tuple_ipv4.ipv4_src;
+ctKey->dst.addr.ipv4 = flowKey->ct.tuple_ipv4.ipv4_dst;
+ctKey->nw_proto = flowKey->ct.tuple_ipv4.ipv4_proto;
+ctKey->src.port = flowKey->ct.tuple_ipv4.src_port;
+ctKey->dst.port = flowKey->ct.tuple_ipv4.dst_port;
+}
+ }
+ return NDIS_STATUS_SUCCESS;
+}
+
static __inline NDIS_STATUS
OvsCtSetupLookupCtx(OvsFlowKey *flowKey,
UINT16 zone,
@@ -646,6 +672,7 @@ OvsCtSetupLookupCtx(OvsFlowKey *flowKey,
ctx->key.src.port = flowKey->ipKey.l4.tpSrc;
ctx->key.dst.port = flowKey->ipKey.l4.tpDst;
+
if (flowKey->ipKey.nwProto == IPPROTO_ICMP) {
ICMPHdr icmpStorage;
const ICMPHdr *icmp;
@@ -700,6 +727,10 @@ OvsCtSetupLookupCtx(OvsFlowKey *flowKey,
/* Translate address first for reverse NAT */
ctx->key = natEntry->ctEntry->key;
OvsCtKeyReverse(>key);
+} else {
+if (flowKey->l2.dlType == htons(ETH_TYPE_IPV4)) {
+OvsPickupCtTupleAsLookupKey(&(ctx->key), zone, flowKey);
+}
}
ctx->hash = OvsCtHashKey(>key);
diff --git a/datapath-windows/ovsext/Recirc.c b/datapath-windows/ovsext/Recirc.c
index a32b75352..7a688c874 100644
--- a/datapath-windows/ovsext/Recirc.c
+++ b/datapath-windows/ovsext/Recirc.c
@@ -310,8 +310,7 @@ NDIS_STATUS
OvsProcessDeferredActions(POVS_SWITCH_CONTEXT switchContext,
OvsCompletionList *completionList,
UINT32 portNo,
-
Re: [ovs-dev] OVS DPDK MFEX Unit Tests Failing
On Wed, Jan 12, 2022 at 5:17 PM David Marchand wrote: > > On Wed, Jan 12, 2022 at 4:48 PM Phelan, Michael > wrote: > > > > Hi, > > > > During internal testing of the AVX-512 CI, a bug was picked up on the OVS > > master branch which resulted in the MFEX unit tests consistently failing. I > > believe the bug was introduced by commit > > d446dcb7e03fc7bd4e3050c83c22233b0a46d364 “system-dpdk: Refactor common logs > > matching”. It looks like this commit changed the logs which in turn meant > > that the expected output became outdated causing the unit tests to fail. > > The unit tests are still failing with the most recent updates to master so > > this bug does not seem to have been fixed with any commits that have come > > after the one specified. > > Thanks for reporting. > I did not recheck my series after the dpdk upgrade to 21.11 (with only > the first patch applied and the whole series does not have the issue > since it does not rely on net/pcap anymore). > > Could you try the following diff: Sorry, forgot about gmail mangling stuff. Please try: https://github.com/david-marchand/ovs/commit/system-dpdk -- David Marchand ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] dpif-netdev: fix handling of vlan and ipv4 parsing in avx512
This commit fixes the minimum packet size for the vlan/ipv4/tcp
traffic profile, which was previously incorrectly set.
This commit also disallows any fragmented IPv4 packets from being
matched in the optimized miniflow-extract, avoiding complexity of
handling fragmented packets and using scalar fallback instead.
Fixes: aa85a25095 ("dpif-netdev/mfex: Add more AVX512 traffic profiles.")
Signed-off-by: Harry van Haaren
---
This patch should be applied to 2.16 as well. I expect it applies cleanly, but
volunteer to rebase/fixup on 2.16 release and send new patch if required.
---
lib/dpif-netdev-extract-avx512.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/lib/dpif-netdev-extract-avx512.c b/lib/dpif-netdev-extract-avx512.c
index d23349482..7b21a3af9 100644
--- a/lib/dpif-netdev-extract-avx512.c
+++ b/lib/dpif-netdev-extract-avx512.c
@@ -157,7 +157,7 @@ _mm512_maskz_permutexvar_epi8_wrap(__mmask64 kmask, __m512i
idx, __m512i a)
0, 0, 0, 0, /* Src IP */ \
0, 0, 0, 0, /* Dst IP */
-#define PATTERN_IPV4_MASK PATTERN_IPV4_GEN(0xFF, 0xFE, 0xFF, 0xFF)
+#define PATTERN_IPV4_MASK PATTERN_IPV4_GEN(0xFF, 0xFF, 0xFF, 0xFF)
#define PATTERN_IPV4_UDP PATTERN_IPV4_GEN(0x45, 0, 0, 0x11)
#define PATTERN_IPV4_TCP PATTERN_IPV4_GEN(0x45, 0, 0, 0x06)
@@ -389,7 +389,7 @@ static const struct mfex_profile
mfex_profiles[PROFILE_COUNT] =
.dp_pkt_offs = {
14, UINT16_MAX, 18, 38,
},
-.dp_pkt_min_size = 46,
+.dp_pkt_min_size = 58,
},
};
--
2.25.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v5 2/2] dpif-netdev/mfex: Optimize packet hash and enable autovalidator
Bleep bloop. Greetings Harry van Haaren, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Harry van Haaren Lines checked: 95, Warnings: 1, Errors: 0 Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4] netdev-linux: Ingress policing to use matchall if basic is not available.
On 12/4/21 00:06, Mike Pattrick wrote: > Currently ingress policing uses the basic classifier to apply traffic > control filters if hardware offload is not enabled, in which case it > uses matchall. This change changes the behavior to always use matchall, > and fall back onto basic if the kernel is built without matchall > support. > > The system tests are modified to allow either basic or matchall > classification on the ingestion filter, and to allow either 1 or > 10240 packets for the packet burst filter. 1 is accurate for kernel > 5.14 and the most recent iproute2, however, 10240 is left for > compatibility with older kernels. > > Signed-off-by: Mike Pattrick > Acked-by: Eelco Chaudron > --- > lib/netdev-linux.c | 32 +--- > tests/system-offloads-traffic.at | 20 +--- > 2 files changed, 30 insertions(+), 22 deletions(-) > Thanks, Mike and Eelco! I added a NEWS entry for this change and I also re-named a patch to better describe what is going on as the current name is a bit misleading. New name: "netdev-linux: Use matchall classifier for ingress policing." With that, applied. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] OVS DPDK MFEX Unit Tests Failing
On Wed, Jan 12, 2022 at 4:48 PM Phelan, Michael
wrote:
>
> Hi,
>
> During internal testing of the AVX-512 CI, a bug was picked up on the OVS
> master branch which resulted in the MFEX unit tests consistently failing. I
> believe the bug was introduced by commit
> d446dcb7e03fc7bd4e3050c83c22233b0a46d364 “system-dpdk: Refactor common logs
> matching”. It looks like this commit changed the logs which in turn meant
> that the expected output became outdated causing the unit tests to fail. The
> unit tests are still failing with the most recent updates to master so this
> bug does not seem to have been fixed with any commits that have come after
> the one specified.
Thanks for reporting.
I did not recheck my series after the dpdk upgrade to 21.11 (with only
the first patch applied and the whole series does not have the issue
since it does not rely on net/pcap anymore).
Could you try the following diff:
diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at
index 1dd7aae1b..922f27032 100644
--- a/tests/system-dpdk.at
+++ b/tests/system-dpdk.at
@@ -243,7 +243,10 @@ OVS_WAIT_UNTIL([test `ovs-vsctl get interface p1
statistics | grep -oP 'rx_packe
dnl Clean up
AT_CHECK([ovs-vsctl del-port br0 p1], [], [stdout], [stderr])
-OVS_VSWITCHD_STOP("[SYSTEM_DPDK_ALLOWED_LOGS]")
+OVS_VSWITCHD_STOP("m4_join([], [SYSTEM_DPDK_ALLOWED_LOGS], [
+\@Interface p1 does not support MTU configuration, max packet size
supported is 1500.@d
+\@Rx checksum offload is not supported on port 0@d
+])")
AT_CLEANUP
dnl --
@@ -271,7 +274,10 @@ OVS_WAIT_UNTIL([test `ovs-vsctl get interface p1
statistics | grep -oP 'rx_packe
dnl Clean up
AT_CHECK([ovs-vsctl del-port br0 p1], [], [stdout], [stderr])
-OVS_VSWITCHD_STOP("[SYSTEM_DPDK_ALLOWED_LOGS]")
+OVS_VSWITCHD_STOP("m4_join([], [SYSTEM_DPDK_ALLOWED_LOGS], [
+\@Interface p1 does not support MTU configuration, max packet size
supported is 1500.@d
+\@Rx checksum offload is not supported on port 0@d
+])")
AT_CLEANUP
dnl --
@@ -389,6 +395,8 @@ OVS_VSWITCHD_STOP("m4_join([], [SYSTEM_DPDK_ALLOWED_LOGS], [
\@Error: no miniflow extract name provided. Output of
miniflow-parser-get shows implementation list.@d
\@Error: unknown miniflow extract implementation superstudy.@d
\@Error: invalid study_pkt_cnt value: -pmd.@d
+\@Interface p1 does not support MTU configuration, max packet size
supported is 1500.@d
+\@Rx checksum offload is not supported on port 0@d
])")
AT_CLEANUP dnl
dnl --
--
David Marchand
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v5 2/2] dpif-netdev/mfex: Optimize packet hash and enable autovalidator
From: Kumar Amber
This patch adds error checking of packet hashes to the mfex
autovalidator infrastructure, ensuring that hashes calculated by
optimized mfex implementations is identical to the scalar code.
This patch avoids calculating the software hash of the packet again
if the optimized miniflow-extract hit and has already calculated the
packet hash. In cases of scalar miniflow extract, the normal hashing
calculation is performed.
Signed-off-by: Kumar Amber
Signed-off-by: Harry van Haaren
---
v5:
- Always use SW hashing to validate optimized hash implementations
---
lib/dpif-netdev-avx512.c | 6 +++---
lib/dpif-netdev-private-extract.c | 19 +++
2 files changed, 22 insertions(+), 3 deletions(-)
diff --git a/lib/dpif-netdev-avx512.c b/lib/dpif-netdev-avx512.c
index b7131ba3f..c68b79f6b 100644
--- a/lib/dpif-netdev-avx512.c
+++ b/lib/dpif-netdev-avx512.c
@@ -212,15 +212,15 @@ dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread
*pmd,
if (!mfex_hit) {
/* Do a scalar miniflow extract into keys. */
miniflow_extract(packet, >mf);
+key->len = netdev_flow_key_size(miniflow_n_values(>mf));
+key->hash = dpif_netdev_packet_get_rss_hash_orig_pkt(packet,
+ >mf);
}
/* Cache TCP and byte values for all packets. */
pkt_meta[i].bytes = dp_packet_size(packet);
pkt_meta[i].tcp_flags = miniflow_get_tcp_flags(>mf);
-key->len = netdev_flow_key_size(miniflow_n_values(>mf));
-key->hash = dpif_netdev_packet_get_rss_hash_orig_pkt(packet, >mf);
-
if (emc_enabled) {
f = emc_lookup(>emc_cache, key);
diff --git a/lib/dpif-netdev-private-extract.c
b/lib/dpif-netdev-private-extract.c
index a29bdcfa7..2957c0172 100644
--- a/lib/dpif-netdev-private-extract.c
+++ b/lib/dpif-netdev-private-extract.c
@@ -252,8 +252,15 @@ dpif_miniflow_extract_autovalidator(struct dp_packet_batch
*packets,
/* Run scalar miniflow_extract to get default result. */
DP_PACKET_BATCH_FOR_EACH (i, packet, packets) {
+
+/* remove the NIC RSS bit to force SW hashing for validation. */
+dp_packet_reset_offload(packet);
+
pkt_metadata_init(>md, in_port);
miniflow_extract(packet, [i].mf);
+keys[i].len = netdev_flow_key_size(miniflow_n_values([i].mf));
+keys[i].hash = dpif_netdev_packet_get_rss_hash_orig_pkt(packet,
+[i].mf);
/* Store known good metadata to compare with optimized metadata. */
good_l2_5_ofs[i] = packet->l2_5_ofs;
@@ -271,7 +278,10 @@ dpif_miniflow_extract_autovalidator(struct dp_packet_batch
*packets,
/* Reset keys and offsets before each implementation. */
memset(test_keys, 0, keys_size * sizeof(struct netdev_flow_key));
DP_PACKET_BATCH_FOR_EACH (i, packet, packets) {
+/* Ensure offsets is set by the opt impl. */
dp_packet_reset_offsets(packet);
+/* Ensure packet hash is re-calculated by opt impl. */
+dp_packet_reset_offload(packet);
}
/* Call optimized miniflow for each batch of packet. */
uint32_t hit_mask = mfex_impls[j].extract_func(packets, test_keys,
@@ -303,6 +313,15 @@ dpif_miniflow_extract_autovalidator(struct dp_packet_batch
*packets,
failed = 1;
}
+/* Check hashes are equal. */
+if ((keys[i].hash != test_keys[i].hash) ||
+(keys[i].len != test_keys[i].len)) {
+ds_put_format(_msg, "Good hash: %d len: %d\tTest hash:%d"
+ " len:%d\n", keys[i].hash, keys[i].len,
+ test_keys[i].hash, test_keys[i].len);
+failed = 1;
+}
+
if (!miniflow_equal([i].mf, _keys[i].mf)) {
uint32_t block_cnt = miniflow_n_values([i].mf);
uint32_t test_block_cnt = miniflow_n_values(_keys[i].mf);
--
2.25.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v5 0/2] MFEX Hashing Optimizations
Following from the MFEX Optimizations IPv6 + Hashing patchset, https://patchwork.ozlabs.org/project/openvswitch/list/?series=275590 This patchset introduces the optimization as described at OVS Conference; https://www.openvswitch.org/support/ovscon2021/#T32 https://youtu.be/X_uPybauF3g?list=PLaJlRa-xItwARDGAUp7lXviOgOhcRxSU-=976 The optimizations allow for simpler compute to hash the packet data, and the mfex autovalidator is updated to compare resulting hash values. This ensures that the hash values from optimized and scalar hashing functions are always identical. v5: - Force autovalidator to always calculate and validate hash value. - Rename "len" variable in mfex profile describe its use better. See here for previous versions of this patchset; https://patchwork.ozlabs.org/project/openvswitch/cover/20211207110425.3873101-1-kumar.am...@intel.com/ Kumar Amber (2): dpif-netdev/mfex: Add ipv4 profile based hashing dpif-netdev/mfex: Optimize packet hash and enable autovalidator NEWS | 2 +- lib/dpif-netdev-avx512.c | 6 +-- lib/dpif-netdev-extract-avx512.c | 65 +++ lib/dpif-netdev-private-extract.c | 19 + 4 files changed, 88 insertions(+), 4 deletions(-) -- 2.25.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v5 1/2] dpif-netdev/mfex: Add ipv4 profile based hashing
From: Kumar Amber
This commit adds IPv4 profile specific hashing which
uses fixed offsets into the packet to improve hashing
performance.
Signed-off-by: Kumar Amber
Signed-off-by: Harry van Haaren
Co-authored-by: Harry van Haaren
---
v5:
- Rename "hash_len" to "key_len" to describe its use better.
---
NEWS | 2 +-
lib/dpif-netdev-extract-avx512.c | 65
2 files changed, 66 insertions(+), 1 deletion(-)
diff --git a/NEWS b/NEWS
index afef81b40..e70c968a6 100644
--- a/NEWS
+++ b/NEWS
@@ -3,6 +3,7 @@ Post-v2.16.0
- Userspace datapath:
* Optimized flow lookups for datapath flows with simple match criteria.
See 'Simple Match Lookup' in Documentation/topics/dpdk/bridge.rst.
+ * Add IPv4 profile based 5tuple hashing optimizations.
- DPDK:
* EAL argument --socket-mem is no longer configured by default upon
start-up. If dpdk-socket-mem and dpdk-alloc-mem are not specified,
@@ -38,7 +39,6 @@ Post-v2.16.0
now dp_hash. Previously this was limited to 64 buckets. This change
is mainly for the benefit of OVN load balancing configurations.
-
v2.16.0 - 16 Aug 2021
-
- Removed support for 1024-bit Diffie-Hellman key exchange, which is now
diff --git a/lib/dpif-netdev-extract-avx512.c b/lib/dpif-netdev-extract-avx512.c
index d23349482..64b1c29cb 100644
--- a/lib/dpif-netdev-extract-avx512.c
+++ b/lib/dpif-netdev-extract-avx512.c
@@ -258,6 +258,10 @@ struct mfex_profile {
uint64_t mf_bits[FLOWMAP_UNITS];
uint16_t dp_pkt_offs[4];
uint16_t dp_pkt_min_size;
+
+/* Constant data offsets for Hashing. */
+uint8_t hash_pkt_offs[6];
+uint32_t key_len;
};
/* Ensure dp_pkt_offs[4] is the correct size as in struct dp_packet. */
@@ -307,6 +311,13 @@ enum MFEX_PROFILES {
PROFILE_COUNT,
};
+/* Packet offsets for 5 tuple Hash function. */
+#define HASH_IPV4 \
+26, 30, 23, 34, 0, 0
+
+#define HASH_DT1Q_IPV4 \
+30, 34, 27, 38, 0, 0
+
/* Static const instances of profiles. These are compile-time constants,
* and are specialized into individual miniflow-extract functions.
* NOTE: Order of the fields is significant, any change in the order must be
@@ -326,6 +337,9 @@ static const struct mfex_profile
mfex_profiles[PROFILE_COUNT] =
0, UINT16_MAX, 14, 34,
},
.dp_pkt_min_size = 42,
+
+.hash_pkt_offs = { HASH_IPV4 },
+.key_len = 72,
},
[PROFILE_ETH_IPV4_TCP] = {
@@ -348,6 +362,9 @@ static const struct mfex_profile
mfex_profiles[PROFILE_COUNT] =
0, UINT16_MAX, 14, 34,
},
.dp_pkt_min_size = 54,
+
+.hash_pkt_offs = { HASH_IPV4 },
+.key_len = 80,
},
[PROFILE_ETH_VLAN_IPV4_UDP] = {
@@ -366,6 +383,9 @@ static const struct mfex_profile
mfex_profiles[PROFILE_COUNT] =
14, UINT16_MAX, 18, 38,
},
.dp_pkt_min_size = 46,
+
+.hash_pkt_offs = { HASH_DT1Q_IPV4 },
+.key_len = 80,
},
[PROFILE_ETH_VLAN_IPV4_TCP] = {
@@ -390,10 +410,40 @@ static const struct mfex_profile
mfex_profiles[PROFILE_COUNT] =
14, UINT16_MAX, 18, 38,
},
.dp_pkt_min_size = 46,
+
+.hash_pkt_offs = { HASH_DT1Q_IPV4 },
+.key_len = 88,
},
};
+static inline void
+mfex_5tuple_hash_ipv4(struct dp_packet *packet, const uint8_t *pkt,
+ struct netdev_flow_key *key,
+ const uint8_t *pkt_offsets)
+{
+if (!dp_packet_rss_valid(packet)) {
+uint32_t hash = 0;
+void *ipv4_src = (void *) [pkt_offsets[0]];
+void *ipv4_dst = (void *) [pkt_offsets[1]];
+void *ports_l4 = (void *) [pkt_offsets[3]];
+
+/* IPv4 Src and Dst. */
+hash = hash_add(hash, *(uint32_t *) ipv4_src);
+hash = hash_add(hash, *(uint32_t *) ipv4_dst);
+/* IPv4 proto. */
+hash = hash_add(hash, pkt[pkt_offsets[2]]);
+/* L4 ports. */
+hash = hash_add(hash, *(uint32_t *) ports_l4);
+hash = hash_finish(hash, 42);
+
+dp_packet_set_rss_hash(packet, hash);
+key->hash = hash;
+} else {
+key->hash = dp_packet_get_rss_hash(packet);
+}
+}
+
/* Protocol specific helper functions, for calculating offsets/lenghts. */
static int32_t
mfex_ipv4_set_l2_pad_size(struct dp_packet *pkt, struct ip_header *nh,
@@ -551,6 +601,10 @@ mfex_avx512_process(struct dp_packet_batch *packets,
/* Process TCP flags, and store to blocks. */
const struct tcp_header *tcp = (void *)[38];
mfex_handle_tcp_flags(tcp, [7]);
+
+mfex_5tuple_hash_ipv4(packet, pkt, [i],
+ profile->hash_pkt_offs);
+keys[i].len = profile->key_len;
} break;
case PROFILE_ETH_VLAN_IPV4_UDP: {
@@ -562,6 +616,10 @@ mfex_avx512_process(struct dp_packet_batch *packets,
Re: [ovs-dev] [PATCH v5 3/8] odp-execute: Add auto validation function for actions.
> This commit introduced the auto-validation function which
> allows users to compare the batch of packets obtained from
> different action implementations against the linear
> action implementation.
>
> The autovalidator function can be triggered at runtime using the
> following command:
>
> $ ovs-appctl dpif-netdev/action-impl-set autovalidator
>
> Signed-off-by: Emma Finn
> Acked-by: Harry van Haaren
Hi Emma,
Thanks for the patch, minor comment below.
> ---
> NEWS | 2 +
> lib/dp-packet.c | 23 +
> lib/dp-packet.h | 5 ++
> lib/odp-execute-private.c | 99 +++
> lib/odp-execute-private.h | 3 ++
> 5 files changed, 132 insertions(+)
>
> diff --git a/NEWS b/NEWS
> index afef81b40..26be454df 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -19,6 +19,8 @@ Post-v2.16.0
> * Add support for DPDK 21.11.
> * Forbid use of DPDK multiprocess feature.
> * Add support for running threads on cores >= RTE_MAX_LCORE.
> + * Add actions auto-validator function to compare different actions
> + implementations against default implementation.
> - Python:
> * For SSL support, the use of the pyOpenSSL library has been replaced
> with the native 'ssl' module.
> diff --git a/lib/dp-packet.c b/lib/dp-packet.c
> index 72f6d09ac..1e4ff35ef 100644
> --- a/lib/dp-packet.c
> +++ b/lib/dp-packet.c
> @@ -506,3 +506,26 @@ dp_packet_resize_l2(struct dp_packet *b, int
> increment)
> dp_packet_adjust_layer_offset(>l2_5_ofs, increment);
> return dp_packet_data(b);
> }
> +
> +bool
> +dp_packet_compare_and_log(struct dp_packet *good, struct dp_packet *test,
> + struct ds *err_str)
> +{
> +if ((good->l2_pad_size != test->l2_pad_size) ||
> +(good->l2_5_ofs != test->l2_5_ofs) ||
> +(good->l3_ofs != test->l3_ofs) ||
> +(good->l4_ofs != test->l4_ofs)) {
> +ds_put_format(err_str, "Autovalidation packet offsets failed"
> + "\n");
> +ds_put_format(err_str, "Good offsets: l2_pad_size %u,"
> + " l2_5_ofs : %u l3_ofs %u, l4_ofs %u\n",
> + good->l2_pad_size, good->l2_5_ofs,
> + good->l3_ofs, good->l4_ofs);
> +ds_put_format(err_str, "Test offsets: l2_pad_size %u,"
> + " l2_5_ofs : %u l3_ofs %u, l4_ofs %u\n",
> + test->l2_pad_size, test->l2_5_ofs,
> + test->l3_ofs, test->l4_ofs);
> +return false;
> +}
> +return true;
> +}
> diff --git a/lib/dp-packet.h b/lib/dp-packet.h
> index ee0805ae6..723215add 100644
> --- a/lib/dp-packet.h
> +++ b/lib/dp-packet.h
> @@ -236,6 +236,11 @@ void *dp_packet_steal_data(struct dp_packet *);
> static inline bool dp_packet_equal(const struct dp_packet *,
> const struct dp_packet *);
>
> +
Hi Emma, from that patch this looks like extra white space but in the code it
seems we've added a page break token which is not needed, would suggest
removing the additional token added and leaving the original token in place.
Other than that LGTM.
Thanks
Ian
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1 07/18] python: introduce OpenFlow Flow parsing
On 12/24/21 15:07, Eelco Chaudron wrote:
+class OFPFlowFactory:
See my comments on patch 8, where I do think we should get rid of this class,
and update the OFPFlow class to take this string at init.
Being more OOO it would look like this:
def __init__(self, ofp_string, id=None):
"""Constructor"""
sections = self._sections_from_string(ofp_string)
super(OFPFlow, self).__init__(sections, ofp_string, id)
Where sections_from_string(ofp_string) is just the same code as in this class
but returns the sections.
@staticmethod
def _sections_from_string(ofp_string):
...
That was more or less how I originally implemented it. I then refactored it into
a factory class because when parsing thousands of flows I saw a considerable
amount of time being spent on parser "preparation", this is: creating all the
KVParser objects. So I created the factory just to cache this data.
Another alternative would be to make them global (for the class) and initialize
them the first time we parse a flow so we could keep the nicer syntax. I'm
trying to avoid running functions on module load. WDYT?
Do we need OFPFlowFactory(object):, some times flake reports this as H238. But
might no longer be valid with the latest python3.
Not 100% sure either, I'll check. Thanks.
+"""OpenFlow Flow Factory is a class capable of creating OFPFLow objects"""
+
+def __init__(self):
+self.info_decoders = self._info_decoders()
+self.match_decoders = KVDecoders(
The name suggests that we return KVDecoders() as the __info_decoders() does, I
think all three should return the same.
What name exactly? Do you mean the _flow_match_decoders and _field_decoders?
Or do you mean match_decoders should be a function rather than the object
directly?
Trying to remember what my thoughts were ;) Guess I wondered why
_flow_match_decoders() was not doing the ‘return
KVDecoders({**self._field_decoders(), **self._flow_match_decoders()})’.
I'll unify the names to make them more consistent.
[...]
+
+@classmethod
+def _output_actions_decoders(cls):
+"""Returns the decoders for the output actions"""
If you have them, I think it might be useful, to add some example strings to
the individual decoders. This way, it's easy to see what they intend to decode.
Ack.
[...]
+def decode_learn(action_decoders):
It’s getting late, and I have a hard time (focussing ;) understanding where the
value for this one comes from? I'll pick it up from here when I continue the
review.
I hope I can clarify. Learn action has two added complexities:
- It accepts any other action key-value. For this we need to create a wrapper
around the pre-calculated action_decoders.
- The way fields can be specified is augmented. Not only we have 'field=value', but we
also have 'field=_src_' (where _src_ is another field name) and just 'field'. For this we
need to create a wrapper of field_decoders that, for each "field=X" key-value
we check if X is a field_name or if it's acually a value that we need to send to the
appropriate field_decoder to process.
Ack thanks, it makes sense now.
I'll add a comment to make it clearer.
--
Adrián Moreno
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v5 2/8] odp-execute: Add function pointer for pop_vlan action.
> -Original Message-
> From: Finn, Emma
> Sent: Wednesday, January 12, 2022 9:43 AM
> To: d...@openvswitch.org; Van Haaren, Harry ;
> Amber, Kumar ; Stokes, Ian ;
> i.maxim...@ovn.org
> Cc: Finn, Emma
> Subject: [PATCH v5 2/8] odp-execute: Add function pointer for pop_vlan action.
>
> This commit removes the pop_vlan action from the large switch
> and creates a separate function for batched processing. A function
> pointer is also added to call the new batched function for the pop_vlan
> action.
>
> Signed-off-by: Emma Finn
> Acked-by: Harry van Haaren
Thanks for the patch Emma, Minor comment below.
> ---
> lib/odp-execute-private.c | 19 +-
> lib/odp-execute.c | 41 +--
> lib/odp-execute.h | 2 ++
> 3 files changed, 55 insertions(+), 7 deletions(-)
>
> diff --git a/lib/odp-execute-private.c b/lib/odp-execute-private.c
> index 6441c491c..d88ff4921 100644
> --- a/lib/odp-execute-private.c
> +++ b/lib/odp-execute-private.c
> @@ -29,13 +29,14 @@
>
> int32_t action_autoval_init(struct odp_execute_action_impl *self);
> VLOG_DEFINE_THIS_MODULE(odp_execute_private);
> +static uint32_t active_action_impl_index;
>
> static struct odp_execute_action_impl action_impls[] = {
> [ACTION_IMPL_SCALAR] = {
> .available = 1,
> .name = "scalar",
> .probe = NULL,
> -.init_func = NULL,
> +.init_func = odp_action_scalar_init,
> },
> };
>
> @@ -49,6 +50,22 @@ action_impl_copy_funcs(struct odp_execute_action_impl
> *to,
> }
> }
>
> +int32_t
> +odp_execute_action_set(const char *name,
> + struct odp_execute_action_impl *active)
> +{
> +uint32_t i;
> +for (i = 0; i < ACTION_IMPL_MAX; i++) {
> +/* string compare, and set ptrs *atomically*. */
Minor, capitalize String above for comment.
Other than that LGTM.
Ian
> +if (strcmp(action_impls[i].name, name) == 0) {
> +action_impl_copy_funcs(active, _impls[i]);
> +active_action_impl_index = i;
> +return 0;
> +}
> +}
> +return -1;
> +}
> +
> void
> odp_execute_action_init(void)
> {
> diff --git a/lib/odp-execute.c b/lib/odp-execute.c
> index 49dfa2a74..ab051aecc 100644
> --- a/lib/odp-execute.c
> +++ b/lib/odp-execute.c
> @@ -831,6 +831,28 @@ requires_datapath_assistance(const struct nlattr *a)
> return false;
> }
>
> +static void
> +action_pop_vlan(void *dp OVS_UNUSED, struct dp_packet_batch *batch,
> +const struct nlattr *a OVS_UNUSED,
> +bool should_steal OVS_UNUSED)
> +{
> +struct dp_packet *packet;
> +DP_PACKET_BATCH_FOR_EACH (i, packet, batch) {
> +eth_pop_vlan(packet);
> +}
> +}
> +
> +/* Implementation of the scalar actions impl init function. Build up the
> + * array of func ptrs here.
> + */
> +int32_t
> +odp_action_scalar_init(struct odp_execute_action_impl *self)
> +{
> +self->funcs[OVS_ACTION_ATTR_POP_VLAN] = action_pop_vlan;
> +
> +return 0;
> +}
> +
> /* The active function pointers on the datapath. ISA optimized
> implementations
> * are enabled by plugging them into this static arary, which is consulted
> when
> * applying actions on the datapath.
> @@ -843,10 +865,22 @@ odp_execute_init(void)
> static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
> if (ovsthread_once_start()) {
> odp_execute_action_init();
> +odp_actions_impl_set("scalar");
> ovsthread_once_done();
> }
> }
>
> +int32_t
> +odp_actions_impl_set(const char *name)
> +{
> +
> +int err = odp_execute_action_set(name, _active_impl);
> +if (err) {
> +VLOG_ERR("error %d from action set to %s\n", err, name);
> +return -1;
> +}
> +return 0;
> +}
>
> /* Executes all of the 'actions_len' bytes of datapath actions in 'actions'
> on
> * the packets in 'batch'. If 'steal' is true, possibly modifies and
> @@ -962,12 +996,6 @@ odp_execute_actions(void *dp, struct
> dp_packet_batch *batch, bool steal,
> break;
> }
>
> -case OVS_ACTION_ATTR_POP_VLAN:
> -DP_PACKET_BATCH_FOR_EACH (i, packet, batch) {
> -eth_pop_vlan(packet);
> -}
> -break;
> -
> case OVS_ACTION_ATTR_PUSH_MPLS: {
> const struct ovs_action_push_mpls *mpls = nl_attr_get(a);
>
> @@ -1100,6 +1128,7 @@ odp_execute_actions(void *dp, struct
> dp_packet_batch *batch, bool steal,
> }
> case OVS_ACTION_ATTR_OUTPUT:
> case OVS_ACTION_ATTR_LB_OUTPUT:
> +case OVS_ACTION_ATTR_POP_VLAN:
> case OVS_ACTION_ATTR_TUNNEL_PUSH:
> case OVS_ACTION_ATTR_TUNNEL_POP:
> case OVS_ACTION_ATTR_USERSPACE:
> diff --git a/lib/odp-execute.h b/lib/odp-execute.h
> index c4f5303e7..6441392b9 100644
> --- a/lib/odp-execute.h
> +++ b/lib/odp-execute.h
> @@ -32,6 +32,8 @@ struct dp_packet_batch;
> /* Called once at
Re: [ovs-dev] [PATCH v5 1/8] odp-execute: Add function pointers to odp-execute for different action implementations.
> -Original Message-
> From: Finn, Emma
> Sent: Wednesday, January 12, 2022 9:43 AM
> To: d...@openvswitch.org; Van Haaren, Harry ;
> Amber, Kumar ; Stokes, Ian ;
> i.maxim...@ovn.org
> Cc: Finn, Emma
> Subject: [PATCH v5 1/8] odp-execute: Add function pointers to odp-execute for
> different action implementations.
>
> This commit introduces the initial infrastructure required to allow
> different implementations for OvS actions. The patch introduces action
> function pointers which allows user to switch between different action
> implementations available. This will allow for more performance and
> flexibility
> so the user can choose the action implementation to best suite their use case.
>
> Signed-off-by: Emma Finn
> Acked-by: Harry van Haaren
Thanks for the patch Emma, few minor comments below.
> ---
> lib/automake.mk | 2 +
> lib/dpif-netdev.c | 2 +
> lib/odp-execute-private.c | 84 +
> lib/odp-execute-private.h | 98 +++
> lib/odp-execute.c | 39 ++--
> lib/odp-execute.h | 4 ++
> 6 files changed, 224 insertions(+), 5 deletions(-)
> create mode 100644 lib/odp-execute-private.c
> create mode 100644 lib/odp-execute-private.h
>
> diff --git a/lib/automake.mk b/lib/automake.mk
> index 5224e0856..1bc855a6b 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -203,6 +203,8 @@ lib_libopenvswitch_la_SOURCES = \
> lib/nx-match.h \
> lib/object-collection.c \
> lib/object-collection.h \
> + lib/odp-execute-private.c \
> + lib/odp-execute-private.h \
These two should be placed after odp-execute.h below I would say (Similar to
how vport.c and vport-private.c were added in order above this section).
> lib/odp-execute.c \
> lib/odp-execute.h \
> lib/odp-util.c \
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 649c700cb..eada4fcd7 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -1618,6 +1618,8 @@ create_dpif_netdev(struct dp_netdev *dp)
> dpif->dp = dp;
> dpif->last_port_seq = seq_read(dp->port_seq);
>
> +odp_execute_init();
Single line comment above this explaining its purpose would be nice to see.
> +
> return >dpif;
> }
>
> diff --git a/lib/odp-execute-private.c b/lib/odp-execute-private.c
> new file mode 100644
> index 0..6441c491c
> --- /dev/null
> +++ b/lib/odp-execute-private.c
> @@ -0,0 +1,84 @@
> +/*
> + * Copyright (c) 2021 Intel.
> + *
Not sure if we need to update above to 2022 as that would be the year of
targeted merge to the code base? @Ilya any thoughts here?
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include
> +#include
> +#include
> +#include
Above should be added in alphabetical order.
> +#include "dpdk.h"
This should be added with the block below (again alphabetical order)
> +
> +#include "openvswitch/vlog.h"
> +#include "odp-execute-private.h"
> +#include "odp-netlink.h"
> +#include "dp-packet.h"
> +#include "odp-util.h"
> +
> +
> +int32_t action_autoval_init(struct odp_execute_action_impl *self);
> +VLOG_DEFINE_THIS_MODULE(odp_execute_private);
> +
> +static struct odp_execute_action_impl action_impls[] = {
> +[ACTION_IMPL_SCALAR] = {
> +.available = 1,
> +.name = "scalar",
> +.probe = NULL,
> +.init_func = NULL,
> +},
> +};
> +
> +static void
> +action_impl_copy_funcs(struct odp_execute_action_impl *to,
> + const struct odp_execute_action_impl *from)
> +{
> +for (uint32_t i = 0; i < __OVS_KEY_ATTR_MAX; i++) {
> +atomic_uintptr_t *func = (void *) >funcs[i];
> +atomic_store_relaxed(func, (uintptr_t) from->funcs[i]);
> +}
> +}
> +
> +void
> +odp_execute_action_init(void)
> +{
> +/* Call probe on each impl, and cache the result. */
> +for (int i = 0; i < ACTION_IMPL_MAX; i++) {
> +bool avail = true;
> +if (action_impls[i].probe) {
> +/* Return zero is success, non-zero means error. */
I found this comment is a bit misleading.
You say return 0 on success, yet looking at the comments for typedef int
(*odp_execute_action_probe)(void); It says
/* Probe function is used to detect if this CPU has the ISA required
* to run the optimized action implementation.
* returns one on successful probe.
* returns negative errno on failure.
*/
So is it not more accurate to
Re: [ovs-dev] [PATCH ovn] northd: fix IPv6-PD with northd IP rework
On Wed, Jan 12, 2022 at 8:44 AM Lorenzo Bianconi
wrote:
>
> > On Tue, Jan 11, 2022 at 2:19 PM Lorenzo Bianconi
> > wrote:
> > >
> > > Since commit 4597317f1 ("Introduce incremental processing for northd"),
> > > we rely on (partial) IP for ovn-northd. We need to track SB port_binding
> > > option column in order to notify the CMS whenever the controller
> > > receives the IPv6 prefix from the server.
> > >
> > > Fixes: 4597317f1 ("Introduce incremental processing for northd")
> > > Signed-off-by: Lorenzo Bianconi
> >
> > Hi Lorenzo,
> >
> > The fix makes sense to me. Do you think it's straightforward to add a
> > test case for this ?
> > If so, I'd suggest adding one.
>
> Hi Numan,
>
> we already have one full-test in system-ovn.at. I posted a patch to replace
> dibbler (no longer maintained) with dhcpd so we can enable it by default in
> github repo.
>
I forgot that Ipv6 PD can't be tested with unit tests.
Thanks
Numan
> Regards,
> Lorenzo
>
> >
> > Thanks
> > Numan
> >
> > > ---
> > > northd/ovn-northd.c | 3 ++-
> > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > > index 2b58bfcec..793135ede 100644
> > > --- a/northd/ovn-northd.c
> > > +++ b/northd/ovn-northd.c
> > > @@ -786,7 +786,8 @@ main(int argc, char *argv[])
> > > _port_binding_col_parent_port);
> > > add_column_noalert(ovnsb_idl_loop.idl, _port_binding_col_tag);
> > > add_column_noalert(ovnsb_idl_loop.idl, _port_binding_col_type);
> > > -add_column_noalert(ovnsb_idl_loop.idl,
> > > _port_binding_col_options);
> > > +ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> > > + _port_binding_col_options);
> > > add_column_noalert(ovnsb_idl_loop.idl, _port_binding_col_mac);
> > > add_column_noalert(ovnsb_idl_loop.idl,
> > > _port_binding_col_nat_addresses);
> > > --
> > > 2.34.1
> > >
> > > ___
> > > dev mailing list
> > > d...@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >
> >
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] netdev-dpdk: Refactor the DPDK transmit path.
Hello Sunil, Marko and Ian.
Mike worked to identify the reason for the performance issue
reported by you a while ago. He summarized below. I wonder
if you can give a try on his patch too and tell us if we are
on the right track.
Thanks,
fbl
On Wed, Jan 05, 2022 at 03:01:47PM -0500, Mike Pattrick wrote:
> Hello Flavio,
>
> Great patch, I think you really did a lot to improve the code here and
> I think that's borne out by the consistent performance improvements
> across multiple tests.
>
> Regarding the 4% regression that Intel detected, I found the following
> white paper to describe the "scatter" test:
>
> https://builders.intel.com/docs/networkbuilders/open-vswitch-optimized-deployment-benchmark-technology-guide.pdf
>
> This document calls out the following key points:
>
> The original test was summarized as:
> - 32 VMs with one million flows.
> - Test runs on four physical cores for OVS and 10 hyper-threaded cores
> for TestPMD
> - An Ixia pitches traffic at a sub 0.1% loss rate
> - The server catches traffic with a E810-C 100G
> - The traffic's profile is: Ether()/IP()/UDP()/VXLAN()/Ether()/IP()
> - On the outer IP, the source address changes incrementally across the
> 32 instances
> - The destination address remains the same on the outer IP.
> - The inner source IP remains
> - The inner destination address increments to create the one million
> flows for the test
> - EMC and SMC were disabled
>
> I could not reproduce this test exactly because I don't have access to
> the same hardware - notably the Intel NIC and an Ixia - and I didn't
> want to create an environment that wouldn't be reproduced in real world
> scenarios. I did pin VM and TXQs/RXQs threads to cores, but I didn't
> optimize the setup nearly to the extant that the white paper described.
> My test setup consisted of two Fedora 35 servers directly connected
> across Mellanox5E cards with Trex pitching traffic and TestPMD
> reflecting it.
>
> In my test I was still able to reproduce a similar performance penalty.
> I found that the key factors was the combination of VXLAN and a large
> number of flows. So once I had a setup that could reproduce close to
> the 4% penalty I stopped modifying my test framework and started
> searching for the slow code.
>
> I didn't see any obvious issues in the code that should cause a
> significant slowdown, in fact, most of the code is identical or
> slightly improved. So to help my analysis, I created several variations
> of your patch reverting small aspects of the change and benchmarked
> each variation.
>
> Because the difference in performance across each variation was so
> minor, I took a lot of samples. I pitched traffic over one million
> flows for 240 seconds and averaged out the throughput, I then repeated
> this process a total of five times for each patch. Finally, I repeated
> the whole process three times to produce 15 data points per patch.
>
> The best results came from the patch enclosed below, with the code from
> netdev_dpdk_common_send() protected by the splinlock, as it is in the
> pre-patch code. This yielded a 2.7% +/- 0.64 performance boost over the
> master branch.
>
>
> Cheers,
> Michael
>
>
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index bc1633663..5db5d7e2a 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -2777,13 +2777,13 @@ netdev_dpdk_vhost_send(struct netdev *netdev, int qid,
> return 0;
> }
>
> -cnt = netdev_dpdk_common_send(netdev, batch, );
> -
> if (OVS_UNLIKELY(!rte_spinlock_trylock(>tx_q[qid].tx_lock))) {
> COVERAGE_INC(vhost_tx_contention);
> rte_spinlock_lock(>tx_q[qid].tx_lock);
> }
>
> +cnt = netdev_dpdk_common_send(netdev, batch, );
> +
> pkts = (struct rte_mbuf **) batch->packets;
> vhost_batch_cnt = cnt;
> retries = 0;
> @@ -2843,13 +2843,15 @@ netdev_dpdk_eth_send(struct netdev *netdev, int qid,
> return 0;
> }
>
> -cnt = netdev_dpdk_common_send(netdev, batch, );
> -dropped = batch_cnt - cnt;
> if (OVS_UNLIKELY(concurrent_txq)) {
> qid = qid % dev->up.n_txq;
> rte_spinlock_lock(>tx_q[qid].tx_lock);
> }
>
> +cnt = netdev_dpdk_common_send(netdev, batch, );
> +
> +dropped = batch_cnt - cnt;
> +
> dropped += netdev_dpdk_eth_tx_burst(dev, qid, pkts, cnt);
> if (OVS_UNLIKELY(dropped)) {
> struct netdev_dpdk_sw_stats *sw_stats = dev->sw_stats;
>
>
> On Sun, 2021-01-10 at 00:05 -0300, Flavio Leitner wrote:
> > This patch split out the common code between vhost and
> > dpdk transmit paths to shared functions to simplify the
> > code and fix an issue.
> >
> > The issue is that the packet coming from non-DPDK device
> > and egressing on a DPDK device currently skips the hwol
> > preparation.
> >
> > This also have the side effect of leaving only the dpdk
> > transmit code under the txq lock.
> >
> > Signed-off-by: Flavio Leitner
> > Reviewed-by: David Marchand
> > ---
> >
Re: [ovs-dev] [PATCH ovn] northd: fix IPv6-PD with northd IP rework
> On Tue, Jan 11, 2022 at 2:19 PM Lorenzo Bianconi
> wrote:
> >
> > Since commit 4597317f1 ("Introduce incremental processing for northd"),
> > we rely on (partial) IP for ovn-northd. We need to track SB port_binding
> > option column in order to notify the CMS whenever the controller
> > receives the IPv6 prefix from the server.
> >
> > Fixes: 4597317f1 ("Introduce incremental processing for northd")
> > Signed-off-by: Lorenzo Bianconi
>
> Hi Lorenzo,
>
> The fix makes sense to me. Do you think it's straightforward to add a
> test case for this ?
> If so, I'd suggest adding one.
Hi Numan,
we already have one full-test in system-ovn.at. I posted a patch to replace
dibbler (no longer maintained) with dhcpd so we can enable it by default in
github repo.
Regards,
Lorenzo
>
> Thanks
> Numan
>
> > ---
> > northd/ovn-northd.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > index 2b58bfcec..793135ede 100644
> > --- a/northd/ovn-northd.c
> > +++ b/northd/ovn-northd.c
> > @@ -786,7 +786,8 @@ main(int argc, char *argv[])
> > _port_binding_col_parent_port);
> > add_column_noalert(ovnsb_idl_loop.idl, _port_binding_col_tag);
> > add_column_noalert(ovnsb_idl_loop.idl, _port_binding_col_type);
> > -add_column_noalert(ovnsb_idl_loop.idl,
> > _port_binding_col_options);
> > +ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
> > + _port_binding_col_options);
> > add_column_noalert(ovnsb_idl_loop.idl, _port_binding_col_mac);
> > add_column_noalert(ovnsb_idl_loop.idl,
> > _port_binding_col_nat_addresses);
> > --
> > 2.34.1
> >
> > ___
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn] test: replace dibbler with dhcpd
Replace dibbler dhcp6 server with dhcpd since the former is no longer
maintained.
Signed-off-by: Lorenzo Bianconi
---
tests/atlocal.in| 4 ++--
tests/system-ovn.at | 34 +++---
2 files changed, 13 insertions(+), 25 deletions(-)
diff --git a/tests/atlocal.in b/tests/atlocal.in
index 310fd46a5..eb80cbd1b 100644
--- a/tests/atlocal.in
+++ b/tests/atlocal.in
@@ -181,8 +181,8 @@ else
DIFF_SUPPORTS_NORMAL_FORMAT=no
fi
-# Set HAVE_DIBBLER-SERVER
-find_command dibbler-server
+# Set HAVE_DHCPD
+find_command dhcpd
# Set HAVE_BFDD_BEACON
find_command bfdd-beacon
diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index 7f6cb32dc..4c95d9336 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -4915,7 +4915,7 @@ AT_CLEANUP
OVN_FOR_EACH_NORTHD([
AT_SETUP([IPv6 prefix delegation])
-AT_SKIP_IF([test $HAVE_DIBBLER_SERVER = no])
+AT_SKIP_IF([test $HAVE_DHCPD = no])
AT_SKIP_IF([test $HAVE_TCPDUMP = no])
AT_KEYWORDS([ovn-ipv6-prefix_d])
@@ -4989,28 +4989,16 @@ ovn-nbctl set logical_router_port rp-public
options:prefix=true
ovn-nbctl set logical_router_port rp-sw0 options:prefix=true
ovn-nbctl set logical_router_port rp-sw1 options:prefix=true
-# reset dibbler state
-sed s/^iface.*/"iface \"s1\" {"/g -i /etc/dibbler/server.conf
-sed s/pd-pool.*/"pd-pool 2001:1db8:::\/80"/g -i /etc/dibbler/server.conf
-sed s/t1.*/"t1 10"/g -i /etc/dibbler/server.conf
-sed s/t2.*/"t2 15"/g -i /etc/dibbler/server.conf
-cat > /var/lib/dibbler/server-AddrMgr.xml <
- 1575481348
- 0
-
-EOF
-cat > /var/lib/dibbler/server-CfgMgr.xml <
- /var/lib/dibbler
- Server
- 8
- 0
- 0
-
+cat > dhcpd6.conf < dibbler.log &])
+NS_CHECK_EXEC([server], [dhcpd -6 -cf ./dhcpd6.conf s1 > dhcpd.log &])
ovn-nbctl --wait=hv sync
OVS_WAIT_WHILE([test "$(ovn-nbctl get logical_router_port rp-public
ipv6_prefix | cut -c4-15)" = ""])
@@ -5034,7 +5022,7 @@ ovn-nbctl set logical_router_port rp-sw1
options:prefix=false
# Renew message
NS_CHECK_EXEC([server], [tcpdump -c 1 -nni s1 ip6[[48:1]]=0x05 and
ip6[[113:4]]=0x${prefix} > renew.pcap &])
# Reply message with Status OK
-NS_CHECK_EXEC([server], [tcpdump -c 1 -nni s1 ip6[[48:1]]=0x07 and
ip6[[81:4]]=0x${prefix} and ip6[[98:1]]=0x0d and ip6[[101:2]]=0x >
reply.pcap &])
+NS_CHECK_EXEC([server], [tcpdump -c 1 -nni s1 ip6[[48:1]]=0x07 and
ip6[[81:4]]=0x${prefix} > reply.pcap &])
OVS_WAIT_UNTIL([
total_pkts=$(cat renew.pcap | wc -l)
@@ -5046,7 +5034,7 @@ OVS_WAIT_UNTIL([
test "${total_pkts}" = "1"
])
-kill $(pidof dibbler-server)
+kill $(pidof dhcpd)
kill $(pidof tcpdump)
ovn-nbctl set logical_router_port rp-sw0 options:prefix=false
--
2.34.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1 05/18] build-aux: generate ofp field decoders
On 12/17/21 15:37, Eelco Chaudron wrote:
On 22 Nov 2021, at 12:22, Adrian Moreno wrote:
Based on meta-field information extracted by extract_ofp_fields,
autogenerate the right decoder to be used
.
Signed-off-by: Adrian Moreno
---
build-aux/automake.mk| 3 +-
build-aux/gen_ofp_field_decoders | 73
python/.gitignore| 1 +
python/automake.mk | 7 +++
4 files changed, 83 insertions(+), 1 deletion(-)
create mode 100755 build-aux/gen_ofp_field_decoders
diff --git a/build-aux/automake.mk b/build-aux/automake.mk
index 6267ccd7c..a8bb0acfd 100644
--- a/build-aux/automake.mk
+++ b/build-aux/automake.mk
@@ -9,7 +9,8 @@ EXTRA_DIST += \
build-aux/sodepends.py \
build-aux/soexpand.py \
build-aux/text2c \
- build-aux/xml2nroff
+ build-aux/xml2nroff \
+ build-aux/gen_ofp_field_decoders
FLAKE8_PYFILES += \
$(srcdir)/build-aux/xml2nroff \
diff --git a/build-aux/gen_ofp_field_decoders b/build-aux/gen_ofp_field_decoders
new file mode 100755
index 0..e60410af8
--- /dev/null
+++ b/build-aux/gen_ofp_field_decoders
@@ -0,0 +1,73 @@
+#!/bin/env python
+
+import argparse
+import re
+import os
+import sys
+import importlib
Got the following errors:
build-aux/gen_ofp_field_decoders:4:1: F401 're' imported but unused
build-aux/gen_ofp_field_decoders:5:1: F401 'os' imported but unused
build-aux/gen_ofp_field_decoders:6:1: F401 'sys' imported but unused
build-aux/gen_ofp_field_decoders:7:1: F401 'importlib' imported but unused
The rest of the files looks good to me!
//Eelco
Will remove them, thanks.
+
+import build.extract_ofp_fields as extract_fields
+
+
+def main():
+parser = argparse.ArgumentParser(
+description="Tool to generate python ofproto field decoders from"
+"meta-flow information"
+)
+parser.add_argument(
+"metaflow",
+metavar="FILE",
+type=str,
+help="Read meta-flow info from file",
+)
+
+args = parser.parse_args()
+
+fields = extract_fields.extract_ofp_fields(args.metaflow)
+
+field_decoders = {}
+for field in fields:
+decoder = get_decoder(field)
+field_decoders[field.get("name")] = decoder
+if field.get("extra_name"):
+field_decoders[field.get("extra_name")] = decoder
+
+code = """
+# This file is auto-generated. Do not edit
+
+import functools
+from ovs.flows import decoders
+
+field_decoders = {{
+{decoders}
+}}
+""".format(
+decoders="\n".join(
+[
+"'{name}': {decoder},".format(name=name, decoder=decoder)
+for name, decoder in field_decoders.items()
+]
+)
+)
+print(code)
+
+
+def get_decoder(field):
+formatting = field.get("formatting")
+if formatting in ["decimal", "hexadecimal"]:
+if field.get("mask") == "MFM_NONE":
+return "decoders.decode_int"
+else:
+if field.get("n_bits") in [8, 16, 32, 64, 128, 992]:
+return "decoders.Mask{}".format(field.get("n_bits"))
+return "decoders.decode_mask({})".format(field.get("n_bits"))
+elif formatting in ["IPv4", "IPv6"]:
+return "decoders.IPMask"
+elif formatting == "Ethernet":
+return "decoders.EthMask"
+else:
+return "decoders.decode_default"
+
+
+if __name__ == "__main__":
+main()
diff --git a/python/.gitignore b/python/.gitignore
index 60ace6f05..c8ffd4574 100644
--- a/python/.gitignore
+++ b/python/.gitignore
@@ -1,2 +1,3 @@
dist/
*.egg-info
+ovs/flows/ofp_fields.py
diff --git a/python/automake.mk b/python/automake.mk
index b869eb355..9dfc62fce 100644
--- a/python/automake.mk
+++ b/python/automake.mk
@@ -123,3 +123,10 @@ $(srcdir)/python/ovs/dirs.py: python/ovs/dirs.py.template
mv $@.tmp $@
EXTRA_DIST += python/ovs/dirs.py.template
CLEANFILES += python/ovs/dirs.py
+
+$(srcdir)/python/ovs/flows/ofp_fields.py:
$(srcdir)/build-aux/gen_ofp_field_decoders include/openvswitch/meta-flow.h
+ $(AM_V_GEN)$(run_python) $< $(srcdir)/include/openvswitch/meta-flow.h >
$@.tmp
+ $(AM_V_at)mv $@.tmp $@
+EXTRA_DIST += python/ovs/flows/ofp_fields.py
+CLEANFILES += python/ovs/flows/ofp_fields.py
+
--
2.31.1
--
Adrián Moreno
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dpif-netdev: improve loading of packet data for undersized packets
On 1/6/22 12:45, Harry van Haaren wrote:
> This commit improves handling of packets where the allocated memory
> is less than 64 bytes. In the DPDK datapath this never matters, as
> an mbuf always pre-allocates enough space, however this can occur in
> test environments such as the dummy netdev.
This statement is not correct. Few reasons:
1. Nitpick: there is no such thing as 'DPDK datapath'.
2. The issue is easily reproducible in production environments, i.e.
it's not test-only. The reason for that is netdev-linux and other
ports which are present in every OVS setup (at least the bridge port
in userspace datapath is a tap interface). In a vast majority of
setups these ports are actually up and has ip addresses. E.g.
OpenStack is using tap/veth interfaces for DHCP and other stuff.
And locally delivered packets (packets that never left the hypervisor)
are not obliged to be padded up to 64 bytes. You may find that
local ARP packets, for example, are typically 42 bytes long and that
triggers the memory over-read in our case.
> The fix is required to
> ensure ASAN enabled builds don't error on testing this, hence the
> fix is valuable.
>
> The solution implemented uses a mask-to-zero if the available buffer
> size is less than 64 bytes, and a branch for which type of load is used.
>
> Fixes: 250ceddcc2d0 ("dpif-netdev/mfex: Add AVX512 based optimized miniflow
> extract")
>
> Reported-by: Ilya Maximets
> Signed-off-by: Harry van Haaren
> ---
> lib/dpif-netdev-extract-avx512.c | 9 -
> 1 file changed, 8 insertions(+), 1 deletion(-)
Thanks, Harry and Eelco. The change itself looks good to me and it
fixes ASAN errors while running a few tests under SDE. So, I fixed
the commit message and applied the patch. Also backported to 2.16.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1 04/18] build-aux: split extract-ofp-fields
On 12/17/21 14:53, Eelco Chaudron wrote:
On 22 Nov 2021, at 12:22, Adrian Moreno wrote:
In order to be able to reuse the core extaction logic, split the command
extaction -> extraction
in two parts. The core extraction logic is moved to python/build while
the command that writes the different files out of the extracted field
info is kept in build-aux
Add dot
Signed-off-by: Adrian Moreno
---
build-aux/extract-ofp-fields | 393 +
python/automake.mk | 3 +-
python/build/extract_ofp_fields.py | 386
3 files changed, 397 insertions(+), 385 deletions(-)
create mode 100644 python/build/extract_ofp_fields.py
diff --git a/build-aux/extract-ofp-fields b/build-aux/extract-ofp-fields
index 8766995d9..725718336 100755
--- a/build-aux/extract-ofp-fields
+++ b/build-aux/extract-ofp-fields
@@ -7,78 +7,16 @@ import re
The re module is no longer used, so you can remove the import.
Will do.
import xml.dom.minidom
import build.nroff
-line = ""
-
-# Maps from user-friendly version number to its protocol encoding.
-VERSION = {"1.0": 0x01,
- "1.1": 0x02,
- "1.2": 0x03,
- "1.3": 0x04,
- "1.4": 0x05,
- "1.5": 0x06}
-VERSION_REVERSE = dict((v,k) for k, v in VERSION.items())
+from build.extract_ofp_fields import (
+extract_ofp_fields,
+PREREQS,
+OXM_CLASSES,
+VERSION,
+fatal,
+n_errors
+)
-TYPES = {"u8": (1, False),
- "be16": (2, False),
- "be32": (4, False),
- "MAC": (6, False),
- "be64": (8, False),
- "be128":(16, False),
- "tunnelMD": (124, True)}
-
-FORMATTING = {"decimal":("MFS_DECIMAL", 1, 8),
- "hexadecimal":("MFS_HEXADECIMAL", 1, 127),
- "ct state": ("MFS_CT_STATE", 4, 4),
- "Ethernet": ("MFS_ETHERNET", 6, 6),
- "IPv4": ("MFS_IPV4", 4, 4),
- "IPv6": ("MFS_IPV6",16, 16),
- "OpenFlow 1.0 port": ("MFS_OFP_PORT", 2, 2),
- "OpenFlow 1.1+ port": ("MFS_OFP_PORT_OXM", 4, 4),
- "frag": ("MFS_FRAG", 1, 1),
- "tunnel flags": ("MFS_TNL_FLAGS",2, 2),
- "TCP flags": ("MFS_TCP_FLAGS",2, 2),
- "packet type":("MFS_PACKET_TYPE", 4, 4)}
-
-PREREQS = {"none": "MFP_NONE",
- "Ethernet": "MFP_ETHERNET",
- "ARP": "MFP_ARP",
- "VLAN VID": "MFP_VLAN_VID",
- "IPv4": "MFP_IPV4",
- "IPv6": "MFP_IPV6",
- "IPv4/IPv6": "MFP_IP_ANY",
- "NSH": "MFP_NSH",
- "CT": "MFP_CT_VALID",
- "MPLS": "MFP_MPLS",
- "TCP": "MFP_TCP",
- "UDP": "MFP_UDP",
- "SCTP": "MFP_SCTP",
- "ICMPv4": "MFP_ICMPV4",
- "ICMPv6": "MFP_ICMPV6",
- "ND": "MFP_ND",
- "ND solicit": "MFP_ND_SOLICIT",
- "ND advert": "MFP_ND_ADVERT"}
-
-# Maps a name prefix into an (experimenter ID, class) pair, so:
-#
-# - Standard OXM classes are written as (0, )
-#
-# - Experimenter OXM classes are written as (, 0x)
-#
-# If a name matches more than one prefix, the longest one is used.
-OXM_CLASSES = {"NXM_OF_":(0, 0x, 'extension'),
- "NXM_NX_":(0, 0x0001, 'extension'),
- "NXOXM_NSH_": (0x005ad650, 0x, 'extension'),
- "OXM_OF_":(0, 0x8000, 'standard'),
- "OXM_OF_PKT_REG": (0, 0x8001, 'standard'),
- "ONFOXM_ET_": (0x4f4e4600, 0x, 'standard'),
- "ERICOXM_OF_":(0, 0x1000, 'extension'),
-
- # This is the experimenter OXM class for Nicira, which is the
- # one that OVS would be using instead of NXM_OF_ and NXM_NX_
- # if OVS didn't have those grandfathered in. It is currently
- # used only to test support for experimenter OXM, since there
- # are barely any real uses of experimenter OXM in the wild.
- "NXOXM_ET_": (0x2320, 0x, 'extension')}
+VERSION_REVERSE = dict((v,k) for k, v in VERSION.items())
Originally, I kept it as it was, but now that at it, I'll format the entire file
properly.
Space after v, so; dict((v, k) for k, v in VERSION.items())
def oxm_name_to_class(name):
prefix = ''
@@ -95,39 +33,6 @@ def is_standard_oxm(name):
return oxm_class_type == 'standard'
-def decode_version_range(range):
-if range in VERSION:
-return (VERSION[range], VERSION[range])
-elif range.endswith('+'):
-return (VERSION[range[:-1]], max(VERSION.values()))
-else:
-a, b = re.match(r'^([^-]+)-([^-]+)$', range).groups()
-return
Re: [ovs-dev] [PATCH v1 03/18] python: add list parser
On 12/17/21 14:41, Eelco Chaudron wrote:
See some minor comment below.
//Eelco
On 22 Nov 2021, at 12:22, Adrian Moreno wrote:
Some openflow or dpif flows encode their arguments in lists, eg:
"some_action(arg1,arg2,arg3)". In order to decode this in a way that can
be then stored and queried, add ListParser and ListDecoders classes
that parse lists into KeyValue instances.
The ListParser / ListDecoders mechanism is quite similar to KVParser and
KVDecoders. Since the "key" of the different KeyValue objects is now
ommited, it has to be provided by ListDecoders.
For example, take the openflow action "resubmit" that can be written as:
resubmit([port],[table][,ct])
Can be decoded by creating a ListDecoders instance such as:
ListDecoders([
("port", decode_default),
("table", decode_int),
("ct", decode_flag),
])
Naturally, the order of the decoders must be kept.
Signed-off-by: Adrian Moreno
---
python/automake.mk | 3 +-
python/ovs/flows/list.py | 123 +++
2 files changed, 125 insertions(+), 1 deletion(-)
create mode 100644 python/ovs/flows/list.py
diff --git a/python/automake.mk b/python/automake.mk
index 13aa2b4c3..a3265292d 100644
--- a/python/automake.mk
+++ b/python/automake.mk
@@ -44,7 +44,8 @@ ovs_pyfiles = \
python/ovs/winutils.py \
python/ovs/flows/__init__.py \
python/ovs/flows/decoders.py \
- python/ovs/flows/kv.py
+ python/ovs/flows/kv.py \
+ python/ovs/flows/list.py
# These python files are used at build time but not runtime,
# so they are not installed.
diff --git a/python/ovs/flows/list.py b/python/ovs/flows/list.py
new file mode 100644
index 0..d7ad315a6
--- /dev/null
+++ b/python/ovs/flows/list.py
@@ -0,0 +1,123 @@
+import re
+import functools
+
+from ovs.flows.kv import KeyValue, KeyMetadata, ParseError
+from ovs.flows.decoders import decode_default
+
+
+class ListDecoders:
+"""ListDecoders is used by ListParser to decode the elements in the list
Please add dots here, and to the other comments.
+A decoder is a function that accepts a value and returns its decoded
+object
+The list_decoder to be used is determined by index in the list provided to
+ListDecoders is important.
This last sentence does not make sense.
Agree :)
+
+Args:
+decoders (list of tuples): Optional, A list of tuples.
+The first element in the tuple is the keyword associated with the
+value. The second element in the tuple is the decoder function.
+"""
+
+def __init__(self, decoders=None):
+self._decoders = decoders or list()
+
+def decode(self, index, value_str):
+"""Decode the index'th element of the list
+
+Args:
+index (int): the position in the list of the element ot decode
Guess ot, should be to.
+value_str (str): the value string to decode
+"""
+if index < 0 or index >= len(self._decoders):
+return self._default_decoder(index, value_str)
+
+try:
+key = self._decoders[index][0]
+value = self._decoders[index][1](value_str)
+return key, value
+except Exception as e:
+raise ParseError(
+"Failed to decode value_str %s: %s" % (value_str, str(e))
Personally, I try to move away from % formatting and use .format() but not sure
what the general rule for OVS is.
I know there now also is f””, but I’m not using that yet.
I normally also prefer .format(), don't know why this slipped. Thanks for
spotting it.
+)
+
+@staticmethod
+def _default_decoder(index, value):
+key = "elem_{}".format(index)
+return key, decode_default(value)
+
+
+class ListParser:
+"""ListParser parses a list of values and stores them as key-value pairs
+
+It uses a ListDecoders instance to decode each element in the list.
+
+Args:
+decoders (ListDecoders): Optional, the decoders to use
+delims (list): Optional, list of delimiters of the list. Defaults to
+[',']
+"""
+
+def __init__(self, decoders=None, delims=None):
Guess delims=[","] would reflect the default value (and saves the below
definition).
Will change it in the next version.
+self._decoders = decoders or ListDecoders()
+self._keyval = list()
+delims = delims or [","]
+delims.append("$")
+self._regexp = r"({})".format("|".join(delims))
+
+def kv(self):
+return self._keyval
+
+def __iter__(self):
+return iter(self._keyval)
+
+def parse(self, string):
+"""Parse the list in string
+
+Args:
+string (str): the string to parse
+
+Raises:
+ParseError if any parsing error occurs.
+"""
+kpos = 0
+index = 0
+while kpos < len(string) and string[kpos] !=
Re: [ovs-dev] [PATCH] tests: Add de-serialization check to the json string benchmark.
Ilya Maximets writes: > Since we're testing serialization, it also makes sense to test > the opposite operation. Should be useful in the future for > exploring possible optimizations. > > CMD: $ ./tests/ovstest json-string-benchmark > > Signed-off-by: Ilya Maximets > --- Acked-by: Aaron Conole ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4] acinclude: Provide better error info when linking fails with DPDK.
On 1/9/22 10:05, Sunil Pai G wrote: > Currently, on failure to link with DPDK, the configure script provides > an error message to update the PKG_CONFIG_PATH even though the cause of > failure was missing dependencies. Improve the error message to include this > scenario. > > Signed-off-by: Sunil Pai G > --- > v3-> v4: Address comments. > v2-> v3: Fix sentence. > v1-> v2: Improve logging instead of printing contents from config.log > --- > acinclude.m4 | 7 --- > 1 file changed, 4 insertions(+), 3 deletions(-) Applied. Thanks! Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2/2] netdev-dpdk: Expose per rxq/txq basic statistics.
On 12/14/21 14:33, Kevin Traynor wrote:
> On 02/12/2021 21:16, David Marchand wrote:
>> When troubleshooting multiqueue setups, having per queue statistics helps
>> checking packets repartition in rx and tx queues.
>>
>> Per queue statistics are exported by most DPDK drivers (with capability
>> RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS).
>> OVS only filters DPDK statistics, there is nothing to request in DPDK API.
>> So the only change is to extend the filter on xstats.
>>
>> Querying statistics with
>> $ ovs-vsctl get interface dpdk0 statistics |
>> sed -e 's#[{}]##g' -e 's#, #\n#g'
>>
>> and comparing gives:
>> @@ -13,7 +13,12 @@
>> rx_phy_crc_errors=0
>> rx_phy_in_range_len_errors=0
>> rx_phy_symbol_errors=0
>> +rx_q0_bytes=0
>> rx_q0_errors=0
>> +rx_q0_packets=0
>> +rx_q1_bytes=0
>> rx_q1_errors=0
>> +rx_q1_packets=0
>> rx_wqe_errors=0
>> tx_broadcast_packets=0
>> tx_bytes=0
>> @@ -27,3 +32,13 @@
>> tx_pp_rearm_queue_errors=0
>> tx_pp_timestamp_future_errors=0
>> tx_pp_timestamp_past_errors=0
>> +tx_q0_bytes=0
>> +tx_q0_packets=0
>> +tx_q1_bytes=0
>> +tx_q1_packets=0
>> +tx_q2_bytes=0
>> +tx_q2_packets=0
>> +tx_q3_bytes=0
>> +tx_q3_packets=0
>> +tx_q4_bytes=0
>> +tx_q4_packets=0
>>
>> Signed-off-by: David Marchand
>> Reviewed-by: Maxime Coquelin
>
> Tested and working when increasing/decreasing rx/tx queues.
>
> 'rx_q15_bytes=0, rx_q15_errors=0, rx_q15_packets=0'
>
> Acked-by: Kevin Traynor
Thanks, David, Maxime and Kevin! I applied the series.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [ovs-dev v7 2/3] ipf: micro-optimize ipf_ctx_eq
Hi,
Thanks for the reviewing.
Aaron Conole 于2022年1月12日周三 05:37写道：
> Peng He writes:
>
> > by using ipf_list's key instead of first frags' metadata can reduce
> > quite a lot of cache access as by the time calling ipf_ctx_eq, ipf_list
> > is cache-hot.
> >
> > Signed-off-by: Peng He
> > ---
>
> Is there a reason not to just fold this into 1/3? It's strange to
> introduce something and immediately re-write it in the same series.
>
> I don't see a reason not to fold this into 1/3. The performance
> optimization isn't difficult to understand, and doesn't seem awkward.
>
will merge it into the first patch.
>
> > lib/ipf.c | 18 +++---
> > 1 file changed, 11 insertions(+), 7 deletions(-)
> >
> > diff --git a/lib/ipf.c b/lib/ipf.c
> > index 1fd3d8d30..26de7bbcf 100644
> > --- a/lib/ipf.c
> > +++ b/lib/ipf.c
> > @@ -1047,14 +1047,17 @@ ipf_send_frags_in_list(struct ipf *ipf, struct
> ipf_list *ipf_list,
> > }
> >
> > static bool
> > -ipf_ctx_eq(struct ipf_list *ipf_list, struct ipf_ctx *ctx)
> > +ipf_ctx_eq(struct ipf_list *ipf_list, struct ipf_ctx *ctx,
> > + struct dp_packet *pkt)
> > {
> > -struct dp_packet *pkt =
> > -ipf_list->frag_list[ipf_list->last_sent_idx + 1].pkt;
> > -
> > -if (pkt->md.recirc_id != ctx->recirc_id ||
> > +/* NOTE: not using the first pkt's metadata, use ipf_list instead,
> > + * using pkt's metadata causes too much cache miss,
> > + * using ipf_list instead drops ipf_postprocessing cpu usage
> > + * from 4 percent to 2 percent. ^_^.
> > + */
>
> Maybe this note is better in the commit log. We can't actually be sure
> about each system's underlying memory architecture (for example, does
> this hold true even on ARM?).
>
Ok, will put it into the commit log.
>
> > +if (ipf_list->key.recirc_id != ctx->recirc_id ||
> > pkt->md.in_port.odp_port != ctx->in_port.odp_port ||
> > -pkt->md.ct_zone != ctx->zone) {
> > +ipf_list->key.zone != ctx->zone) {
> > return false;
> > }
> > return true;
> > @@ -1077,7 +1080,8 @@ ipf_send_completed_frags(struct ipf *ipf, struct
> dp_packet_batch *pb,
> >
> > LIST_FOR_EACH_SAFE (ipf_list, next, list_node,
> >frag_complete_list) {
> >
> > -if (ctx && !ipf_ctx_eq(ipf_list, ctx)) {
> > +if (ctx && !ipf_ctx_eq(ipf_list, ctx, \
> > +ipf_list->frag_list[ipf_list->last_sent_idx +
> 1].pkt)) {
> > continue;
> > }
>
>
--
hepeng
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] something is wrong with the ovs-actions man page
On 1/11/22 23:54, Nick Bouliane wrote: >> BTW, the up-to-date versions of some of these docs are available in >> html form in the documentation: >> https://docs.openvswitch.org/en/latest/ref/ovs-actions.7/ > > oh, thanks ! any plan to continue generate the pdf/txt format for easy > offline reading? I think, the goal is to convert all/most of the man pages into rST format (a few pages converted right now); with that we can generate .html, .pdf, .txt and .man at the same time. html pages are available in the auto-generated documentation on a website. We will still publish .txt, .man and .pdf pages on a website too at least until all the pages converted (no-one is actually actively working on that, so I don't know if that will ever happen). It's just that they will be always slightly outdated, since it's a manual process. BTW, the full pdf of the most recent documentation (including already converted man pages) is available here: https://docs.openvswitch.org/_/downloads/en/latest/pdf/ You can find above link in the right-bottom corner of any documentation page on a website. There is also epub version, but it doesn't seem to render well. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v8 2/3] conntrack: prefer dst port range during unique tuple search
we...@ucloud.cn writes:
> From: wenxu
>
> Splits the nested loop used to search the unique ports for the
> reverse tuple.
> It affects only the dnat action, giving more precedence to the dnat
> range, similarly to the kernel dp, instead of searching through the
> default ephemeral source range for each destination port.
>
> Signed-off-by: wenxu
> ---
I see you changed the patch a bit (below the initial draft shared
off-list). My personal preference is to keep the function calls, if
possible. Let's see what's other's opinion about it.
-- >8 --
Subject: [PATCH] conntrack: Prefer dst port range during unique tuple search.
This commit splits the nested loop used to search the unique ports for
the reverse tuple.
It affects only the dnat action, giving more precedence to the dnat
range, similarly to the kernel dp, instead of searching through the
default ephemeral source range for each destination port.
Signed-off-by: Paolo Valerio
---
lib/conntrack.c | 49 +
1 file changed, 37 insertions(+), 12 deletions(-)
diff --git a/lib/conntrack.c b/lib/conntrack.c
index 33a1a9295..fec9f288a 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -2389,6 +2389,22 @@ next_addr_in_range_guarded(union ct_addr *curr, union
ct_addr *min,
return exhausted;
}
+static bool
+nat_get_unique_l4(struct conntrack *ct, struct conn *nat_conn,
+ ovs_be16 *port, uint16_t curr, uint16_t min,
+ uint16_t max)
+{
+FOR_EACH_PORT_IN_RANGE(curr, min, max) {
+*port = htons(curr);
+if (!conn_lookup(ct, _conn->rev_key,
+ time_msec(), NULL, NULL)) {
+return true;
+}
+}
+
+return false;
+}
+
/* This function tries to get a unique tuple.
* Every iteration checks that the reverse tuple doesn't
* collide with any existing one.
@@ -2403,9 +2419,11 @@ next_addr_in_range_guarded(union ct_addr *curr, union
ct_addr *min,
*
* In case of DNAT:
*- For each dst IP address in the range (if any).
- *- For each dport in range (if any).
- * - Try to find a source port in the ephemeral range
- * (after testing the port used by the sender).
+ *- For each dport in range (if any) tries to find
+ * an unique tuple.
+ *- Eventually, if the previous attempt fails,
+ * tries to find a source port in the ephemeral
+ * range (after testing the port used by the sender).
*
* If none can be found, return exhaustion to the caller. */
static bool
@@ -2449,15 +2467,22 @@ another_round:
goto next_addr;
}
-FOR_EACH_PORT_IN_RANGE(curr_dport, min_dport, max_dport) {
-nat_conn->rev_key.src.port = htons(curr_dport);
-FOR_EACH_PORT_IN_RANGE(curr_sport, min_sport, max_sport) {
-nat_conn->rev_key.dst.port = htons(curr_sport);
-if (!conn_lookup(ct, _conn->rev_key,
- time_msec(), NULL, NULL)) {
-return true;
-}
-}
+nat_conn->rev_key.src.port = htons(curr_dport);
+nat_conn->rev_key.dst.port = htons(curr_sport);
+
+bool found = false;
+if (nat_info->nat_action & NAT_ACTION_DST_PORT) {
+found = nat_get_unique_l4(ct, nat_conn, _conn->rev_key.src.port,
+ curr_dport, min_dport, max_dport);
+}
+
+if (!found) {
+found = nat_get_unique_l4(ct, nat_conn, _conn->rev_key.dst.port,
+ curr_sport, min_sport, max_sport);
+}
+
+if (found) {
+return true;
}
/* Check if next IP is in range and respin. Otherwise, notify
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v8 3/3] conntrack: limit port clash resolution attempts
Hello wenxu,
I tested a bit more the patch, and it seems to effectively limit the
number of attempts. There is a case with a sufficiently large port range
that will always tries the same ports.
E.g. (incresing the IPs you can reduce the port range):
actions=ct(commit,nat(dst=10.1.1.100-10.1.1.101:80-144)
in this case the source port will never get the chance to resolve the
clash and the only IPs/ports tested would be the ones above.
we...@ucloud.cn writes:
> From: wenxu
>
> In case almost or all available ports are taken, clash resolution can
> take a very long time, resulting in pmd hang in conntrack.
>
> This can happen when many to-be-natted hosts connect to same
> destination:port (e.g. a proxy) and all connections pass the same SNAT.
>
> Pick a random offset in the acceptable range, then try ever smaller
> number of adjacent port numbers, until either the limit is reached or a
> useable port was found. This results in at most 248 attempts
> (128 + 64 + 32 + 16 + 8, i.e. 4 restarts with new search offset)
> instead of 64000+.
>
> And if thenumber of ip address will limit the max attempts and which
> will lead the total attempts under 248.
>
> Signed-off-by: wenxu
> ---
> lib/conntrack.c | 65
> +
> 1 file changed, 56 insertions(+), 9 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 2a5d72a..dae8dd7 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -2426,10 +2426,14 @@ nat_get_unique_tuple(struct conntrack *ct, const
> struct conn *conn,
> union ct_addr min_addr = {0}, max_addr = {0}, curr_addr = {0},
>guard_addr = {0};
> uint32_t hash = nat_range_hash(conn, ct->hash_basis, nat_info);
> +uint16_t min_sport, max_sport, curr_sport, orig_sport;
> bool pat_proto = conn->key.nw_proto == IPPROTO_TCP ||
> conn->key.nw_proto == IPPROTO_UDP;
> +unsigned int attempts, max_attempts, min_attempts;
> uint16_t min_dport, max_dport, curr_dport;
> -uint16_t min_sport, max_sport, curr_sport;
> +uint16_t range_src, range_dst, range_max;
> +uint32_t range_addr;
> +unsigned int i;
>
> min_addr = nat_info->min_addr;
> max_addr = nat_info->max_addr;
> @@ -2441,11 +2445,29 @@ nat_get_unique_tuple(struct conntrack *ct, const
> struct conn *conn,
> * we can stop once we reach it. */
> guard_addr = curr_addr;
>
> -set_sport_range(nat_info, >key, hash, _sport,
> +set_sport_range(nat_info, >key, hash, _sport,
> _sport, _sport);
> set_dport_range(nat_info, >key, hash, _dport,
> _dport, _dport);
>
> +range_src = max_sport - min_sport + 1;
> +range_dst = max_dport - min_dport + 1;
> +range_max = range_src > range_dst ? range_src : range_dst;
> +if (conn->key.dl_type == htons(ETH_TYPE_IP)) {
> +range_addr = ntohl(max_addr.ipv4) - ntohl(min_addr.ipv4) + 1;
> +} else {
> +range_addr = nat_ipv6_addrs_delta(_info->min_addr.ipv6,
> + _info->max_addr.ipv6) + 1;
> +}
> +max_attempts = 128 / range_addr;
> +if (max_attempts < 1) {
> +max_attempts = 1;
> +}
> +min_attempts = 16 / range_addr;
> +if (min_attempts < 2) {
> +min_attempts = 2;
> +}
> +
> another_round:
> store_addr_to_key(_addr, _conn->rev_key,
>nat_info->nat_action);
> @@ -2459,22 +2481,47 @@ another_round:
> goto next_addr;
> }
>
> +curr_sport = orig_sport;
I think that you should restore the dport as well, right?
> +
> +attempts = range_max;
> +if (attempts > max_attempts) {
> +attempts = max_attempts;
> +}
> +
> +another_port_round:
> +i = 0;
> if (nat_info->nat_action & NAT_ACTION_DST_PORT) {
> FOR_EACH_PORT_IN_RANGE(curr_dport, min_dport, max_dport) {
> -nat_conn->rev_key.src.port = htons(curr_dport);
> +if (i++ < attempts) {
> +nat_conn->rev_key.src.port = htons(curr_dport);
> +if (!conn_lookup(ct, _conn->rev_key,
> + time_msec(), NULL, NULL)) {
> +return true;
> +}
> +} else {
> +break;
I don't know if it's really a problem (and maybe you noticed
already), but breaking before you go through the whole range will change
the dport (that is, it will not use the initial destination port) during
the the next clash resolution (based on the source port).
All in all, the patch does its job, but probably the DNAT case above
should be kept in mind.
> +}
> +}
> +}
> +
> +FOR_EACH_PORT_IN_RANGE(curr_sport, min_sport, max_sport) {
> +if (i++ < attempts) {
> +nat_conn->rev_key.dst.port = htons(curr_sport);
> if (!conn_lookup(ct, _conn->rev_key,
> time_msec(), NULL, NULL)) {
>
[ovs-dev] [PATCH v5 8/8] odp-execute: Add ISA implementation of push_vlan action.
This commit adds the AVX512 implementation of the push_vlan action.
The implementation here is auto-validated by the miniflow
extract autovalidator, hence its correctness can be easily
tested and verified.
Signed-off-by: Emma Finn
---
lib/odp-execute-avx512.c | 62 +++
lib/odp-execute-private.c | 1 +
lib/odp-execute.c | 24 +--
3 files changed, 72 insertions(+), 15 deletions(-)
diff --git a/lib/odp-execute-avx512.c b/lib/odp-execute-avx512.c
index fcf27f070..03c0fd446 100644
--- a/lib/odp-execute-avx512.c
+++ b/lib/odp-execute-avx512.c
@@ -43,6 +43,13 @@ static inline void ALWAYS_INLINE
avx512_dp_packet_resize_l2(struct dp_packet *b, int resize_by_bytes)
{
/* update packet size/data pointers */
+if (resize_by_bytes >= 0) {
+dp_packet_prealloc_headroom(b, resize_by_bytes);
+} else {
+ovs_assert(dp_packet_size(b) - dp_packet_l2_pad_size(b) >=
+-resize_by_bytes);
+}
+
dp_packet_set_data(b, (char *) dp_packet_data(b) - resize_by_bytes);
dp_packet_set_size(b, dp_packet_size(b) + resize_by_bytes);
@@ -50,9 +57,9 @@ avx512_dp_packet_resize_l2(struct dp_packet *b, int
resize_by_bytes)
const __m128i v_zeros = _mm_setzero_si128();
const __m128i v_u16_max = _mm_cmpeq_epi16(v_zeros, v_zeros);
-/* Only these lanes can be incremented for push-VLAN action. */
+/* Only these lanes can be incremented/decremented for L2. */
const uint8_t k_lanes = 0b1110;
-__m128i v_offset = _mm_set1_epi16(VLAN_HEADER_LEN);
+__m128i v_offset = _mm_set1_epi16(abs(resize_by_bytes));
/* Load packet and compare with UINT16_MAX */
void *adjust_ptr = >l2_pad_size;
@@ -60,9 +67,17 @@ avx512_dp_packet_resize_l2(struct dp_packet *b, int
resize_by_bytes)
__mmask8 k_cmp = _mm_mask_cmpneq_epu16_mask(k_lanes, v_adjust_src,
v_u16_max);
-/* Add VLAN_HEADER_LEN using compare mask, store results. */
-__m128i v_adjust_wip = _mm_mask_sub_epi16(v_adjust_src, k_cmp,
- v_adjust_src, v_offset);
+/* Update VLAN_HEADER_LEN using compare mask, store results. */
+__m128i v_adjust_wip;
+
+if (resize_by_bytes >= 0) {
+v_adjust_wip = _mm_mask_add_epi16(v_adjust_src, k_cmp,
+v_adjust_src, v_offset);
+} else {
+v_adjust_wip = _mm_mask_sub_epi16(v_adjust_src, k_cmp,
+v_adjust_src, v_offset);
+}
+
_mm_storeu_si128(adjust_ptr, v_adjust_wip);
}
@@ -80,7 +95,6 @@ avx512_eth_pop_vlan(struct dp_packet *packet)
16 - VLAN_HEADER_LEN);
_mm_storeu_si128((void *) veh, v_realign);
avx512_dp_packet_resize_l2(packet, -VLAN_HEADER_LEN);
-
}
}
@@ -96,6 +110,41 @@ action_avx512_pop_vlan(void *dp OVS_UNUSED, struct
dp_packet_batch *batch,
}
}
+static inline void ALWAYS_INLINE
+avx512_eth_push_vlan(struct dp_packet *packet, ovs_be16 tpid, ovs_be16 tci)
+{
+avx512_dp_packet_resize_l2(packet, VLAN_HEADER_LEN);
+
+/* Build up the VLAN TCI/TPID, and merge with the moving of Ether. */
+char *pkt_data = (char *) dp_packet_data(packet);
+const uint16_t tci_proc = tci & htons(~VLAN_CFI);
+const uint32_t tpid_tci = (tci_proc << 16) | tpid;
+
+static const uint8_t vlan_push_shuffle_mask[16] = {
+4, 5, 6, 7, 8, 9, 10, 11,
+12, 13, 14, 15, 0xFF, 0xFF, 0xFF, 0xFF
+};
+
+__m128i v_ether = _mm_loadu_si128((void *) pkt_data);
+__m128i v_index = _mm_loadu_si128((void *) vlan_push_shuffle_mask);
+__m128i v_shift = _mm_shuffle_epi8(v_ether, v_index);
+__m128i v_vlan_hdr = _mm_insert_epi32(v_shift, tpid_tci, 3);
+ _mm_storeu_si128((void *) pkt_data, v_vlan_hdr);
+}
+
+static void
+action_avx512_push_vlan(void *dp OVS_UNUSED, struct dp_packet_batch *batch,
+ const struct nlattr *a,
+ bool should_steal OVS_UNUSED)
+{
+struct dp_packet *packet;
+const struct ovs_action_push_vlan *vlan = nl_attr_get(a);
+
+DP_PACKET_BATCH_FOR_EACH (i, packet, batch) {
+avx512_eth_push_vlan(packet, vlan->vlan_tpid, vlan->vlan_tci);
+}
+}
+
/* Probe functions to check ISA requirements. */
static int32_t
avx512_isa_probe(uint32_t needs_vbmi)
@@ -136,6 +185,7 @@ action_avx512_init(struct odp_execute_action_impl *self)
{
avx512_isa_probe(0);
self->funcs[OVS_ACTION_ATTR_POP_VLAN] = action_avx512_pop_vlan;
+self->funcs[OVS_ACTION_ATTR_PUSH_VLAN] = action_avx512_push_vlan;
return 0;
}
diff --git a/lib/odp-execute-private.c b/lib/odp-execute-private.c
index 175a80159..607f0fa94 100644
--- a/lib/odp-execute-private.c
+++ b/lib/odp-execute-private.c
@@ -218,6 +218,7 @@ int32_t
action_autoval_init(struct odp_execute_action_impl *self)
{
self->funcs[OVS_ACTION_ATTR_POP_VLAN] =
[ovs-dev] [PATCH v5 7/8] odp-execute: Add ISA implementation of pop_vlan action.
This commit adds the AVX512 implementation of the pop_vlan action.
The implementation here is auto-validated by the miniflow
extract autovalidator, hence its correctness can be easily
tested and verified.
Signed-off-by: Emma Finn
---
lib/odp-execute-avx512.c | 77 ++-
lib/odp-execute-private.c | 2 +-
lib/odp-execute-private.h | 2 +-
3 files changed, 78 insertions(+), 3 deletions(-)
diff --git a/lib/odp-execute-avx512.c b/lib/odp-execute-avx512.c
index aa71faa1c..fcf27f070 100644
--- a/lib/odp-execute-avx512.c
+++ b/lib/odp-execute-avx512.c
@@ -14,6 +14,11 @@
* limitations under the License.
*/
+#ifdef __x86_64__
+/* Sparse cannot handle the AVX512 instructions. */
+#if !defined(__CHECKER__)
+
+
#include
#include
@@ -25,6 +30,71 @@
#include "immintrin.h"
+VLOG_DEFINE_THIS_MODULE(odp_execute_avx512);
+BUILD_ASSERT_DECL(offsetof(struct dp_packet, l2_5_ofs) +
+ MEMBER_SIZEOF(struct dp_packet, l2_5_ofs) ==
+ offsetof(struct dp_packet, l3_ofs));
+
+BUILD_ASSERT_DECL(offsetof(struct dp_packet, l3_ofs) +
+ MEMBER_SIZEOF(struct dp_packet, l3_ofs) ==
+ offsetof(struct dp_packet, l4_ofs));
+
+static inline void ALWAYS_INLINE
+avx512_dp_packet_resize_l2(struct dp_packet *b, int resize_by_bytes)
+{
+/* update packet size/data pointers */
+dp_packet_set_data(b, (char *) dp_packet_data(b) - resize_by_bytes);
+dp_packet_set_size(b, dp_packet_size(b) + resize_by_bytes);
+
+/* Increment u16 packet offset values */
+const __m128i v_zeros = _mm_setzero_si128();
+const __m128i v_u16_max = _mm_cmpeq_epi16(v_zeros, v_zeros);
+
+/* Only these lanes can be incremented for push-VLAN action. */
+const uint8_t k_lanes = 0b1110;
+__m128i v_offset = _mm_set1_epi16(VLAN_HEADER_LEN);
+
+/* Load packet and compare with UINT16_MAX */
+void *adjust_ptr = >l2_pad_size;
+__m128i v_adjust_src = _mm_loadu_si128(adjust_ptr);
+__mmask8 k_cmp = _mm_mask_cmpneq_epu16_mask(k_lanes, v_adjust_src,
+v_u16_max);
+
+/* Add VLAN_HEADER_LEN using compare mask, store results. */
+__m128i v_adjust_wip = _mm_mask_sub_epi16(v_adjust_src, k_cmp,
+ v_adjust_src, v_offset);
+_mm_storeu_si128(adjust_ptr, v_adjust_wip);
+
+}
+
+static inline void ALWAYS_INLINE
+avx512_eth_pop_vlan(struct dp_packet *packet)
+{
+struct vlan_eth_header *veh = dp_packet_eth(packet);
+
+if (veh && dp_packet_size(packet) >= sizeof *veh &&
+eth_type_vlan(veh->veth_type)) {
+
+__m128i v_ether = _mm_loadu_si128((void *) veh);
+__m128i v_realign = _mm_alignr_epi8(v_ether, _mm_setzero_si128(),
+16 - VLAN_HEADER_LEN);
+_mm_storeu_si128((void *) veh, v_realign);
+avx512_dp_packet_resize_l2(packet, -VLAN_HEADER_LEN);
+
+}
+}
+
+static void
+action_avx512_pop_vlan(void *dp OVS_UNUSED, struct dp_packet_batch *batch,
+ const struct nlattr *a OVS_UNUSED,
+ bool should_steal OVS_UNUSED)
+{
+struct dp_packet *packet;
+
+DP_PACKET_BATCH_FOR_EACH (i, packet, batch) {
+avx512_eth_pop_vlan(packet);
+}
+}
/* Probe functions to check ISA requirements. */
static int32_t
@@ -62,8 +132,13 @@ action_avx512_probe(void)
int32_t
-action_avx512_init(void)
+action_avx512_init(struct odp_execute_action_impl *self)
{
avx512_isa_probe(0);
+self->funcs[OVS_ACTION_ATTR_POP_VLAN] = action_avx512_pop_vlan;
+
return 0;
}
+
+#endif
+#endif
diff --git a/lib/odp-execute-private.c b/lib/odp-execute-private.c
index e61136e8b..175a80159 100644
--- a/lib/odp-execute-private.c
+++ b/lib/odp-execute-private.c
@@ -52,7 +52,7 @@ static struct odp_execute_action_impl action_impls[] = {
.available = 1,
.name = "avx512",
.probe = action_avx512_probe,
-.init_func = NULL,
+.init_func = action_avx512_init,
},
#endif
};
diff --git a/lib/odp-execute-private.h b/lib/odp-execute-private.h
index 4c09bee63..5ba2868bf 100644
--- a/lib/odp-execute-private.h
+++ b/lib/odp-execute-private.h
@@ -102,7 +102,7 @@ int32_t odp_execute_action_set(const char *name,
int32_t odp_action_scalar_init(struct odp_execute_action_impl *self);
/* Init function for the optimized with AVX512 actions. */
-int32_t action_avx512_init(void);
+int32_t action_avx512_init(struct odp_execute_action_impl *self);
/* Probe function to check ISA requirements. */
int32_t action_avx512_probe(void);
--
2.25.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v5 6/8] odp-execute: Add ISA implementation of actions.
This commit adds the AVX512 implementation of the action functionality. Usage: $ ovs-appctl dpif-netdev/action-impl-set avx512 Signed-off-by: Emma Finn Acked-by: Harry van Haaren --- Documentation/topics/dpdk/bridge.rst | 25 ++ Documentation/topics/testing.rst | 20 +--- NEWS | 1 + lib/automake.mk | 4 +- lib/cpu.c| 1 + lib/cpu.h| 1 + lib/odp-execute-avx512.c | 69 lib/odp-execute-private.c| 9 lib/odp-execute-private.h| 9 9 files changed, 131 insertions(+), 8 deletions(-) create mode 100644 lib/odp-execute-avx512.c diff --git a/Documentation/topics/dpdk/bridge.rst b/Documentation/topics/dpdk/bridge.rst index ceee91015..67089e08f 100644 --- a/Documentation/topics/dpdk/bridge.rst +++ b/Documentation/topics/dpdk/bridge.rst @@ -321,3 +321,28 @@ following command:: ``scalar`` can be selected on core ``3`` by the following command:: $ ovs-appctl dpif-netdev/miniflow-parser-set -pmd 3 scalar + +Actions Performance +--- + +Actions are used in OpenFlow flows to describe what to do when the flow +matches a packet. Just like with the datapath interface, SIMD instructions +can be applied to the action implementation to improve performance. + +OVS provides multiple implementations of the actions. +Available implementations can be listed with the following command:: + +$ ovs-appctl dpif-netdev/action-impl-get +Available Actions implementations: +scalar (available: True, active: True) +autovalidator (available: True, active: False) +avx512 (available: True, active: False) + +By default, ``scalar`` is used. Implementations can be selected by +name:: + +$ ovs-appctl dpif-netdev/action-impl-set avx512 +action implementation set to avx512. + +$ ovs-appctl dpif-netdev/action-impl-set scalar +action implementation set to scalar. diff --git a/Documentation/topics/testing.rst b/Documentation/topics/testing.rst index c15d5b38f..10d0ecc48 100644 --- a/Documentation/topics/testing.rst +++ b/Documentation/topics/testing.rst @@ -361,12 +361,12 @@ testsuite. Userspace datapath: Testing and Validation of CPU-specific Optimizations -As multiple versions of the datapath classifier and packet parsing functions -can co-exist, each with different CPU ISA optimizations, it is important to -validate that they all give the exact same results. To easily test all the -implementations, an ``autovalidator`` implementation of them exists. This -implementation runs all other available implementations, and verifies that the -results are identical. +As multiple versions of the datapath classifier, packet parsing functions and +actions can co-exist, each with different CPU ISA optimizations, it is +important to validate that they all give the exact same results. To easily +test all the implementations, an ``autovalidator`` implementation of them +exists. This implementation runs all other available implementations, and +verifies that the results are identical. Running the OVS unit tests with the autovalidator enabled ensures all implementations provide the same results. Note that the performance of the @@ -382,18 +382,24 @@ To set the autovalidator for the packet parser, use this command:: $ ovs-appctl dpif-netdev/miniflow-parser-set autovalidator +To set the autovalidator for actions, use this command:: + +$ ovs-appctl dpif-netdev/action-impl-set autovalidator + To run the OVS unit test suite with the autovalidator as the default implementation, it is required to recompile OVS. During the recompilation, the default priority of the `autovalidator` implementation is set to the maximum priority, ensuring every test will be run with every implementation:: -$ ./configure --enable-autovalidator --enable-mfex-default-autovalidator +$ ./configure --enable-autovalidator --enable-mfex-default-autovalidator \ +--enable-actions-default-autovalidator The following line should be seen in the configuration log when the above options are used:: checking whether DPCLS Autovalidator is default implementation... yes checking whether MFEX Autovalidator is default implementation... yes +checking whether actions Autovalidator is default implementation... yes Compile OVS in debug mode to have `ovs_assert` statements error out if there is a mis-match in the datapath classifier lookup or packet parser diff --git a/NEWS b/NEWS index 1fd2f7375..72787ccc1 100644 --- a/NEWS +++ b/NEWS @@ -25,6 +25,7 @@ Post-v2.16.0 implementations available at run time. * Add build time configure command to enable auto-validator as default actions implementation at build time. + * Add AVX512 implementation of actions.
[ovs-dev] [PATCH v5 5/8] dpif-netdev: Add configure option to enable actions autovalidator at build time.
From: Kumar Amber
This commit adds a new command to allow the user to enable the
actions autovalidator by default at build time thus allowing for
running unit test by default.
$ ./configure --enable-actions-default-autovalidator
Signed-off-by: Kumar Amber
Acked-by: Harry van Haaren
---
NEWS | 2 ++
acinclude.m4 | 17 +
configure.ac | 1 +
lib/odp-execute.c | 4
4 files changed, 24 insertions(+)
diff --git a/NEWS b/NEWS
index 42bb876da..1fd2f7375 100644
--- a/NEWS
+++ b/NEWS
@@ -23,6 +23,8 @@ Post-v2.16.0
implementations against default implementation.
* Add command line option to switch between different actions
implementations available at run time.
+ * Add build time configure command to enable auto-validator as default
+ actions implementation at build time.
- Python:
* For SSL support, the use of the pyOpenSSL library has been replaced
with the native 'ssl' module.
diff --git a/acinclude.m4 b/acinclude.m4
index 23cd6df44..6514f2bd7 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -14,6 +14,23 @@
# See the License for the specific language governing permissions and
# limitations under the License.
+dnl Set OVS Actions Autovalidator as default action at compile time?
+dnl This enables automatically running all unit tests with all actions
+dnl implementations.
+AC_DEFUN([OVS_CHECK_ACTIONS_AUTOVALIDATOR], [
+ AC_ARG_ENABLE([actions-default-autovalidator],
+[AC_HELP_STRING([--enable-actions-default-autovalidator],
[Enable actions autovalidator as default ovs actions implementation.])],
+[autovalidator=yes],[autovalidator=no])
+ AC_MSG_CHECKING([whether actions Autovalidator is default implementation])
+ if test "$autovalidator" != yes; then
+AC_MSG_RESULT([no])
+ else
+OVS_CFLAGS="$OVS_CFLAGS -DACTIONS_AUTOVALIDATOR_DEFAULT"
+AC_MSG_RESULT([yes])
+ fi
+])
+
+
dnl Set OVS MFEX Autovalidator as default miniflow extract at compile time?
dnl This enables automatically running all unit tests with all MFEX
dnl implementations.
diff --git a/configure.ac b/configure.ac
index eaa9bf7ee..bfd0a9aff 100644
--- a/configure.ac
+++ b/configure.ac
@@ -185,6 +185,7 @@ OVS_CTAGS_IDENTIFIERS
OVS_CHECK_DPCLS_AUTOVALIDATOR
OVS_CHECK_DPIF_AVX512_DEFAULT
OVS_CHECK_MFEX_AUTOVALIDATOR
+OVS_CHECK_ACTIONS_AUTOVALIDATOR
OVS_CHECK_AVX512
AC_ARG_VAR(KARCH, [Kernel Architecture String])
diff --git a/lib/odp-execute.c b/lib/odp-execute.c
index ab051aecc..1bc9fae09 100644
--- a/lib/odp-execute.c
+++ b/lib/odp-execute.c
@@ -865,7 +865,11 @@ odp_execute_init(void)
static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
if (ovsthread_once_start()) {
odp_execute_action_init();
+#ifdef ACTIONS_AUTOVALIDATOR_DEFAULT
+odp_actions_impl_set("autovalidator");
+#else
odp_actions_impl_set("scalar");
+#endif
ovsthread_once_done();
}
}
--
2.25.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v5 4/8] odp-execute: Add command to switch action implementation.
This commit adds a new command to allow the user to switch
the active action implementation at runtime. A probe function
is executed before switching the implementation, to ensure
the CPU is capable of running the ISA required.
Usage:
$ ovs-appctl dpif-netdev/action-impl-set scalar
This commit also adds a new command to retrieve the list of available
action implementations. This can be used by to check what implementations
of actions are available and what implementation is active during runtime.
Usage:
$ ovs-appctl dpif-netdev/action-impl-get
Added separate test-case for ovs-actions get/set commands:
1023: PMD - ovs-actions configuration
Signed-off-by: Emma Finn
Co-authored-by: Kumar Amber
Signed-off-by: Kumar Amber
Acked-by: Harry van Haaren
---
NEWS| 2 ++
lib/dpif-netdev-unixctl.man | 6 ++
lib/dpif-netdev.c | 39 +
lib/odp-execute-private.c | 14 +
lib/odp-execute.h | 3 +++
tests/pmd.at| 21
6 files changed, 85 insertions(+)
diff --git a/NEWS b/NEWS
index 26be454df..42bb876da 100644
--- a/NEWS
+++ b/NEWS
@@ -21,6 +21,8 @@ Post-v2.16.0
* Add support for running threads on cores >= RTE_MAX_LCORE.
* Add actions auto-validator function to compare different actions
implementations against default implementation.
+ * Add command line option to switch between different actions
+ implementations available at run time.
- Python:
* For SSL support, the use of the pyOpenSSL library has been replaced
with the native 'ssl' module.
diff --git a/lib/dpif-netdev-unixctl.man b/lib/dpif-netdev-unixctl.man
index 8cd847416..500daf4de 100644
--- a/lib/dpif-netdev-unixctl.man
+++ b/lib/dpif-netdev-unixctl.man
@@ -262,3 +262,9 @@ PMDs in the case where no value is specified. By default
"scalar" is used.
\fIstudy_cnt\fR defaults to 128 and indicates the number of packets that the
"study" miniflow implementation must parse before choosing an optimal
implementation.
+
+.IP "\fBdpif-netdev/action-impl-get\fR
+Lists the actions implementations that are available.
+.
+.IP "\fBdpif-netdev/action-impl-set\fR \fIaction_impl\fR"
+Sets the action to be used to \fIaction_impl\fR. By default "scalar" is used.
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index eada4fcd7..f6cc779ef 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -60,6 +60,7 @@
#include "netdev-vport.h"
#include "netlink.h"
#include "odp-execute.h"
+#include "odp-execute-private.h"
#include "odp-util.h"
#include "openvswitch/dynamic-string.h"
#include "openvswitch/list.h"
@@ -1330,6 +1331,38 @@ error:
ds_destroy();
}
+static void
+action_impl_set(struct unixctl_conn *conn, int argc OVS_UNUSED,
+const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
+{
+struct ds reply = DS_EMPTY_INITIALIZER;
+
+int32_t err = odp_actions_impl_set(argv[1]);
+if (err) {
+ds_put_format(, "action implementation %s not found.\n",
+ argv[1]);
+const char *reply_str = ds_cstr();
+unixctl_command_reply_error(conn, reply_str);
+VLOG_ERR("%s", reply_str);
+ds_destroy();
+return;
+}
+
+ds_put_format(, "action implementation set to %s.\n", argv[1]);
+unixctl_command_reply(conn, ds_cstr());
+ds_destroy();
+}
+
+static void
+action_impl_get(struct unixctl_conn *conn, int argc OVS_UNUSED,
+const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
+{
+struct ds reply = DS_EMPTY_INITIALIZER;
+odp_execute_action_get();
+unixctl_command_reply(conn, ds_cstr());
+ds_destroy();
+}
+
static void
dpif_netdev_pmd_rebalance(struct unixctl_conn *conn, int argc,
const char *argv[], void *aux OVS_UNUSED)
@@ -1567,6 +1600,12 @@ dpif_netdev_init(void)
unixctl_command_register("dpif-netdev/miniflow-parser-get", "",
0, 0, dpif_miniflow_extract_impl_get,
NULL);
+unixctl_command_register("dpif-netdev/action-impl-set", "name",
+ 1, 1, action_impl_set,
+ NULL);
+unixctl_command_register("dpif-netdev/action-impl-get", "",
+ 0, 0, action_impl_get,
+ NULL);
return 0;
}
diff --git a/lib/odp-execute-private.c b/lib/odp-execute-private.c
index a4155b5df..c17882a33 100644
--- a/lib/odp-execute-private.c
+++ b/lib/odp-execute-private.c
@@ -74,6 +74,20 @@ odp_execute_action_set(const char *name,
return -1;
}
+void
+odp_execute_action_get(struct ds *string)
+{
+uint32_t i;
+
+ds_put_cstr(string, "Available Actions implementations:\n");
+for (i = 0; i < ACTION_IMPL_MAX; i++) {
+ds_put_format(string, " %s (available: %s, active: %s)\n",
+ action_impls[i].name,
+
[ovs-dev] [PATCH v5 3/8] odp-execute: Add auto validation function for actions.
This commit introduced the auto-validation function which
allows users to compare the batch of packets obtained from
different action implementations against the linear
action implementation.
The autovalidator function can be triggered at runtime using the
following command:
$ ovs-appctl dpif-netdev/action-impl-set autovalidator
Signed-off-by: Emma Finn
Acked-by: Harry van Haaren
---
NEWS | 2 +
lib/dp-packet.c | 23 +
lib/dp-packet.h | 5 ++
lib/odp-execute-private.c | 99 +++
lib/odp-execute-private.h | 3 ++
5 files changed, 132 insertions(+)
diff --git a/NEWS b/NEWS
index afef81b40..26be454df 100644
--- a/NEWS
+++ b/NEWS
@@ -19,6 +19,8 @@ Post-v2.16.0
* Add support for DPDK 21.11.
* Forbid use of DPDK multiprocess feature.
* Add support for running threads on cores >= RTE_MAX_LCORE.
+ * Add actions auto-validator function to compare different actions
+ implementations against default implementation.
- Python:
* For SSL support, the use of the pyOpenSSL library has been replaced
with the native 'ssl' module.
diff --git a/lib/dp-packet.c b/lib/dp-packet.c
index 72f6d09ac..1e4ff35ef 100644
--- a/lib/dp-packet.c
+++ b/lib/dp-packet.c
@@ -506,3 +506,26 @@ dp_packet_resize_l2(struct dp_packet *b, int increment)
dp_packet_adjust_layer_offset(>l2_5_ofs, increment);
return dp_packet_data(b);
}
+
+bool
+dp_packet_compare_and_log(struct dp_packet *good, struct dp_packet *test,
+ struct ds *err_str)
+{
+if ((good->l2_pad_size != test->l2_pad_size) ||
+(good->l2_5_ofs != test->l2_5_ofs) ||
+(good->l3_ofs != test->l3_ofs) ||
+(good->l4_ofs != test->l4_ofs)) {
+ds_put_format(err_str, "Autovalidation packet offsets failed"
+ "\n");
+ds_put_format(err_str, "Good offsets: l2_pad_size %u,"
+ " l2_5_ofs : %u l3_ofs %u, l4_ofs %u\n",
+ good->l2_pad_size, good->l2_5_ofs,
+ good->l3_ofs, good->l4_ofs);
+ds_put_format(err_str, "Test offsets: l2_pad_size %u,"
+ " l2_5_ofs : %u l3_ofs %u, l4_ofs %u\n",
+ test->l2_pad_size, test->l2_5_ofs,
+ test->l3_ofs, test->l4_ofs);
+return false;
+}
+return true;
+}
diff --git a/lib/dp-packet.h b/lib/dp-packet.h
index ee0805ae6..723215add 100644
--- a/lib/dp-packet.h
+++ b/lib/dp-packet.h
@@ -236,6 +236,11 @@ void *dp_packet_steal_data(struct dp_packet *);
static inline bool dp_packet_equal(const struct dp_packet *,
const struct dp_packet *);
+
+bool dp_packet_compare_and_log(struct dp_packet *good,
+ struct dp_packet *test,
+ struct ds *err_str);
+
/* Frees memory that 'b' points to, as well as 'b' itself. */
static inline void
diff --git a/lib/odp-execute-private.c b/lib/odp-execute-private.c
index d88ff4921..a4155b5df 100644
--- a/lib/odp-execute-private.c
+++ b/lib/odp-execute-private.c
@@ -30,8 +30,16 @@
int32_t action_autoval_init(struct odp_execute_action_impl *self);
VLOG_DEFINE_THIS_MODULE(odp_execute_private);
static uint32_t active_action_impl_index;
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
static struct odp_execute_action_impl action_impls[] = {
+[ACTION_IMPL_AUTOVALIDATOR] = {
+.available = 1,
+.name = "autovalidator",
+.probe = NULL,
+.init_func = action_autoval_init,
+},
+
[ACTION_IMPL_SCALAR] = {
.available = 1,
.name = "scalar",
@@ -99,3 +107,94 @@ odp_execute_action_init(void)
}
}
}
+
+/* Init sequence required to be scalar first to pick up the default scalar
+* implementations, allowing over-riding of the optimized functions later.
+*/
+BUILD_ASSERT_DECL(ACTION_IMPL_SCALAR == 0);
+BUILD_ASSERT_DECL(ACTION_IMPL_AUTOVALIDATOR == 1);
+
+/* Loop over packets, and validate each one for the given action. */
+static void
+action_autoval_generic(void *dp OVS_UNUSED, struct dp_packet_batch *batch,
+ const struct nlattr *a, bool should_steal)
+{
+uint32_t failed = 0;
+
+int type = nl_attr_type(a);
+enum ovs_action_attr attr_type = (enum ovs_action_attr) type;
+
+struct odp_execute_action_impl *scalar = _impls[ACTION_IMPL_SCALAR];
+
+struct dp_packet_batch good_batch;
+dp_packet_batch_clone(_batch, batch);
+
+scalar->funcs[attr_type](NULL, _batch, a, should_steal);
+
+for (uint32_t impl = ACTION_IMPL_BEGIN; impl < ACTION_IMPL_MAX; impl++) {
+/* Clone original batch and execute implementation under test. */
+struct dp_packet_batch test_batch;
+dp_packet_batch_clone(_batch, batch);
+action_impls[impl].funcs[attr_type](NULL, _batch, a,
+
[ovs-dev] [PATCH v5 1/8] odp-execute: Add function pointers to odp-execute for different action implementations.
This commit introduces the initial infrastructure required to allow
different implementations for OvS actions. The patch introduces action
function pointers which allows user to switch between different action
implementations available. This will allow for more performance and flexibility
so the user can choose the action implementation to best suite their use case.
Signed-off-by: Emma Finn
Acked-by: Harry van Haaren
---
lib/automake.mk | 2 +
lib/dpif-netdev.c | 2 +
lib/odp-execute-private.c | 84 +
lib/odp-execute-private.h | 98 +++
lib/odp-execute.c | 39 ++--
lib/odp-execute.h | 4 ++
6 files changed, 224 insertions(+), 5 deletions(-)
create mode 100644 lib/odp-execute-private.c
create mode 100644 lib/odp-execute-private.h
diff --git a/lib/automake.mk b/lib/automake.mk
index 5224e0856..1bc855a6b 100644
--- a/lib/automake.mk
+++ b/lib/automake.mk
@@ -203,6 +203,8 @@ lib_libopenvswitch_la_SOURCES = \
lib/nx-match.h \
lib/object-collection.c \
lib/object-collection.h \
+ lib/odp-execute-private.c \
+ lib/odp-execute-private.h \
lib/odp-execute.c \
lib/odp-execute.h \
lib/odp-util.c \
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 649c700cb..eada4fcd7 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -1618,6 +1618,8 @@ create_dpif_netdev(struct dp_netdev *dp)
dpif->dp = dp;
dpif->last_port_seq = seq_read(dp->port_seq);
+odp_execute_init();
+
return >dpif;
}
diff --git a/lib/odp-execute-private.c b/lib/odp-execute-private.c
new file mode 100644
index 0..6441c491c
--- /dev/null
+++ b/lib/odp-execute-private.c
@@ -0,0 +1,84 @@
+/*
+ * Copyright (c) 2021 Intel.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include
+#include
+#include
+#include
+#include "dpdk.h"
+
+#include "openvswitch/vlog.h"
+#include "odp-execute-private.h"
+#include "odp-netlink.h"
+#include "dp-packet.h"
+#include "odp-util.h"
+
+
+int32_t action_autoval_init(struct odp_execute_action_impl *self);
+VLOG_DEFINE_THIS_MODULE(odp_execute_private);
+
+static struct odp_execute_action_impl action_impls[] = {
+[ACTION_IMPL_SCALAR] = {
+.available = 1,
+.name = "scalar",
+.probe = NULL,
+.init_func = NULL,
+},
+};
+
+static void
+action_impl_copy_funcs(struct odp_execute_action_impl *to,
+ const struct odp_execute_action_impl *from)
+{
+for (uint32_t i = 0; i < __OVS_KEY_ATTR_MAX; i++) {
+atomic_uintptr_t *func = (void *) >funcs[i];
+atomic_store_relaxed(func, (uintptr_t) from->funcs[i]);
+}
+}
+
+void
+odp_execute_action_init(void)
+{
+/* Call probe on each impl, and cache the result. */
+for (int i = 0; i < ACTION_IMPL_MAX; i++) {
+bool avail = true;
+if (action_impls[i].probe) {
+/* Return zero is success, non-zero means error. */
+avail = (action_impls[i].probe() == 0);
+}
+VLOG_INFO("Action implementation %s (available: %s)\n",
+ action_impls[i].name, avail ? "available" : "not available");
+action_impls[i].available = avail;
+}
+
+uint32_t i;
+for (i = 0; i < ACTION_IMPL_MAX; i++) {
+/* Each impl's function array is initialized to reflect the scalar
+ * implementation. This simplifies adding optimized implementations,
+ * as the autovalidator can always compare all actions.
+ *
+ * Below copies the scalar functions to all other implementations.
+ */
+if (i != ACTION_IMPL_SCALAR) {
+action_impl_copy_funcs(_impls[i],
+ _impls[ACTION_IMPL_SCALAR]);
+}
+
+if (action_impls[i].init_func) {
+action_impls[i].init_func(_impls[i]);
+}
+}
+}
diff --git a/lib/odp-execute-private.h b/lib/odp-execute-private.h
new file mode 100644
index 0..c2e86bbee
--- /dev/null
+++ b/lib/odp-execute-private.h
@@ -0,0 +1,98 @@
+/*
+ * Copyright (c) 2021 Intel.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ *
[ovs-dev] [PATCH v5 2/8] odp-execute: Add function pointer for pop_vlan action.
This commit removes the pop_vlan action from the large switch
and creates a separate function for batched processing. A function
pointer is also added to call the new batched function for the pop_vlan
action.
Signed-off-by: Emma Finn
Acked-by: Harry van Haaren
---
lib/odp-execute-private.c | 19 +-
lib/odp-execute.c | 41 +--
lib/odp-execute.h | 2 ++
3 files changed, 55 insertions(+), 7 deletions(-)
diff --git a/lib/odp-execute-private.c b/lib/odp-execute-private.c
index 6441c491c..d88ff4921 100644
--- a/lib/odp-execute-private.c
+++ b/lib/odp-execute-private.c
@@ -29,13 +29,14 @@
int32_t action_autoval_init(struct odp_execute_action_impl *self);
VLOG_DEFINE_THIS_MODULE(odp_execute_private);
+static uint32_t active_action_impl_index;
static struct odp_execute_action_impl action_impls[] = {
[ACTION_IMPL_SCALAR] = {
.available = 1,
.name = "scalar",
.probe = NULL,
-.init_func = NULL,
+.init_func = odp_action_scalar_init,
},
};
@@ -49,6 +50,22 @@ action_impl_copy_funcs(struct odp_execute_action_impl *to,
}
}
+int32_t
+odp_execute_action_set(const char *name,
+ struct odp_execute_action_impl *active)
+{
+uint32_t i;
+for (i = 0; i < ACTION_IMPL_MAX; i++) {
+/* string compare, and set ptrs *atomically*. */
+if (strcmp(action_impls[i].name, name) == 0) {
+action_impl_copy_funcs(active, _impls[i]);
+active_action_impl_index = i;
+return 0;
+}
+}
+return -1;
+}
+
void
odp_execute_action_init(void)
{
diff --git a/lib/odp-execute.c b/lib/odp-execute.c
index 49dfa2a74..ab051aecc 100644
--- a/lib/odp-execute.c
+++ b/lib/odp-execute.c
@@ -831,6 +831,28 @@ requires_datapath_assistance(const struct nlattr *a)
return false;
}
+static void
+action_pop_vlan(void *dp OVS_UNUSED, struct dp_packet_batch *batch,
+const struct nlattr *a OVS_UNUSED,
+bool should_steal OVS_UNUSED)
+{
+struct dp_packet *packet;
+DP_PACKET_BATCH_FOR_EACH (i, packet, batch) {
+eth_pop_vlan(packet);
+}
+}
+
+/* Implementation of the scalar actions impl init function. Build up the
+ * array of func ptrs here.
+ */
+int32_t
+odp_action_scalar_init(struct odp_execute_action_impl *self)
+{
+self->funcs[OVS_ACTION_ATTR_POP_VLAN] = action_pop_vlan;
+
+return 0;
+}
+
/* The active function pointers on the datapath. ISA optimized implementations
* are enabled by plugging them into this static arary, which is consulted when
* applying actions on the datapath.
@@ -843,10 +865,22 @@ odp_execute_init(void)
static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
if (ovsthread_once_start()) {
odp_execute_action_init();
+odp_actions_impl_set("scalar");
ovsthread_once_done();
}
}
+int32_t
+odp_actions_impl_set(const char *name)
+{
+
+int err = odp_execute_action_set(name, _active_impl);
+if (err) {
+VLOG_ERR("error %d from action set to %s\n", err, name);
+return -1;
+}
+return 0;
+}
/* Executes all of the 'actions_len' bytes of datapath actions in 'actions' on
* the packets in 'batch'. If 'steal' is true, possibly modifies and
@@ -962,12 +996,6 @@ odp_execute_actions(void *dp, struct dp_packet_batch
*batch, bool steal,
break;
}
-case OVS_ACTION_ATTR_POP_VLAN:
-DP_PACKET_BATCH_FOR_EACH (i, packet, batch) {
-eth_pop_vlan(packet);
-}
-break;
-
case OVS_ACTION_ATTR_PUSH_MPLS: {
const struct ovs_action_push_mpls *mpls = nl_attr_get(a);
@@ -1100,6 +1128,7 @@ odp_execute_actions(void *dp, struct dp_packet_batch
*batch, bool steal,
}
case OVS_ACTION_ATTR_OUTPUT:
case OVS_ACTION_ATTR_LB_OUTPUT:
+case OVS_ACTION_ATTR_POP_VLAN:
case OVS_ACTION_ATTR_TUNNEL_PUSH:
case OVS_ACTION_ATTR_TUNNEL_POP:
case OVS_ACTION_ATTR_USERSPACE:
diff --git a/lib/odp-execute.h b/lib/odp-execute.h
index c4f5303e7..6441392b9 100644
--- a/lib/odp-execute.h
+++ b/lib/odp-execute.h
@@ -32,6 +32,8 @@ struct dp_packet_batch;
/* Called once at initialization time. */
void odp_execute_init(void);
+int32_t odp_actions_impl_set(const char *name);
+
typedef void (*odp_execute_cb)(void *dp, struct dp_packet_batch *batch,
const struct nlattr *action, bool should_steal);
--
2.25.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v5 0/8] Actions Infrastructure + Optimizations
--- v5: - Rebase to master - Minor change to variable names - Added Tags from Harry. --- v4: - Rebase to master - Add ISA implementation of push_vlan action --- v3: - Refactored to fix unit test failures - Removed some sign-off on commits --- v2: - Fix the CI build issues --- This patchset introduces actions infrastructure changes which allows the user to choose between different action implementations based on CPU ISA by using different commands. The Infrastructure also provides a way to check the correctness of the ISA optimized action version against the scalar version. This patchset also introduces an optimized version of the push and pop vlan actions. Emma Finn (7): odp-execute: Add function pointers to odp-execute for different action implementations. odp-execute: Add function pointer for pop_vlan action. odp-execute: Add auto validation function for actions. odp-execute: Add command to switch action implementation. odp-execute: Add ISA implementation of actions. odp-execute: Add ISA implementation of pop_vlan action. odp-execute: Add ISA implementation of push_vlan action. Kumar Amber (1): dpif-netdev: Add configure option to enable actions autovalidator at build time. Documentation/topics/dpdk/bridge.rst | 25 +++ Documentation/topics/testing.rst | 20 ++- NEWS | 7 + acinclude.m4 | 17 ++ configure.ac | 1 + lib/automake.mk | 6 +- lib/cpu.c| 1 + lib/cpu.h| 1 + lib/dp-packet.c | 23 +++ lib/dp-packet.h | 5 + lib/dpif-netdev-unixctl.man | 6 + lib/dpif-netdev.c| 41 + lib/odp-execute-avx512.c | 194 +++ lib/odp-execute-private.c| 224 +++ lib/odp-execute-private.h| 110 + lib/odp-execute.c| 108 ++--- lib/odp-execute.h| 9 ++ tests/pmd.at | 21 +++ 18 files changed, 791 insertions(+), 28 deletions(-) create mode 100644 lib/odp-execute-avx512.c create mode 100644 lib/odp-execute-private.c create mode 100644 lib/odp-execute-private.h -- 2.25.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
