Re: [ovs-dev] [PATCH] tracing/treewide: Remove second parameter of __assign_str()
On 5/17/24 11:00, Guenter Roeck wrote:
On 5/17/24 10:48, Steven Rostedt wrote:
On Fri, 17 May 2024 10:36:37 -0700
Guenter Roeck wrote:
Building csky:allmodconfig (and others) ... failed
--
Error log:
In file included from include/trace/trace_events.h:419,
from include/trace/define_trace.h:102,
from drivers/cxl/core/trace.h:737,
from drivers/cxl/core/trace.c:8:
drivers/cxl/core/./trace.h:383:1: error: macro "__assign_str" passed 2
arguments, but takes just 1
This is with the patch applied on top of v6.9-8410-gff2632d7d08e.
So far that seems to be the only build failure.
Introduced with commit 6aec00139d3a8 ("cxl/core: Add region info to
cxl_general_media and cxl_dram events"). Guess we'll see more of those
towards the end of the commit window.
Looks like I made this patch just before this commit was pulled into
Linus's tree.
Which is why I'll apply and rerun the above again probably on Tuesday of
next week against Linus's latest.
This patch made it through both an allyesconfig and an allmodconfig, but on
the commit I had applied it to, which was:
1b294a1f3561 ("Merge tag 'net-next-6.10' of
git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next")
I'll be compiling those two builds after I update it then.
I am currently repeating my test builds with the above errors fixed.
That should take a couple of hours. I'll let you know how it goes.
There are no more build failures caused by this patch after fixing the above
errors.
Tested-by: Guenter Roeck
Guenter
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] |fail| pw1936643 [PATCH] netdev: Padding runt packet on VXLAN and DPDK ports.
From: ro...@bytheb.org Test-Label: github-robot: Build and Test Test-Status: fail http://patchwork.ozlabs.org/api/patches/1936643/ _github build: failed_ Build URL: https://github.com/ovsrobot/ovs/actions/runs/9135321447 Build Logs: ---Summary of failed steps--- "osx clang --disable-ssl" failed at step build --End summary of failed steps ---BEGIN LOGS [Begin job log] "osx clang --disable-ssl" at step build [End job log] "osx clang --disable-ssl" at step build END LOGS- ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] |fail| pw1936642 [PATCH] netdev: Padding runt packet on VXLAN and DPDK ports.
From: ro...@bytheb.org Test-Label: github-robot: Build and Test Test-Status: fail http://patchwork.ozlabs.org/api/patches/1936642/ _github build: failed_ Build URL: https://github.com/ovsrobot/ovs/actions/runs/9135163013 Build Logs: ---Summary of failed steps--- "osx clang --disable-ssl" failed at step build "linux gcc check check-dpdk dpdk" failed at step build "linux clang check check-dpdk dpdk" failed at step build --End summary of failed steps ---BEGIN LOGS [Begin job log] "osx clang --disable-ssl" at step build [End job log] "osx clang --disable-ssl" at step build [Begin job log] "linux gcc check check-dpdk dpdk" at step build [End job log] "linux gcc check check-dpdk dpdk" at step build [Begin job log] "linux clang check check-dpdk dpdk" at step build [End job log] "linux clang check check-dpdk dpdk" at step build END LOGS- ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] netdev: Padding runt packet on VXLAN and DPDK ports.
The basic idea behind this patch is to pad the runt packet before vxlan
encapsulation and on a DPDK port.
topology:
IXIA <> SWITCH <--vxlan--> OVS <--dpdk--> HOST
The host sends a runt packet (54 bytes) with a size less than 64 bytes to the
OVS. The OVS receives this runt packet and sends it further down the VXLAN
tunnel (original packet 54 + 50 VXLAN = 104 B total packet size) to a physical
switch. At the switch, after decapsulation, the original packet size is 54B and
it then sends the packet on to the ixia. However, the switch adds 2B of
padding with random content (not zero as RFC 1042 mentions) and due to this
random byte padding, ixia drops the packet.
Switch vendors claim to be compliant with both RFC 1042 and RFC 2119 to fix it
in the switch.
RFC 1042 defines:
“IEEE 802 packets may have a minimum size restriction. When
necessary, the data field should be padded (with octets of zero)
to meet the IEEE 802 minimum frame size requirements. This
padding is not part of the IP datagram and is not included in the
total length field of the IP header.”
RFC 2119 defines:
"SHOULD This word, or the adjective "RECOMMENDED", mean that there
may exist valid reasons in particular circumstances to ignore a
particular item, but the full implications must be understood and
carefully weighed before choosing a different course."
So, a fix is needed in the OVS. The OVS is connected to the switch on a VXLAN
port and to the host on a DPDK port. The padding fix is applied to both netdev
types. I tested the fix in the same setup and below are the captures after
padding on both VXLAN and DPDK ports from OVS UT.
VXLAN (46B zero pad):
aa 55 aa 66 00 00 aa 55 aa 55 00 00 08 00 45 00 .U.f...U.UE.
0010 00 60 00 00 40 00 40 11 26 81 0a 00 00 02 0a 00 .`..@.@.&...
0020 00 0b 9b 77 12 b5 00 4c 00 00 08 00 00 00 00 00 ...w...L
0030 7b 00 50 54 00 00 00 0a 50 54 00 00 00 09 12 34 {.PTPT.4
0040 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0050 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0060 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ..
DPDK (16B zero pad):
ff ff ff ff ff ff 5e fb 90 96 7d b7 08 06 00 01 ..^...}.
0010 08 00 06 04 00 01 5e fb 90 96 7d b7 0a 01 01 01 ..^...}.
0020 00 00 00 00 00 00 0a 01 01 02 00 00 00 00 00 00
0030 00 00 00 00 00 00 00 00 00 00 00 00
Signed-off-by: Rohit Kumar
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 7b84c858e..441c7b2ea 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -26,6 +26,7 @@
#include
#include
#include
+#include
#include
#include
@@ -2755,6 +2756,8 @@ netdev_dpdk_eth_tx_burst(struct netdev_dpdk *dev, int qid,
{
uint32_t nb_tx = 0;
uint16_t nb_tx_prep = cnt;
+uint8_t pad = 0;
+char *data = NULL;
nb_tx_prep = rte_eth_tx_prepare(dev->port_id, qid, pkts, cnt);
if (nb_tx_prep != cnt) {
@@ -2765,6 +2768,24 @@ netdev_dpdk_eth_tx_burst(struct netdev_dpdk *dev, int
qid,
"First invalid packet", pkts[nb_tx_prep]);
}
+while (nb_tx != nb_tx_prep) {
+/* adjust the packet to minimum ethernet frame len of 60 bytes */
+if (OVS_UNLIKELY (rte_pktmbuf_pkt_len (pkts[nb_tx]) < ETH_ZLEN)) {
+pad = ETH_ZLEN - rte_pktmbuf_pkt_len (pkts[nb_tx]);
+data = rte_pktmbuf_append (pkts[nb_tx], pad);
+if (OVS_UNLIKELY (data == NULL)) {
+VLOG_WARN_RL(, "Mbuf appending failed to pad %d bytes",
pad);
+/* This packet has error, can't send it, so just free it */
+rte_pktmbuf_free(pkts[nb_tx]);
+nb_tx_prep -= 1;
+continue;
+}
+memset (data, 0, pad);
+}
+nb_tx += 1;
+}
+
+nb_tx = 0;
while (nb_tx != nb_tx_prep) {
uint32_t ret;
diff --git a/lib/netdev.c b/lib/netdev.c
index f2d921ed6..97d3711fb 100644
--- a/lib/netdev.c
+++ b/lib/netdev.c
@@ -24,6 +24,7 @@
#include
#include
#include
+#include
#ifndef _WIN32
#include
@@ -1007,8 +1008,16 @@ netdev_push_header(const struct netdev *netdev,
{
struct dp_packet *packet;
size_t i, size = dp_packet_batch_size(batch);
+uint8_t pad = 0;
DP_PACKET_BATCH_REFILL_FOR_EACH (i, size, packet, batch) {
+/* adjust the packet to minimum ethernet frame len of 60 bytes */
+if (OVS_UNLIKELY (data->tnl_type == OVS_VPORT_TYPE_VXLAN
+&& dp_packet_is_eth (packet)
+&& dp_packet_size (packet) < ETH_ZLEN)) {
+pad = ETH_ZLEN - dp_packet_size (packet);
+dp_packet_put_zeros (packet, pad);
+}
if (OVS_UNLIKELY(data->tnl_type != OVS_VPORT_TYPE_GENEVE &&
data->tnl_type != OVS_VPORT_TYPE_VXLAN &&
[ovs-dev] [PATCH] netdev: Padding runt packet on VXLAN and DPDK ports.
The basic idea behind this patch is to pad the runt packet before vxlan
encapsulation and on a DPDK port.
topology:
IXIA <> SWITCH <--vxlan--> OVS <--dpdk--> HOST
The host sends a runt packet (54 bytes) with a size less than 64 bytes to the
OVS. The OVS receives this runt packet and sends it further down the VXLAN
tunnel (original packet 54 + 50 VXLAN = 104 B total packet size) to a physical
switch. At the switch, after decapsulation, the original packet size is 54B and
it then sends the packet on to the ixia. However, the switch adds 2B of
padding with random content (not zero as RFC 1042 mentions) and due to this
random byte padding, ixia drops the packet.
Switch vendors claim to be compliant with both RFC 1042 and RFC 2119 to fix it
in the switch.
RFC 1042 defines:
“IEEE 802 packets may have a minimum size restriction. When
necessary, the data field should be padded (with octets of zero)
to meet the IEEE 802 minimum frame size requirements. This
padding is not part of the IP datagram and is not included in the
total length field of the IP header.”
RFC 2119 defines:
"SHOULD This word, or the adjective "RECOMMENDED", mean that there
may exist valid reasons in particular circumstances to ignore a
particular item, but the full implications must be understood and
carefully weighed before choosing a different course."
So, a fix is needed in the OVS. The OVS is connected to the switch on a VXLAN
port and to the host on a DPDK port. The padding fix is applied to both netdev
types. I tested the fix in the same setup and below are the captures after
padding on both VXLAN and DPDK ports from OVS UT.
VXLAN (46B zero pad):
aa 55 aa 66 00 00 aa 55 aa 55 00 00 08 00 45 00 .U.f...U.UE.
0010 00 60 00 00 40 00 40 11 26 81 0a 00 00 02 0a 00 .`..@.@.&...
0020 00 0b 9b 77 12 b5 00 4c 00 00 08 00 00 00 00 00 ...w...L
0030 7b 00 50 54 00 00 00 0a 50 54 00 00 00 09 12 34 {.PTPT.4
0040 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0050 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0060 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ..
DPDK (16B zero pad):
ff ff ff ff ff ff 5e fb 90 96 7d b7 08 06 00 01 ..^...}.
0010 08 00 06 04 00 01 5e fb 90 96 7d b7 0a 01 01 01 ..^...}.
0020 00 00 00 00 00 00 0a 01 01 02 00 00 00 00 00 00
0030 00 00 00 00 00 00 00 00 00 00 00 00
Signed-off-by: Rohit Kumar
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 7b84c858e..2b73f174c 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -26,6 +26,7 @@
#include
#include
#include
+#include
#include
#include
@@ -2755,6 +2756,8 @@ netdev_dpdk_eth_tx_burst(struct netdev_dpdk *dev, int qid,
{
uint32_t nb_tx = 0;
uint16_t nb_tx_prep = cnt;
+uint8_t pad = 0;
+char *data = NULL;
nb_tx_prep = rte_eth_tx_prepare(dev->port_id, qid, pkts, cnt);
if (nb_tx_prep != cnt) {
@@ -2765,6 +2768,25 @@ netdev_dpdk_eth_tx_burst(struct netdev_dpdk *dev, int
qid,
"First invalid packet", pkts[nb_tx_prep]);
}
+while (nb_tx != nb_tx_prep) {
+/* adjust the packet to minimum ethernet frame len of 60 bytes */
+if (OVS_UNLIKELY (rte_pktmbuf_pkt_len (pkts[nb_tx]) < ETH_ZLEN)) {
+VLOG_ERR ("smaller packet...");
+pad = ETH_ZLEN - rte_pktmbuf_pkt_len (pkts[nb_tx]);
+data = rte_pktmbuf_append (pkts[nb_tx], pad);
+if (OVS_UNLIKELY (data == NULL)) {
+VLOG_WARN_RL(, "Mbuf appending failed to pad %d bytes",
pad);
+/* This packet has error, can't send it, so just free it */
+rte_pktmbuf_free(pkts[nb_tx]);
+nb_tx_prep -= 1;
+continue;
+}
+memset (data, 0, pad);
+}
+nb_tx += 1;
+}
+
+nb_tx = 0;
while (nb_tx != nb_tx_prep) {
uint32_t ret;
diff --git a/lib/netdev.c b/lib/netdev.c
index f2d921ed6..97d3711fb 100644
--- a/lib/netdev.c
+++ b/lib/netdev.c
@@ -24,6 +24,7 @@
#include
#include
#include
+#include
#ifndef _WIN32
#include
@@ -1007,8 +1008,16 @@ netdev_push_header(const struct netdev *netdev,
{
struct dp_packet *packet;
size_t i, size = dp_packet_batch_size(batch);
+uint8_t pad = 0;
DP_PACKET_BATCH_REFILL_FOR_EACH (i, size, packet, batch) {
+/* adjust the packet to minimum ethernet frame len of 60 bytes */
+if (OVS_UNLIKELY (data->tnl_type == OVS_VPORT_TYPE_VXLAN
+&& dp_packet_is_eth (packet)
+&& dp_packet_size (packet) < ETH_ZLEN)) {
+pad = ETH_ZLEN - dp_packet_size (packet);
+dp_packet_put_zeros (packet, pad);
+}
if (OVS_UNLIKELY(data->tnl_type != OVS_VPORT_TYPE_GENEVE &&
data->tnl_type !=
Re: [ovs-dev] [PATCH 5/5] netdev-linux: Fix Clang's static analyzer uninitialized values warnings.
On 5/16/24 21:37, Mike Pattrick wrote:
> Clang's static analyzer will complain about an uninitialized value
> because in some error conditions we wouldn't set all values that are
> used later.
Some more details would be nice here.
>
> Now we initialize more values that are needed later even in error
> conditions.
>
> Signed-off-by: Mike Pattrick
> ---
Same for the subject line and a Fixes tag.
Also, there seems to be two separate issues fixed here, they should
be separate patches with distinct names and Fixes tags as they may
need to go to different branches (I didn't check).
> lib/netdev-linux.c | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index 25349c605..66dae3e1a 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -2439,7 +2439,9 @@ netdev_linux_read_definitions(struct netdev_linux
> *netdev,
> int error = 0;
>
> error = netdev_linux_read_stringset_info(netdev, );
> -if (error || !len) {
> +if (!len) {
> +return -EOPNOTSUPP;
> +} else if (error) {
> return error;
> }
> strings = xzalloc(sizeof *strings + len * ETH_GSTRING_LEN);
> @@ -2724,6 +2726,7 @@ netdev_linux_get_speed_locked(struct netdev_linux
> *netdev,
>uint32_t *current, uint32_t *max)
> {
> if (netdev_linux_netnsid_is_remote(netdev)) {
> +*current = 0;
> return EOPNOTSUPP;
> }
>
> @@ -2733,6 +2736,8 @@ netdev_linux_get_speed_locked(struct netdev_linux
> *netdev,
> ? 0 : netdev->current_speed;
> *max = MIN(UINT32_MAX,
> netdev_features_to_bps(netdev->supported, 0) /
> 100ULL);
> +} else {
> +*current = 0;
> }
> return netdev->get_features_error;
> }
We should be clearing both the current and a max for consistency.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 4/5] socket: Fix Clang's static analyzer 'garbage value' warnings.
On 5/16/24 21:36, Mike Pattrick wrote:
> Clang's static analyzer will complain about an uninitialized value
> because we weren't setting a value for dns_failure in all code paths.
>
> Now we initialize this on declaration.
>
> Signed-off-by: Mike Pattrick
> ---
Same comments for the subject line and a Fixes tag.
> lib/socket-util.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/socket-util.c b/lib/socket-util.c
> index 2d89fce85..1d21ce01c 100644
> --- a/lib/socket-util.c
> +++ b/lib/socket-util.c
> @@ -711,7 +711,7 @@ inet_open_passive(int style, const char *target, int
> default_port,
> struct sockaddr_storage ss;
> int fd = 0, error;
> unsigned int yes = 1;
> -bool dns_failure;
> +bool dns_failure = false;
>
> if (!inet_parse_passive(target, default_port, , true, _failure)) {
> if (dns_failure) {
I'm not sure this is a right solution. inet_parse_passive() should set
the 'dns_failure' whenever it fails, i.e. it should set 'dns_failure'
in every condition where it sets ok = false.
inet_parse_passive() is also not a static function, so we should fix it
instead, so other potential users do not have this issue.
While at it, inet_parse_active() has the same problem. It should set
the 'dns_failure' whenever it fails.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 3/5] dpctl: Fix Clang's static analyzer 'garbage value' warnings.
On 5/16/24 21:36, Mike Pattrick wrote:
> Clang's static analyzer will complain about an uninitialized value
> because we weren't setting a value for ufid_generated in all code paths.
>
> Now we initialize this on declaration.
>
> Signed-off-by: Mike Pattrick
> ---
Same comments for the subject line.
Also needs a Fixes tag.
> lib/dpctl.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/lib/dpctl.c b/lib/dpctl.c
> index 3c555a559..9c287d060 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -1366,12 +1366,11 @@ dpctl_del_flow_dpif(struct dpif *dpif, const char
> *key_s,
> struct ofpbuf mask; /* To be ignored. */
>
> ovs_u128 ufid;
> -bool ufid_generated;
> -bool ufid_present;
> +bool ufid_generated = false;
> +bool ufid_present = false;
Maybe move these above the 'ufid' for the sake of a reverse x-mass.
And move the 'ufid' below the port names.
> struct simap port_names;
> int n, error;
>
> -ufid_present = false;
> n = odp_ufid_from_string(key_s, );
> if (n < 0) {
> dpctl_error(dpctl_p, -n, "parsing flow ufid");
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2/5] netdev-native-tnl: Fix Clang's static analyzer 'uninitialized value' warnings.
On 5/17/24 22:14, Ilya Maximets wrote:
> On 5/16/24 21:36, Mike Pattrick wrote:
>> Clang's static analyzer will complain about an uninitialized value
>> because we weren't properly checking the error code from a function that
>> would have initialized the value.
>
> Please, be more specific. :) At least, mention the variable name.
>
> And the same comment for the subject of the patch. This one is
> actually a real potential issue.
And it also needs a Fixes tag, since it's not a false-positive.
> So, something like this would
> be an appropriate name:
>
> netdev-native-tnl: Fix use of uninitialized offset on SRv6 header pop.
>
>>
>> Instead, add a check for that return code.
>>
>> Signed-off-by: Mike Pattrick
>> ---
>> lib/netdev-native-tnl.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
>> index dee9ab344..6e6b15764 100644
>> --- a/lib/netdev-native-tnl.c
>> +++ b/lib/netdev-native-tnl.c
>> @@ -1068,7 +1068,9 @@ netdev_srv6_pop_header(struct dp_packet *packet)
>> }
>>
>> pkt_metadata_init_tnl(md);
>> -netdev_tnl_ip_extract_tnl_md(packet, tnl, );
>> +if (netdev_tnl_ip_extract_tnl_md(packet, tnl, ) == NULL) {
>
> if (!...)
>
>> +goto err;
>> +}
>
> Maybe an empty line here.
>
>> dp_packet_reset_packet(packet, hlen);
>>
>> return packet;
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2/5] netdev-native-tnl: Fix Clang's static analyzer 'uninitialized value' warnings.
On 5/16/24 21:36, Mike Pattrick wrote:
> Clang's static analyzer will complain about an uninitialized value
> because we weren't properly checking the error code from a function that
> would have initialized the value.
Please, be more specific. :) At least, mention the variable name.
And the same comment for the subject of the patch. This one is
actually a real potential issue. So, something like this would
be an appropriate name:
netdev-native-tnl: Fix use of uninitialized offset on SRv6 header pop.
>
> Instead, add a check for that return code.
>
> Signed-off-by: Mike Pattrick
> ---
> lib/netdev-native-tnl.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
> index dee9ab344..6e6b15764 100644
> --- a/lib/netdev-native-tnl.c
> +++ b/lib/netdev-native-tnl.c
> @@ -1068,7 +1068,9 @@ netdev_srv6_pop_header(struct dp_packet *packet)
> }
>
> pkt_metadata_init_tnl(md);
> -netdev_tnl_ip_extract_tnl_md(packet, tnl, );
> +if (netdev_tnl_ip_extract_tnl_md(packet, tnl, ) == NULL) {
if (!...)
> +goto err;
> +}
Maybe an empty line here.
> dp_packet_reset_packet(packet, hlen);
>
> return packet;
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 1/5] netdev-offload: Fix Clang's static analyzer 'null pointer dereference' warnings.
On 5/16/24 21:36, Mike Pattrick wrote:
> Clang's static analyzer will complain about a null pointer dereference
> because dumps can be set to null and then there is a loop where it could
> have been written to.
>
> Instead, return early from the netdev_ports_flow_dump_create function if
> dumps is NULL.
>
> Signed-off-by: Mike Pattrick
> ---
> lib/netdev-offload.c | 9 -
> 1 file changed, 8 insertions(+), 1 deletion(-)
Thanks for the patch!
It is a false-positive as both loops are iterated while holding a lock.
Might be worth mentioning in the commit message.
Also, please, use more precise and concise names for the patches in the
set to reduce the chance of commits with the exact same names in the git
history. This may confuse some automation.
This one, for example, can be named:
netdev-offload: Fix null pointer dereference warning on dump creation.
There is no need to mention clang or static analysis in the subject,
this can be put in the commit message.
>
> diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c
> index 931d634e1..02b1cf203 100644
> --- a/lib/netdev-offload.c
> +++ b/lib/netdev-offload.c
> @@ -638,7 +638,14 @@ netdev_ports_flow_dump_create(const char *dpif_type, int
> *ports, bool terse)
> }
> }
>
> -dumps = count ? xzalloc(sizeof *dumps * count) : NULL;
> +if (count == 0) {
I'd prefer !count for zero checks.
> +*ports = 0;
> +ovs_rwlock_unlock(_to_netdev_rwlock);
> +
> +return NULL;
> +}
> +
> +dumps = xzalloc(sizeof *dumps * count);
I'd suggest we initialize the 'dumps' to NULL at the top and then
just if (!count) { goto unlock; } to avoid code duplication.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 2/2] atlocal: Replace deprecated pkg_resources.
'pkg_resources' module is deprecated and no longer available in newer
versions of python, so pytest tests are skipped:
DeprecationWarning: pkg_resources is deprecated as an API.
See https://setuptools.pypa.io/en/latest/pkg_resources.html
Unfortunately, there is no direct replacement for it and functionality
is scattered between different packages.
Using a new standard library importlib.metadata to find installed
packages and their versions. Using packaging.requirements to parse
lines from the requirements file and compare versions. This covers
all we need.
The 'packaging' is a project used by pip and a dependency for many
other libraries, so should be available for any supported verison of
python. 'importlib' was introduced in python 3.8. Since we support
older versions of python and 'packaging' is not part of the standard
library, checking that import is possible and falling back to
'pkg_resources' if needed. We may remove the fallback when we stop
supporting python below 3.8.
Even though 'packaging' is a common dependency, added to the test
requirements so it will not be missed in CI.
Signed-off-by: Ilya Maximets
---
python/test_requirements.txt | 1 +
tests/atlocal.in | 28 ++--
2 files changed, 23 insertions(+), 6 deletions(-)
diff --git a/python/test_requirements.txt b/python/test_requirements.txt
index 5043c71e2..a1424506b 100644
--- a/python/test_requirements.txt
+++ b/python/test_requirements.txt
@@ -1,4 +1,5 @@
netaddr
+packaging
pyftpdlib
pyparsing
pytest
diff --git a/tests/atlocal.in b/tests/atlocal.in
index 466fd4ed4..8565a0bae 100644
--- a/tests/atlocal.in
+++ b/tests/atlocal.in
@@ -229,15 +229,31 @@ export UBSAN_OPTIONS
REQUIREMENT_PATH=$abs_top_srcdir/python/test_requirements.txt $PYTHON3 -c '
import os
import pathlib
-import pkg_resources
import sys
+PACKAGING = True
+try:
+from packaging import requirements
+from importlib import metadata
+except ModuleNotFoundError:
+PACKAGING = False
+import pkg_resources
+
with pathlib.Path(os.path.join(os.getenv("REQUIREMENT_PATH"))).open() as reqs:
-for req in pkg_resources.parse_requirements(reqs):
-try:
-pkg_resources.require(str(req))
-except pkg_resources.DistributionNotFound:
-sys.exit(2)
+if PACKAGING:
+for req in reqs.readlines():
+try:
+r = requirements.Requirement(req.strip())
+if metadata.version(r.name) not in r.specifier:
+raise metadata.PackageNotFoundError
+except metadata.PackageNotFoundError:
+sys.exit(2)
+else:
+for req in pkg_resources.parse_requirements(reqs):
+try:
+pkg_resources.require(str(req))
+except pkg_resources.DistributionNotFound:
+sys.exit(2)
'
case $? in
0) HAVE_PYTEST=yes ;;
--
2.45.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 1/2] atlocal: Fix setting HAVE_PYTEST on unexpected errors.
If the python script throws an unexpected exception, the HAVE_PYTEST
variable remains undefined. If at the same time dependencies are not
actually present, pytest tests will fail instead of being skipped.
Define the variable to 'no' on unexpected failures to skip the tests
when dependencies cannot be verified.
The issue can be reproduced on systems with python 3.12+ in case the
deprecated 'pkg_resources' module is not available.
Fixes: 445dceb88461 ("python: Introduce unit tests.")
Signed-off-by: Ilya Maximets
---
tests/atlocal.in | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tests/atlocal.in b/tests/atlocal.in
index f321bae55..466fd4ed4 100644
--- a/tests/atlocal.in
+++ b/tests/atlocal.in
@@ -242,5 +242,6 @@ with
pathlib.Path(os.path.join(os.getenv("REQUIREMENT_PATH"))).open() as reqs:
case $? in
0) HAVE_PYTEST=yes ;;
2) HAVE_PYTEST=no ;;
-*) echo "$0: unexpected error probing Python unit test requirements" >&2 ;;
+*) HAVE_PYTEST=no
+ echo "$0: unexpected error probing Python unit test requirements" >&2 ;;
esac
--
2.45.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 0/2] atlocal: Fixes for dependency detection.
'pkg_resources' module is deprecated, so it needs a replacement. Ilya Maximets (2): atlocal: Fix setting HAVE_PYTEST on unexpected errors. atlocal: Replace deprecated pkg_resources. python/test_requirements.txt | 1 + tests/atlocal.in | 31 --- 2 files changed, 25 insertions(+), 7 deletions(-) -- 2.45.0 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] srv6: Fix misaligned writes to segment list.
Segments list in SRv6 header is 16-bit aligned as most of other fields
in packet headers. A little counter-intuitively, compilers are allowed
to make alignment assumptions based on the pointer type passed to
memcpy(), so they can use copy instructions that require 32-bit alignment
in case of struct in6_addr pointer. Reported by UBsan in Clang 18:
lib/netdev-native-tnl.c:985:16: runtime error: store to misaligned
address 0x7fd9e97351ce for type 'struct in6_addr *', which
requires 4 byte alignment
0x7fd9e97351ce: note: pointer points here
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
^
0 0xc1de38 in netdev_srv6_build_header lib/netdev-native-tnl.c:985:9
1 0x6e794b in tnl_port_build_header ofproto/tunnel.c:751:11
2 0x6c9c0a in native_tunnel_output ofproto/ofproto-dpif-xlate.c:3887:11
3 0x6c9c0a in compose_output_action__ ofproto/ofproto-dpif-xlate.c:4502:13
4 0x6b6646 in compose_output_action ofproto/ofproto-dpif-xlate.c:4564:5
5 0x6b6646 in xlate_output_action ofproto/ofproto-dpif-xlate.c:5517:13
6 0x68cfee in do_xlate_actions ofproto/ofproto-dpif-xlate.c:7288:13
7 0x67fed0 in xlate_actions ofproto/ofproto-dpif-xlate.c:8314:13
8 0x6468bd in ofproto_trace__ ofproto/ofproto-dpif-trace.c:782:30
9 0x64484a in ofproto_trace ofproto/ofproto-dpif-trace.c:851:5
10 0x647469 in ofproto_unixctl_trace ofproto/ofproto-dpif-trace.c:490:9
11 0xc33771 in process_command lib/unixctl.c:310:13
12 0xc33771 in run_connection lib/unixctl.c:344:17
13 0xc33771 in unixctl_server_run lib/unixctl.c:395:21
14 0x53e6ef in main vswitchd/ovs-vswitchd.c:131:9
15 0x7f61c7 in __libc_start_call_main (/lib64/libc.so.6+0x2a1c7)
16 0x7f628a in __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x2a28a)
17 0x42ca24 in _start (vswitchd/ovs-vswitchd+0x42ca24)
SUMMARY: UndefinedBehaviorSanitizer:
undefined-behavior lib/netdev-native-tnl.c:985:16
Having misaligned pointers is also generally not allowed in C, let
alone accessing memory through them.
Fix that by using an appropriate ovs_16aligned_in6_addr pointer instead.
Fixes: 7381fd440a88 ("odp: Add SRv6 tunnel actions.")
Fixes: 03fc1ad78521 ("userspace: Add SRv6 tunnel support.")
Signed-off-by: Ilya Maximets
---
lib/netdev-native-tnl.c | 5 ++---
lib/odp-util.c | 4 ++--
2 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
index dee9ab344..b21176037 100644
--- a/lib/netdev-native-tnl.c
+++ b/lib/netdev-native-tnl.c
@@ -932,9 +932,9 @@ netdev_srv6_build_header(const struct netdev *netdev,
const struct netdev_tnl_build_header_params *params)
{
const struct netdev_tunnel_config *tnl_cfg;
+union ovs_16aligned_in6_addr *s;
const struct in6_addr *segs;
struct srv6_base_hdr *srh;
-struct in6_addr *s;
ovs_be16 dl_type;
int nr_segs;
int i;
@@ -978,8 +978,7 @@ netdev_srv6_build_header(const struct netdev *netdev,
return EOPNOTSUPP;
}
-s = ALIGNED_CAST(struct in6_addr *,
- (char *) srh + sizeof *srh);
+s = (union ovs_16aligned_in6_addr *) (srh + 1);
for (i = 0; i < nr_segs; i++) {
/* Segment list is written to the header in reverse order. */
memcpy(s, [nr_segs - i - 1], sizeof *s);
diff --git a/lib/odp-util.c b/lib/odp-util.c
index 21f34d955..724e6f2bc 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -1820,8 +1820,8 @@ ovs_parse_tnl_push(const char *s, struct
ovs_action_push_tnl *data)
} else if (ovs_scan_len(s, , "srv6(segments_left=%"SCNu8,
_left)) {
struct srv6_base_hdr *srh = (struct srv6_base_hdr *) (ip6 + 1);
+union ovs_16aligned_in6_addr *segs;
char seg_s[IPV6_SCAN_LEN + 1];
-struct in6_addr *segs;
struct in6_addr seg;
uint8_t n_segs = 0;
@@ -1844,7 +1844,7 @@ ovs_parse_tnl_push(const char *s, struct
ovs_action_push_tnl *data)
return -EINVAL;
}
-segs = ALIGNED_CAST(struct in6_addr *, srh + 1);
+segs = (union ovs_16aligned_in6_addr *) (srh + 1);
segs += segments_left;
while (ovs_scan_len(s, , IPV6_SCAN_FMT, seg_s)
--
2.45.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] tracing/treewide: Remove second parameter of __assign_str()
On Thu, May 16, 2024 at 01:34:54PM -0400, Steven Rostedt wrote: > From: "Steven Rostedt (Google)" > > [ >This is a treewide change. I will likely re-create this patch again in >the second week of the merge window of v6.10 and submit it then. Hoping >to keep the conflicts that it will cause to a minimum. > ] > > With the rework of how the __string() handles dynamic strings where it > saves off the source string in field in the helper structure[1], the > assignment of that value to the trace event field is stored in the helper > value and does not need to be passed in again. > > This means that with: > > __string(field, mystring) > > Which use to be assigned with __assign_str(field, mystring), no longer > needs the second parameter and it is unused. With this, __assign_str() > will now only get a single parameter. > > There's over 700 users of __assign_str() and because coccinelle does not > handle the TRACE_EVENT() macro I ended up using the following sed script: > > git grep -l __assign_str | while read a ; do > sed -e 's/\(__assign_str([^,]*[^ ,]\) *,[^;]*/\1)/' $a > /tmp/test-file; > mv /tmp/test-file $a; > done > > I then searched for __assign_str() that did not end with ';' as those > were multi line assignments that the sed script above would fail to catch. > > Note, the same updates will need to be done for: > > __assign_str_len() > __assign_rel_str() > __assign_rel_str_len() > > I tested this with both an allmodconfig and an allyesconfig (build only for > both). > > [1] > https://lore.kernel.org/linux-trace-kernel/2024011442.634192...@goodmis.org/ > > Cc: Masami Hiramatsu > Cc: Mathieu Desnoyers > Cc: Linus Torvalds > Cc: Julia Lawall > Signed-off-by: Steven Rostedt (Google) /me finds this pretty magical, but such is the way of macros. Thanks for being much smarter about them than me. :) Acked-by: Darrick J. Wong# xfs --D ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] tracing/treewide: Remove second parameter of __assign_str()
On 5/17/24 10:48, Steven Rostedt wrote:
On Fri, 17 May 2024 10:36:37 -0700
Guenter Roeck wrote:
Building csky:allmodconfig (and others) ... failed
--
Error log:
In file included from include/trace/trace_events.h:419,
from include/trace/define_trace.h:102,
from drivers/cxl/core/trace.h:737,
from drivers/cxl/core/trace.c:8:
drivers/cxl/core/./trace.h:383:1: error: macro "__assign_str" passed 2
arguments, but takes just 1
This is with the patch applied on top of v6.9-8410-gff2632d7d08e.
So far that seems to be the only build failure.
Introduced with commit 6aec00139d3a8 ("cxl/core: Add region info to
cxl_general_media and cxl_dram events"). Guess we'll see more of those
towards the end of the commit window.
Looks like I made this patch just before this commit was pulled into
Linus's tree.
Which is why I'll apply and rerun the above again probably on Tuesday of
next week against Linus's latest.
This patch made it through both an allyesconfig and an allmodconfig, but on
the commit I had applied it to, which was:
1b294a1f3561 ("Merge tag 'net-next-6.10' of
git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next")
I'll be compiling those two builds after I update it then.
I am currently repeating my test builds with the above errors fixed.
That should take a couple of hours. I'll let you know how it goes.
Guenter
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] tracing/treewide: Remove second parameter of __assign_str()
On Fri, 17 May 2024 10:36:37 -0700
Guenter Roeck wrote:
> Building csky:allmodconfig (and others) ... failed
> --
> Error log:
> In file included from include/trace/trace_events.h:419,
> from include/trace/define_trace.h:102,
> from drivers/cxl/core/trace.h:737,
> from drivers/cxl/core/trace.c:8:
> drivers/cxl/core/./trace.h:383:1: error: macro "__assign_str" passed 2
> arguments, but takes just 1
>
> This is with the patch applied on top of v6.9-8410-gff2632d7d08e.
> So far that seems to be the only build failure.
> Introduced with commit 6aec00139d3a8 ("cxl/core: Add region info to
> cxl_general_media and cxl_dram events"). Guess we'll see more of those
> towards the end of the commit window.
Looks like I made this patch just before this commit was pulled into
Linus's tree.
Which is why I'll apply and rerun the above again probably on Tuesday of
next week against Linus's latest.
This patch made it through both an allyesconfig and an allmodconfig, but on
the commit I had applied it to, which was:
1b294a1f3561 ("Merge tag 'net-next-6.10' of
git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next")
I'll be compiling those two builds after I update it then.
-- Steve
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] tracing/treewide: Remove second parameter of __assign_str()
On Thu, May 16, 2024 at 01:34:54PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)"
>
> [
>This is a treewide change. I will likely re-create this patch again in
>the second week of the merge window of v6.10 and submit it then. Hoping
>to keep the conflicts that it will cause to a minimum.
> ]
>
> With the rework of how the __string() handles dynamic strings where it
> saves off the source string in field in the helper structure[1], the
> assignment of that value to the trace event field is stored in the helper
> value and does not need to be passed in again.
>
> This means that with:
>
> __string(field, mystring)
>
> Which use to be assigned with __assign_str(field, mystring), no longer
> needs the second parameter and it is unused. With this, __assign_str()
> will now only get a single parameter.
>
> There's over 700 users of __assign_str() and because coccinelle does not
> handle the TRACE_EVENT() macro I ended up using the following sed script:
>
> git grep -l __assign_str | while read a ; do
> sed -e 's/\(__assign_str([^,]*[^ ,]\) *,[^;]*/\1)/' $a > /tmp/test-file;
> mv /tmp/test-file $a;
> done
>
> I then searched for __assign_str() that did not end with ';' as those
> were multi line assignments that the sed script above would fail to catch.
>
Building csky:allmodconfig (and others) ... failed
--
Error log:
In file included from include/trace/trace_events.h:419,
from include/trace/define_trace.h:102,
from drivers/cxl/core/trace.h:737,
from drivers/cxl/core/trace.c:8:
drivers/cxl/core/./trace.h:383:1: error: macro "__assign_str" passed 2
arguments, but takes just 1
This is with the patch applied on top of v6.9-8410-gff2632d7d08e.
So far that seems to be the only build failure.
Introduced with commit 6aec00139d3a8 ("cxl/core: Add region info to
cxl_general_media and cxl_dram events"). Guess we'll see more of those
towards the end of the commit window.
Guenter
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] compiler: Fix errors in Clang 17 ubsan checks.
On 5/17/24 14:59, Eelco Chaudron wrote: > > > On 16 May 2024, at 15:57, Mike Pattrick wrote: > >> This patch attempts to fix a large number of ubsan error messages that >> take the following form: >> >> lib/netlink-notifier.c:237:13: runtime error: call to function >> route_table_change through pointer to incorrect function type >> 'void (*)(const void *, void *)' >> >> In Clang 17 the undefined behaviour sanatizer check for function >> pointers was enabled by default, whereas it was previously disabled >> while compiling C code. These warnings are a false positive in the case >> of OVS, as our macros already check to make sure the function parameter >> is the correct size. >> >> So that check is disabled in the single function that is causing all of >> the errors. >> >> Signed-off-by: Mike Pattrick > > Thanks for fixing the naming. > > Acked-by: Eelco Chaudron Thanks, Mike, Jakob and Eelco! I fixed the comment spacing and moved the attribute to the same line with a return type as we normally do. With that, applied to all the branches down to 2.17. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4] lib: Assert for incorrect packet.
On 5/15/24 14:15, Amit Prakash Shukla wrote: > Packet that are not encapsulated but metadata of the packet contains > a offload flag set, will call dp_packet_inner_l4 to get TCP, UDP, SCTP > header pointers. dp_packet_inner_l4 for such packets would return NULL > as the inner offsets by-default are configured as UINT16_MAX. On > derefrencing such pointers, segfault is observed. > > Add assert check for packets with incorrect header or incorrect > offload flag set. > > Signed-off-by: Amit Prakash Shukla > --- > v2: > - Added Fixes tag and updated commit message. > > v3: > - Resolved review comment - added assert. > - Updated patch subject and commit message. > > v4: > - Fixed checkpatch warning. Thanks! I added a note to the commit message that the crash was not related to OVS logic and applied the change. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] table: Fix freeing global variable.
On 5/15/24 17:52, Simon Horman wrote:
> On Wed, May 15, 2024 at 02:14:13PM +0800, Yunjian Wang via dev wrote:
>> From: Pengfei Sun
>>
>> In function shash_replace_nocopy, argument to free() is the address
>> of a global variable (argument passed by function table_print_json__),
>> which is not memory allocated by malloc().
>>
>> ovsdb-client -f json monitor Open_vSwitch --timestamp
>>
>> ASan reports:
>> =
>> ==1443083==ERROR: AddressSanitizer: attempting free on address
>> which was not malloc()-ed: 0x00535980 in thread T0
>> #0 0xaf1c9eac in __interceptor_free (/usr/lib64/libasan.so.6+
>> 0xa4eac)
>> #1 0x4826e4 in json_destroy_object lib/json.c:445
>> #2 0x4826e4 in json_destroy__ lib/json.c:403
>> #3 0x4cc4e4 in table_print lib/table.c:633
>> #4 0x410650 in monitor_print_table ovsdb/ovsdb-client.c:1019
>> #5 0x410650 in monitor_print ovsdb/ovsdb-client.c:1040
>> #6 0x4110cc in monitor_print ovsdb/ovsdb-client.c:1030
>> #7 0x4110cc in do_monitor__ ovsdb/ovsdb-client.c:1503
>> #8 0x40743c in main ovsdb/ovsdb-client.c:283
>> #9 0xaeb50038 (/usr/lib64/libc.so.6+0x2b038)
>> #10 0xaeb50110 in __libc_start_main (/usr/lib64/libc.so.6+
>> 0x2b110)
>> #11 0x40906c in _start (/usr/local/bin/ovsdb-client+0x40906c)
>>
>> Fixes: cb139fa8b3a1 ("table: New function table_format() for formatting a
>> table as a string.")
>> Signed-off-by: Pengfei Sun
>
> Acked-by: Simon Horman
>
Thanks, Pengfei, Eelco and Simon!
Applied and backpotred down to 2.17.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] vlog: Destroy async_append first then close log_fd.
On 5/15/24 18:01, Simon Horman wrote: > On Wed, May 15, 2024 at 11:28:21AM +0800, hepeng via dev wrote: >> From: Peng He >> >> async_append stores log_fd, it should be destructed before log_fd >> is closed. >> >> Signed-off-by: Peng He > > Acked-by: Simon Horman > Applied to main and backported down to 2.17. Thanks! Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] compiler: Fix errors in Clang 17 ubsan checks.
On 16 May 2024, at 15:57, Mike Pattrick wrote: > This patch attempts to fix a large number of ubsan error messages that > take the following form: > > lib/netlink-notifier.c:237:13: runtime error: call to function > route_table_change through pointer to incorrect function type > 'void (*)(const void *, void *)' > > In Clang 17 the undefined behaviour sanatizer check for function > pointers was enabled by default, whereas it was previously disabled > while compiling C code. These warnings are a false positive in the case > of OVS, as our macros already check to make sure the function parameter > is the correct size. > > So that check is disabled in the single function that is causing all of > the errors. > > Signed-off-by: Mike Pattrick Thanks for fixing the naming. Acked-by: Eelco Chaudron ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] OVN technical community meeting - May 13th
On 5/14/24 12:29, Dumitru Ceara wrote: > Thanks, everyone, for attending the OVN community meeting yesterday! It > was a very interesting discussion about how to better integrate OVN and BGP! > > Here's the link to the recording: > https://youtu.be/k448ada9aFQ > > And the transcript of the chat: > https://drive.google.com/file/d/1VuF5l9wSR9z4rIH_LVF3PJBScj4Mb8JT > > I'd like to schedule a new session in 5 weeks as it seems there's still > quite some details worth while discussing about how to actually > implement the BGP+OVN integration. > > I was thinking of Monday, June 17th, 3PM UTC. I'll wait for a few days > for people to check and potentially suggest alternatives before sending > out an invite. > I got a request from Han Zhou to move the meeting to June 24th so I went ahead and sent out an invite for then. However, if people prefer other dates, please let me know. Meeting details: Date/Time: Monday, June 24th, 3PM UTC Link: meet.google.com/zns-gqsd-jdn Agenda: https://docs.google.com/document/d/1dG4GwcYOSs4uArPGtOoaP5tH4KCto-GH_C3tIXSnZZ8/edit?usp=meetingnotes Best regards, Dumitru ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] tracing/treewide: Remove second parameter of __assign_str()
On Thu, May 16, 2024 at 7:35 PM Steven Rostedt wrote: > > From: "Steven Rostedt (Google)" > > [ >This is a treewide change. I will likely re-create this patch again in >the second week of the merge window of v6.10 and submit it then. Hoping >to keep the conflicts that it will cause to a minimum. > ] > > With the rework of how the __string() handles dynamic strings where it > saves off the source string in field in the helper structure[1], the > assignment of that value to the trace event field is stored in the helper > value and does not need to be passed in again. > > This means that with: > > __string(field, mystring) > > Which use to be assigned with __assign_str(field, mystring), no longer > needs the second parameter and it is unused. With this, __assign_str() > will now only get a single parameter. > > There's over 700 users of __assign_str() and because coccinelle does not > handle the TRACE_EVENT() macro I ended up using the following sed script: > > git grep -l __assign_str | while read a ; do > sed -e 's/\(__assign_str([^,]*[^ ,]\) *,[^;]*/\1)/' $a > /tmp/test-file; > mv /tmp/test-file $a; > done > > I then searched for __assign_str() that did not end with ';' as those > were multi line assignments that the sed script above would fail to catch. > > Note, the same updates will need to be done for: > > __assign_str_len() > __assign_rel_str() > __assign_rel_str_len() > > I tested this with both an allmodconfig and an allyesconfig (build only for > both). > > [1] > https://lore.kernel.org/linux-trace-kernel/2024011442.634192...@goodmis.org/ > > Cc: Masami Hiramatsu > Cc: Mathieu Desnoyers > Cc: Linus Torvalds > Cc: Julia Lawall > Signed-off-by: Steven Rostedt (Google) Acked-by: Rafael J. Wysocki # for thermal ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] tracing/treewide: Remove second parameter of __assign_str()
On Thu, 16 May 2024 19:34:54 +0200, Steven Rostedt wrote: > > From: "Steven Rostedt (Google)" > > [ >This is a treewide change. I will likely re-create this patch again in >the second week of the merge window of v6.10 and submit it then. Hoping >to keep the conflicts that it will cause to a minimum. > ] > > With the rework of how the __string() handles dynamic strings where it > saves off the source string in field in the helper structure[1], the > assignment of that value to the trace event field is stored in the helper > value and does not need to be passed in again. > > This means that with: > > __string(field, mystring) > > Which use to be assigned with __assign_str(field, mystring), no longer > needs the second parameter and it is unused. With this, __assign_str() > will now only get a single parameter. > > There's over 700 users of __assign_str() and because coccinelle does not > handle the TRACE_EVENT() macro I ended up using the following sed script: > > git grep -l __assign_str | while read a ; do > sed -e 's/\(__assign_str([^,]*[^ ,]\) *,[^;]*/\1)/' $a > /tmp/test-file; > mv /tmp/test-file $a; > done > > I then searched for __assign_str() that did not end with ';' as those > were multi line assignments that the sed script above would fail to catch. > > Note, the same updates will need to be done for: > > __assign_str_len() > __assign_rel_str() > __assign_rel_str_len() > > I tested this with both an allmodconfig and an allyesconfig (build only for > both). > > [1] > https://lore.kernel.org/linux-trace-kernel/2024011442.634192...@goodmis.org/ > > Cc: Masami Hiramatsu > Cc: Mathieu Desnoyers > Cc: Linus Torvalds > Cc: Julia Lawall > Signed-off-by: Steven Rostedt (Google) For the sound part Acked-by: Takashi Iwai thanks, Takashi ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v9 2/2] ofproto-dpif-mirror: Add support for pre-selection filter.
On 1 May 2024, at 16:54, Mike Pattrick wrote:
> Currently a bridge mirror will collect all packets and tools like
> ovs-tcpdump can apply additional filters after they have already been
> duplicated by vswitchd. This can result in inefficient collection.
>
> This patch adds support to apply pre-selection to bridge mirrors, which
> can limit which packets are mirrored based on flow metadata. This
> significantly improves overall vswitchd performance during mirroring if
> only a subset of traffic is required.
>
> Signed-off-by: Mike Pattrick
> ---
> v8:
> - Corrected code from v7 related to sequence and in_port. Mirrors
>reject filters with an in_port set as this could cause confusion.
> - Combined ovsrcu pointers into a new struct, minimatch wasn't used
>because the minimatch_* functions didn't fit the usage here.
> - Added a test to check for modifying filters when partially
>overlapping flows already exist.
> - Corrected documentation.
> v9:
> - Explicitly cleared mirror_config.filter* when not set
> ---
> Documentation/ref/ovs-tcpdump.8.rst | 8 +-
> NEWS| 6 +
> lib/flow.h | 9 ++
> ofproto/ofproto-dpif-mirror.c | 104 +-
> ofproto/ofproto-dpif-mirror.h | 9 +-
> ofproto/ofproto-dpif-xlate.c| 15 ++-
> ofproto/ofproto-dpif.c | 12 +-
> ofproto/ofproto.h | 3 +
> tests/ofproto-dpif.at | 165
> utilities/ovs-tcpdump.in| 13 ++-
> vswitchd/bridge.c | 13 ++-
> vswitchd/vswitch.ovsschema | 7 +-
> vswitchd/vswitch.xml| 16 +++
> 13 files changed, 365 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/ref/ovs-tcpdump.8.rst
> b/Documentation/ref/ovs-tcpdump.8.rst
> index b9f8cdf6f..e21e61211 100644
> --- a/Documentation/ref/ovs-tcpdump.8.rst
> +++ b/Documentation/ref/ovs-tcpdump.8.rst
> @@ -61,8 +61,14 @@ Options
>
>If specified, mirror all ports (optional).
>
> +* ``--filter ``
> +
> + If specified, only mirror flows that match the provided OpenFlow filter.
> + The available fields are documented in ``ovs-fields(7)``.
> +
> See Also
>
>
> ``ovs-appctl(8)``, ``ovs-vswitchd(8)``, ``ovs-pcap(1)``,
> -``ovs-tcpundump(1)``, ``tcpdump(8)``, ``wireshark(8)``.
> +``ovs-fields(7)``, ``ovs-tcpundump(1)``, ``tcpdump(8)``,
> +``wireshark(8)``.
> diff --git a/NEWS b/NEWS
> index b92cec532..f3a4bf076 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -7,6 +7,12 @@ Post-v3.3.0
> - The primary development branch has been renamed from 'master' to 'main'.
> The OVS tree remains hosted on GitHub.
> https://github.com/openvswitch/ovs.git
> + - ovs-vsctl:
> + * Added a new filter column in the Mirror table which can be used to
> + apply filters to mirror ports.
> + - ovs-tcpdump:
> + * Added command line parameter --filter to enable filtering the flows
> + that are captured by tcpdump.
>
>
> v3.3.0 - 16 Feb 2024
> diff --git a/lib/flow.h b/lib/flow.h
> index 75a9be3c1..60ec4b0d7 100644
> --- a/lib/flow.h
> +++ b/lib/flow.h
> @@ -939,6 +939,15 @@ flow_union_with_miniflow(struct flow *dst, const struct
> miniflow *src)
> flow_union_with_miniflow_subset(dst, src, src->map);
> }
>
> +/* Perform a bitwise OR of minimask 'src' mask data with the equivalent
> + * fields in 'dst', storing the result in 'dst'. */
> +static inline void
> +flow_wildcards_union_with_minimask(struct flow_wildcards *dst,
> + const struct minimask *src)
> +{
> +flow_union_with_miniflow_subset(>masks, >masks,
> src->masks.map);
> +}
> +
> static inline bool is_ct_valid(const struct flow *flow,
> const struct flow_wildcards *mask,
> struct flow_wildcards *wc)
> diff --git a/ofproto/ofproto-dpif-mirror.c b/ofproto/ofproto-dpif-mirror.c
> index 4967ecc9a..6d89d13a5 100644
> --- a/ofproto/ofproto-dpif-mirror.c
> +++ b/ofproto/ofproto-dpif-mirror.c
> @@ -21,6 +21,7 @@
> #include "cmap.h"
> #include "hmapx.h"
> #include "ofproto.h"
> +#include "ofproto-dpif-trace.h"
> #include "vlan-bitmap.h"
> #include "openvswitch/vlog.h"
>
> @@ -48,6 +49,11 @@ struct mbundle {
> mirror_mask_t mirror_out; /* Mirrors that output to this mbundle. */
> };
>
> +struct filtermask {
> +struct miniflow *flow;
> +struct minimask *mask;
> +};
> +
> struct mirror {
> struct mbridge *mbridge;/* Owning ofproto. */
> size_t idx; /* In ofproto's "mirrors" array. */
> @@ -57,6 +63,10 @@ struct mirror {
> struct hmapx srcs; /* Contains "struct mbundle*"s. */
> struct hmapx dsts; /* Contains "struct mbundle*"s. */
>
> +/* Filter criteria. */
> +OVSRCU_TYPE(struct filtermask *) filter_mask;
> +char *filter_str;
> +
> /* This is accessed by handler threads assuming RCU protection (see
>
Re: [ovs-dev] [PATCH v9 1/2] ofproto-dpif-mirror: Reduce number of function parameters.
On 1 May 2024, at 16:54, Mike Pattrick wrote: > Previously the mirror_set() and mirror_get() functions took a large > number of parameters, which was inefficient and difficult to read and > extend. This patch moves most of the parameters into a struct. > > Signed-off-by: Mike Pattrick > Acked-by: Simon Horman > Acked-by: Eelco Chaudron > Signed-off-by: Mike Pattrick > --- Even with the additional changes this patch looks good to me. Acked-by: Eelco Chaudron ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] compiler: Fix errors in Clang 17 ubsan checks.
On 16.05.24 15:57, Mike Pattrick wrote: > This patch attempts to fix a large number of ubsan error messages that > take the following form: > > lib/netlink-notifier.c:237:13: runtime error: call to function > route_table_change through pointer to incorrect function type > 'void (*)(const void *, void *)' > > In Clang 17 the undefined behaviour sanatizer check for function > pointers was enabled by default, whereas it was previously disabled > while compiling C code. These warnings are a false positive in the case > of OVS, as our macros already check to make sure the function parameter > is the correct size. > > So that check is disabled in the single function that is causing all of > the errors. > > Signed-off-by: Mike Pattrick > --- > v2: Changed macro name > --- > include/openvswitch/compiler.h | 11 +++ > lib/ovs-rcu.c | 1 + > 2 files changed, 12 insertions(+) Thank you, Mike! This solves most issues I had when running OVS tests on Fedora Rawhide with Clang 18 and "-fsanitize=undefined". Acked-by: Jakob Meng ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2] northd: Fix an issue wrt mac binding aging.
On Thu, May 16, 2024 at 10:09 PM Indrajitt Valsaraj <
indrajitt.valsa...@nutanix.com> wrote:
>
> Issue:
> In case of a Logical_Router without mac_binding_age_threshold set or a
> Logical_Router with an incorrectly formatted mac_binding_threshold
> option, entries were not being purged from the Mac Binding table in
> SouthBound.
>
> This was because in the function `en_mac_binding_aging_run` in case of
> an invalid mac_binding_threshold entry or if mac_binding_threshold is
> not set we are returning from the loop instead of iterating through the
> remaining LRs. As a result, subsequent runs of the aging_waker node are
> also not scehduled and we end up not purging any MAC Bindings.
>
> Fix:
> This patch fixes this issue by changing the return to a continue so that
> we skip the current LR but continue processing for the remaining LRs.
>
> Fixes: 78851b6ffb58 ("Support CIDR-based MAC binding aging threshold.")
> Signed-off-by: Indrajitt Valsaraj
> Acked-by: Naveen Yerramneni
>
Hi Indrajitt,
Thanks for reporting and fixing the issue. Please see my comments below on
the test case.
> ---
> v1:
> - Addressed review comment from Ales
> v2:
> - Fix test failure
> ---
> northd/aging.c | 2 +-
> tests/ovn.at | 107 +++--
> 2 files changed, 77 insertions(+), 32 deletions(-)
>
> diff --git a/northd/aging.c b/northd/aging.c
> index b76963a2d..9685044e7 100644
> --- a/northd/aging.c
> +++ b/northd/aging.c
> @@ -421,7 +421,7 @@ en_mac_binding_aging_run(struct engine_node *node,
void *data OVS_UNUSED)
> if (!parse_aging_threshold(smap_get(>nbr->options,
> "mac_binding_age_threshold"),
> _config)) {
> -return;
> +continue;
> }
>
> aging_context_set_threshold(, _config);
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 486680649..5ab64ae9b 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -34414,10 +34414,15 @@ AT_CHECK([ovn-nbctl lsp-set-addresses ln_port
unknown])
> AT_CHECK([ovn-nbctl lsp-set-type ln_port localnet])
> AT_CHECK([ovn-nbctl lsp-set-options ln_port network_name=physnet1])
>
> -AT_CHECK([ovn-nbctl lsp-add public public-gw])
> -AT_CHECK([ovn-nbctl lsp-set-type public-gw router])
> -AT_CHECK([ovn-nbctl lsp-set-addresses public-gw 00:00:00:00:10:00
router])
> -AT_CHECK([ovn-nbctl lsp-set-options public-gw router-port=gw-public])
> +AT_CHECK([ovn-nbctl lsp-add public public-gw-1])
> +AT_CHECK([ovn-nbctl lsp-set-type public-gw-1 router])
> +AT_CHECK([ovn-nbctl lsp-set-addresses public-gw-1 00:00:00:00:10:00
router])
> +AT_CHECK([ovn-nbctl lsp-set-options public-gw-1 router-port=gw-1-public])
> +
> +AT_CHECK([ovn-nbctl lsp-add public public-gw-2])
> +AT_CHECK([ovn-nbctl lsp-set-type public-gw-2 router])
> +AT_CHECK([ovn-nbctl lsp-set-addresses public-gw-2 00:00:00:00:30:00
router])
> +AT_CHECK([ovn-nbctl lsp-set-options public-gw-2 router-port=gw-2-public])
>
> AT_CHECK([ovn-nbctl lsp-add internal internal-gw])
> AT_CHECK([ovn-nbctl lsp-set-type internal-gw router])
> @@ -34430,9 +34435,12 @@ AT_CHECK([ovn-nbctl lsp-set-addresses vif1
"00:00:00:00:20:10 192.168.20.10"])
> AT_CHECK([ovn-nbctl lsp-add internal vif2])
> AT_CHECK([ovn-nbctl lsp-set-addresses vif2 "00:00:00:00:20:20
192.168.20.20"])
>
> -AT_CHECK([ovn-nbctl lr-add gw])
> -AT_CHECK([ovn-nbctl lrp-add gw gw-public 00:00:00:00:10:00
192.168.10.1/24])
> -AT_CHECK([ovn-nbctl lrp-add gw gw-internal 00:00:00:00:20:00
192.168.20.1/24])
> +AT_CHECK([ovn-nbctl lr-add gw-1])
> +AT_CHECK([ovn-nbctl lrp-add gw-1 gw-1-public 00:00:00:00:10:00
192.168.10.1/24])
> +AT_CHECK([ovn-nbctl lrp-add gw-1 gw-internal 00:00:00:00:20:00
192.168.20.1/24])
> +
> +AT_CHECK([ovn-nbctl lr-add gw-2])
> +AT_CHECK([ovn-nbctl lrp-add gw-2 gw-2-public 00:00:00:00:30:00
192.168.10.2/24])
>
> sim_add hv1
> as hv1
> @@ -34500,21 +34508,27 @@ send_udp() {
> as $hv ovs-appctl netdev-dummy/receive $dev $packet
> }
> # Check if the option is not present by default
> -AT_CHECK([fetch_column nb:logical_router options name="gw" | grep -q
mac_binding_age_threshold], [1])
> +AT_CHECK([fetch_column nb:logical_router options name="gw-1" | grep -q
mac_binding_age_threshold], [1])
> +AT_CHECK([fetch_column nb:logical_router options name="gw-2" | grep -q
mac_binding_age_threshold], [1])
>
> # Send GARP to populate MAC binding table records
> send_garp hv1 ext1 10
> send_garp hv2 ext2 20
>
> -wait_row_count mac_binding 1 ip="192.168.10.10"
> -wait_row_count mac_binding 1 ip="192.168.10.20"
> +# Two rows present for each IP, one corresponding to each logical_port
> +wait_row_count mac_binding 2 ip="192.168.10.10"
> +wait_row_count mac_binding 2 ip="192.168.10.20"
>
> -dp_key=$(printf "0x%x" $(as hv1 fetch_column datapath tunnel_key
external_ids:name=gw))
> -port_key=$(printf "0x%x" $(as hv1 fetch_column port_binding tunnel_key
logical_port=gw-public))
> +dp_key_1=$(printf "0x%x" $(as hv1
Re: [ovs-dev] [PATCH ovn v2] northd: Fix an issue wrt mac binding aging.
On Fri, May 17, 2024 at 7:09 AM Indrajitt Valsaraj <
indrajitt.valsa...@nutanix.com> wrote:
> Issue:
> In case of a Logical_Router without mac_binding_age_threshold set or a
> Logical_Router with an incorrectly formatted mac_binding_threshold
> option, entries were not being purged from the Mac Binding table in
> SouthBound.
>
> This was because in the function `en_mac_binding_aging_run` in case of
> an invalid mac_binding_threshold entry or if mac_binding_threshold is
> not set we are returning from the loop instead of iterating through the
> remaining LRs. As a result, subsequent runs of the aging_waker node are
> also not scehduled and we end up not purging any MAC Bindings.
>
> Fix:
> This patch fixes this issue by changing the return to a continue so that
> we skip the current LR but continue processing for the remaining LRs.
>
> Fixes: 78851b6ffb58 ("Support CIDR-based MAC binding aging threshold.")
> Signed-off-by: Indrajitt Valsaraj
> Acked-by: Naveen Yerramneni
>
> ---
> v1:
> - Addressed review comment from Ales
> v2:
> - Fix test failure
> ---
> northd/aging.c | 2 +-
> tests/ovn.at | 107 +++--
> 2 files changed, 77 insertions(+), 32 deletions(-)
>
> diff --git a/northd/aging.c b/northd/aging.c
> index b76963a2d..9685044e7 100644
> --- a/northd/aging.c
> +++ b/northd/aging.c
> @@ -421,7 +421,7 @@ en_mac_binding_aging_run(struct engine_node *node,
> void *data OVS_UNUSED)
> if (!parse_aging_threshold(smap_get(>nbr->options,
> "mac_binding_age_threshold"),
> _config)) {
> -return;
> +continue;
> }
>
> aging_context_set_threshold(, _config);
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 486680649..5ab64ae9b 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -34414,10 +34414,15 @@ AT_CHECK([ovn-nbctl lsp-set-addresses ln_port
> unknown])
> AT_CHECK([ovn-nbctl lsp-set-type ln_port localnet])
> AT_CHECK([ovn-nbctl lsp-set-options ln_port network_name=physnet1])
>
> -AT_CHECK([ovn-nbctl lsp-add public public-gw])
> -AT_CHECK([ovn-nbctl lsp-set-type public-gw router])
> -AT_CHECK([ovn-nbctl lsp-set-addresses public-gw 00:00:00:00:10:00 router])
> -AT_CHECK([ovn-nbctl lsp-set-options public-gw router-port=gw-public])
> +AT_CHECK([ovn-nbctl lsp-add public public-gw-1])
> +AT_CHECK([ovn-nbctl lsp-set-type public-gw-1 router])
> +AT_CHECK([ovn-nbctl lsp-set-addresses public-gw-1 00:00:00:00:10:00
> router])
> +AT_CHECK([ovn-nbctl lsp-set-options public-gw-1 router-port=gw-1-public])
> +
> +AT_CHECK([ovn-nbctl lsp-add public public-gw-2])
> +AT_CHECK([ovn-nbctl lsp-set-type public-gw-2 router])
> +AT_CHECK([ovn-nbctl lsp-set-addresses public-gw-2 00:00:00:00:30:00
> router])
> +AT_CHECK([ovn-nbctl lsp-set-options public-gw-2 router-port=gw-2-public])
>
> AT_CHECK([ovn-nbctl lsp-add internal internal-gw])
> AT_CHECK([ovn-nbctl lsp-set-type internal-gw router])
> @@ -34430,9 +34435,12 @@ AT_CHECK([ovn-nbctl lsp-set-addresses vif1
> "00:00:00:00:20:10 192.168.20.10"])
> AT_CHECK([ovn-nbctl lsp-add internal vif2])
> AT_CHECK([ovn-nbctl lsp-set-addresses vif2 "00:00:00:00:20:20
> 192.168.20.20"])
>
> -AT_CHECK([ovn-nbctl lr-add gw])
> -AT_CHECK([ovn-nbctl lrp-add gw gw-public 00:00:00:00:10:00
> 192.168.10.1/24])
> -AT_CHECK([ovn-nbctl lrp-add gw gw-internal 00:00:00:00:20:00
> 192.168.20.1/24])
> +AT_CHECK([ovn-nbctl lr-add gw-1])
> +AT_CHECK([ovn-nbctl lrp-add gw-1 gw-1-public 00:00:00:00:10:00
> 192.168.10.1/24])
> +AT_CHECK([ovn-nbctl lrp-add gw-1 gw-internal 00:00:00:00:20:00
> 192.168.20.1/24])
> +
> +AT_CHECK([ovn-nbctl lr-add gw-2])
> +AT_CHECK([ovn-nbctl lrp-add gw-2 gw-2-public 00:00:00:00:30:00
> 192.168.10.2/24])
>
> sim_add hv1
> as hv1
> @@ -34500,21 +34508,27 @@ send_udp() {
> as $hv ovs-appctl netdev-dummy/receive $dev $packet
> }
> # Check if the option is not present by default
> -AT_CHECK([fetch_column nb:logical_router options name="gw" | grep -q
> mac_binding_age_threshold], [1])
> +AT_CHECK([fetch_column nb:logical_router options name="gw-1" | grep -q
> mac_binding_age_threshold], [1])
> +AT_CHECK([fetch_column nb:logical_router options name="gw-2" | grep -q
> mac_binding_age_threshold], [1])
>
> # Send GARP to populate MAC binding table records
> send_garp hv1 ext1 10
> send_garp hv2 ext2 20
>
> -wait_row_count mac_binding 1 ip="192.168.10.10"
> -wait_row_count mac_binding 1 ip="192.168.10.20"
> +# Two rows present for each IP, one corresponding to each logical_port
> +wait_row_count mac_binding 2 ip="192.168.10.10"
> +wait_row_count mac_binding 2 ip="192.168.10.20"
>
> -dp_key=$(printf "0x%x" $(as hv1 fetch_column datapath tunnel_key
> external_ids:name=gw))
> -port_key=$(printf "0x%x" $(as hv1 fetch_column port_binding tunnel_key
> logical_port=gw-public))
> +dp_key_1=$(printf "0x%x" $(as hv1 fetch_column datapath tunnel_key
> external_ids:name=gw-1))
> +port_key_1=$(printf
