Re: [ovs-dev] [PATCH v1] nedev-dpdk: Fix config with dpdk net_bonding offloads.

2024-04-30 Thread Ilya Maximets 
On 4/13/24 06:56, Jun Wang wrote:
>>On 4/12/24 08:29, Jun Wang wrote:
>>> If it's a DPDK net_bonding, it may cause
>>> offload-related configurations to take effect,
>>> leading to offload failure.
>>>
>>> /usr/share/openvswitch/scripts/ovs-ctl restart --no-ovs-vswitchd \
>>> --system-id=test
>>> ovs-vsctl --no-wait set open . external-ids:ovn-bridge-datapath-type\
>>> =netdev
>>>ovs-vsctl --no-wait set Open_vSwitch . other_config:dpdk-init=true
>>> ovs-vsctl --no-wait set Open_vSwitch . other_config:dpdk-socket-mem=\
>>> "4096,4096"
>>> ovs-vsctl --no-wait set Open_vSwitch . other_config:pmd-cpu-mask=\
>>> 0xff
>>> ovs-vsctl --no-wait set Open_vSwitch . other_config:dpdk-extra=\
>>> "-a :ca:00.0  -a :ca:00.1 --vdev net_bonding2494589023,\
>>> mode=4,member=:ca:00.0,member=:ca:00.1,xmit_policy=l34,\
>>> lacp_rate=fast"
>>> ovs-vswitchd unix:/var/run/openvswitch/db.sock -vconsole:emer \
>>> -vsyslog:err -vfile:info --mlockall --no-chdir \
>>> --log-file=/var/log/openvswitch/ovs-vswitchd.log \
>>> --pidfile=/var/run/openvswitch/ovs-vswitchd.pid --detach --monitor
>>> ovs-vsctl  add-br br-tun -- set bridge br-tun datapath_type=netdev
>>> ovs-vsctl --may-exist add-port br-tun dpdk_tun_port -- set Interface \
>>> dpdk_tun_port type=dpdk options:dpdk-devargs="net_bonding2494589023"
>>>
>>> {bus_info="bus_name=vdev", driver_name=net_bonding,
>>>  if_descr="DPDK 23.11.0 net_bonding", if_type="6",link_speed="20Gbps",
>>>  max_hash_mac_addrs="0", max_mac_addrs="16", max_rx_pktlen="1618",
>>>  max_rx_queues="1023", max_tx_queues="1023", max_vfs="0",
>>>  max_vmdq_pools="0", min_rx_bufsize="0", n_rxq="4", n_txq="9",
>>>  numa_id="0", port_no="2", rx-steering=rss, rx_csum_offload="false",
>>>  tx_geneve_tso_offload="false", tx_ip_csum_offload="false",
>>>  tx_out_ip_csum_offload="false", tx_out_udp_csum_offload="false",
>>>  tx_sctp_csum_offload="false", tx_tcp_csum_offload="false",
>>>  tx_tcp_seg_offload="false", tx_udp_csum_offload="false",
>>>  tx_vxlan_tso_offload="false"}
>>>
>>> Fixes: 5d11c47d3ebe ("userspace: Enable IP checksum offloading by default.")
>>>
>>> Signed-off-by: Jun Wang 
>>> ---
>>>  lib/netdev-dpdk.c | 75 
>>>+--
>>>  1 file changed, 45 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>>> index 2111f77..191c83d 100644
>>> --- a/lib/netdev-dpdk.c
>>> +++ b/lib/netdev-dpdk.c
>>> @@ -1294,15 +1294,10 @@ dpdk_eth_dev_init_rx_metadata(struct netdev_dpdk 
>>>*dev)
>>>      dev->rx_metadata_delivery_configured = true;
>>>  }
>>> 
>>> -static int
>>> -dpdk_eth_dev_init(struct netdev_dpdk *dev)
>>> -    OVS_REQUIRES(dev->mutex)
>>> +static void
>>> +dpdk_eth_offload_config(struct netdev_dpdk *dev,
>>> +   struct rte_eth_dev_info *info)
>>>  {
>>> -    struct rte_pktmbuf_pool_private *mbp_priv;
>>> -    struct rte_eth_dev_info info;
>>> -    struct rte_ether_addr eth_addr;
>>> -    int diag;
>>> -    int n_rxq, n_txq;
>>>      uint32_t rx_chksm_offload_capa = RTE_ETH_RX_OFFLOAD_UDP_CKSUM |
>>>                                       RTE_ETH_RX_OFFLOAD_TCP_CKSUM |
>>>                                       RTE_ETH_RX_OFFLOAD_IPV4_CKSUM;
>>> @@ -1319,16 +1314,14 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
>>>          dpdk_eth_dev_init_rx_metadata(dev);
>>>      }
>>> 
>>> -    rte_eth_dev_info_get(dev->port_id, &info);
>>> -
>>> -    if (strstr(info.driver_name, "vf") != NULL) {
>>> +    if (strstr(info->driver_name, "vf") != NULL) {
>>>          VLOG_INFO("Virtual function detected, HW_CRC_STRIP will be 
>>>enabled");
>>>          dev->hw_ol_features |= NETDEV_RX_HW_CRC_STRIP;
>>>      } else {
>>>          dev->hw_ol_features &= ~NETDEV_RX_HW_CRC_STRIP;
>>>      }
>>> 
>>> -    if ((info.rx_offload_capa & rx_chksm_offload_capa) !=
>>> +    if ((info->rx_offload_capa & rx_chksm_offload_capa) !=
>>>              rx_chksm_offload_capa) {
>>>          VLOG_WARN("Rx checksum offload is not supported on port "
>>>                    DPDK_PORT_ID_FMT, dev->port_id);
>>> @@ -1337,66 +1330,66 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
>>>          dev->hw_ol_features |= NETDEV_RX_CHECKSUM_OFFLOAD;
>>>      }
>>> 
>>> -    if (info.rx_offload_capa & RTE_ETH_RX_OFFLOAD_SCATTER) {
>>> +    if (info->rx_offload_capa & RTE_ETH_RX_OFFLOAD_SCATTER) {
>>>          dev->hw_ol_features |= NETDEV_RX_HW_SCATTER;
>>>      } else {
>>>          /* Do not warn on lack of scatter support */
>>>          dev->hw_ol_features &= ~NETDEV_RX_HW_SCATTER;
>>>      }
>>> 
>>> -    if (!strcmp(info.driver_name, "net_tap")) {
>>> +    if (!strcmp(info->driver_name, "net_tap")) {
>>>          /* FIXME: L4 checksum offloading is broken in DPDK net/tap driver.
>>>           * This workaround can be removed once the fix makes it to a DPDK
>>>           * LTS release used by OVS. */
>>>          VLOG_INFO("%s: disabled Tx L4 checksum offloads for a net/tap 
>>>port.",
>>>                    netdev_get_n

Re: [ovs-dev] [PATCH ovn v7 0/6] Correct tunnel ids exhaustion scenario.

2024-04-30 Thread Numan Siddique 
On Fri, Apr 26, 2024 at 3:04 PM Mark Michelson  wrote:
>
> Thanks for the update, Ihar.
>
> For the series,
> Acked-by: Mark Michelson 

Thanks Ihar, Vladislav and Mark.  I applied the series to the main branch.

Numan

>
> On 4/26/24 13:54, Ihar Hrachyshka wrote:
> > v2+ of the original test patch exposed a ubsan failure in port tunnel id
> > allocation code when tunnel id space is exhausted.
> >
> > This series fixes the ubsan failure (patches 1-2); then adjusts the
> > invalid scenario to trigger the originally intended failure mode - id
> > space exhausted (patch 3). Finally, it includes a number of smaller
> > cleanup patches in the area that simplify the allocation code somewhat.
> > (patches 4-6)
> >
> > I attempted to make each patch as simple as possible, to simplify
> > review. If you think it's too granular, let me know and I can squash
> > some.
> >
> > v1: initial version.
> > v2: cover both cases of hint = 0 and hint > max.
> > v3: reduce the number of ports to create in the hint > max scenario needed 
> > to trigger the problem.
> > v4: remove spurious lib/ovn-util.c change.
> > v5: ubsan fixes included.
> > v6: modify patch 5 to honor previously allocated tunnel ids.
> >  always detach op->list in build_ports (and never elsewhere.)
> > v7: simplify pb cleanup logic in ls_port_init.
> >
> > Ihar Hrachyshka (6):
> >northd: Don't cleanup op in ovn_port_allocate_key.
> >northd: Don't detach op->list when it wasn't used.
> >tests: Correct tunnel ids exhaustion scenario.
> >northd: Don't create pb in ls_port_init too early.
> >northd: Remove unused `sb` arg in ls_port_create.
> >northd: Remove unused nbrp arg in ls_port_reinit.
> >
> >   northd/northd.c | 58 ++---
> >   tests/ovn-northd.at | 43 ++---
> >   2 files changed, 73 insertions(+), 28 deletions(-)
> >
>
> ___
> 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] sparse: Add immintrin.h header.

2024-04-30 Thread Ilya Maximets 
On 4/30/24 16:41, Ales Musil wrote:
> 
> 
> On Tue, Apr 30, 2024 at 4:36 PM Ilya Maximets  > wrote:
> 
> Sparse doesn't understand _Float16 and some other types used by
> immintrin.h from GCC 13.  This breaks sparse builds with DPDK on
> Fedora 38+ and Ubuntu 24.04.
> 
> Add another sparse-specific header to workaround the problem.  We do
> need some of the functions and types defined in these headers, so we
> can't really stab out the whole header.  Carving out the main offenders
> instead by defining the inclusion guards.
> 
> This is fragile and depends on internals of immintrin and underlying
> headers, but I'm not sure what the better way to solve the issue
> would be.  This approach should be more or less portable between
> compilers, because it only defines a few specific variables.  We may
> have to add more as GCC headers change over time.
> 
> This fixes the build with a following config on F38 and Ubuntu 24.04:
> 
>   ./configure --enable-sparse --with-dpdk=yes --enable-Werror
> 
> Signed-off-by: Ilya Maximets  >
> ---
> 
> Looks good to me, thanks!
> 
> Acked-by: Ales Musil mailto:amu...@redhat.com>>

Thanks!  Applied to all 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] tc: Fix -Wgnu-variable-sized-type-not-at-end warning with Clang 18.

2024-04-30 Thread Ilya Maximets 
On 4/30/24 12:41, Ales Musil wrote:
> 
> 
> On Fri, Apr 26, 2024 at 7:44 PM Ilya Maximets  > wrote:
> 
> Clang 18.1.3-2.fc41 throws a warning:
> 
>   lib/tc.c:3060:25: error: field 'sel' with variable sized type
>             'struct tc_pedit_sel' not at the end of a struct or class is a
>             GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
> 
>    3060 |         struct tc_pedit sel;
>         |                         ^
> 
> Refactor the structure into a proper union to avoid the build failure.
> 
> Interestingly, clang 18.1.3-2.fc41 on Fedora throws a warning, but
> relatively the same version 18.1.3 (1) on Ubuntu 23.04 does not.
> 
> Signed-off-by: Ilya Maximets  >
> ---
>
> Looks good to me, thanks.
> 
> Acked-by: Ales Musil mailto:amu...@redhat.com>>


Thanks, Ales!  Applied to all 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] tests: Fix build failure with Clang 18 due to -Wformat-truncation.

2024-04-30 Thread Ilya Maximets 
On 4/30/24 13:12, Eelco Chaudron wrote:
> 
> 
> On 30 Apr 2024, at 11:53, Ilya Maximets wrote:
> 
>> On 4/26/24 18:35, Ilya Maximets wrote:
>>> Cirrus CI is broken on FreeBSD 13.3 due to clang version update.
>>> It now complains about snprintf truncation the same way GCC does:
>>>
>>>   tests/test-util.c:1129:16: error: 'snprintf' will always be truncated;
>>>   specified size is 5, but format string expands to at least 6
>>>   [-Werror,-Wformat-truncation]
>>>
>>>   1129 | ovs_assert(snprintf(s, 5, "abcde") == 5);
>>>|^
>>>
>>> Clang 17 on FreeBSD 14.0 works fine, but new Clang 18.1.4 on 13.3
>>> fails to build.
>>>
>>> Fix that by disabling Clang diagnostic the same way as we do for GCC.
>>>
>>> Unfortunately, the pragma's are compiler-specific, so cannot be
>>> combined, AFAIK.
>>>
>>> Signed-off-by: Ilya Maximets 
>>> ---
>>
>> Hi, Eelco and Simon.  May I ask you to take a look at this patch?
>>
>> It's blocking Cirrus CI and I don't think anything else should be
>> merged until CI is fixed.
>>
>> Best regards, Ilya Maximets.
> 
> Thanks Ilya, I agree! This looks good to me.
> 
> Acked-by: Eelco Chaudron 

Thanks, Eelco and Ales!  Applied to all 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 net-next 6/8] net:openvswitch: add psample support

2024-04-30 Thread Dan Carpenter 
Hi Adrian,

kernel test robot noticed the following build warnings:

url:
https://github.com/intel-lab-lkp/linux/commits/Adrian-Moreno/net-netlink-export-genl-private-pointer-getters/20240424-215821
base:   net-next/main
patch link:
https://lore.kernel.org/r/20240424135109.3524355-7-amorenoz%40redhat.com
patch subject: [PATCH net-next 6/8] net:openvswitch: add psample support
config: i386-randconfig-141-20240430 
(https://download.01.org/0day-ci/archive/20240430/202404300611.kjozl2ki-...@intel.com/config)
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot 
| Reported-by: Dan Carpenter 
| Closes: https://lore.kernel.org/r/202404300611.kjozl2ki-...@intel.com/

New smatch warnings:
net/openvswitch/actions.c:1097 sample() error: uninitialized symbol 'ret'.

vim +/ret +1097 net/openvswitch/actions.c

ccb1352e76cff0 Jesse Gross2011-10-25  1061  static int sample(struct 
datapath *dp, struct sk_buff *skb,
ccea74457bbdaf Neil McKee 2015-05-26  1062struct 
sw_flow_key *key, const struct nlattr *attr,
798c166173ffb5 andy zhou  2017-03-20  1063bool last)
ccb1352e76cff0 Jesse Gross2011-10-25  1064  {
ccc0b9e4657efd Adrian Moreno  2024-04-24  1065  const struct sample_arg 
*arg;
798c166173ffb5 andy zhou  2017-03-20  1066  struct nlattr 
*sample_arg;
798c166173ffb5 andy zhou  2017-03-20  1067  int rem = nla_len(attr);
ccc0b9e4657efd Adrian Moreno  2024-04-24  1068  struct nlattr *actions;
bef7f7567a104a andy zhou  2017-03-20  1069  bool clone_flow_key;
ccc0b9e4657efd Adrian Moreno  2024-04-24  1070  int ret;
ccb1352e76cff0 Jesse Gross2011-10-25  1071  
798c166173ffb5 andy zhou  2017-03-20  1072  /* The first action is 
always 'OVS_SAMPLE_ATTR_ARG'. */
798c166173ffb5 andy zhou  2017-03-20  1073  sample_arg = 
nla_data(attr);
798c166173ffb5 andy zhou  2017-03-20  1074  arg = 
nla_data(sample_arg);
798c166173ffb5 andy zhou  2017-03-20  1075  actions = 
nla_next(sample_arg, &rem);
e05176a3283822 Wenyu Zhang2015-08-05  1076  
798c166173ffb5 andy zhou  2017-03-20  1077  if ((arg->probability 
!= U32_MAX) &&
a251c17aa558d8 Jason A. Donenfeld 2022-10-05  1078  (!arg->probability 
|| get_random_u32() > arg->probability)) {
798c166173ffb5 andy zhou  2017-03-20  1079  if (last)
9d802da40b7c82 Adrian Moreno  2023-08-11  1080  
ovs_kfree_skb_reason(skb, OVS_DROP_LAST_ACTION);
ccb1352e76cff0 Jesse Gross2011-10-25  1081  return 0;
ccb1352e76cff0 Jesse Gross2011-10-25  1082  }
651887b0c22cff Simon Horman   2014-07-21  1083  
ccc0b9e4657efd Adrian Moreno  2024-04-24  1084  if (arg->flags & 
OVS_SAMPLE_ARG_FLAG_PSAMPLE) {
ccc0b9e4657efd Adrian Moreno  2024-04-24  1085  ret = 
ovs_psample_packet(dp, key, arg, skb);
ccc0b9e4657efd Adrian Moreno  2024-04-24  1086  if (ret)
ccc0b9e4657efd Adrian Moreno  2024-04-24  1087  return 
ret;
ccc0b9e4657efd Adrian Moreno  2024-04-24  1088  }
ccc0b9e4657efd Adrian Moreno  2024-04-24  1089  
ccc0b9e4657efd Adrian Moreno  2024-04-24  1090  if (nla_ok(actions, 
rem)) {
ccc0b9e4657efd Adrian Moreno  2024-04-24  1091  clone_flow_key 
= !(arg->flags & OVS_SAMPLE_ARG_FLAG_EXEC);
ccc0b9e4657efd Adrian Moreno  2024-04-24  1092  ret = 
clone_execute(dp, skb, key, 0, actions, rem, last,
bef7f7567a104a andy zhou  2017-03-20  1093  
clone_flow_key);
ccc0b9e4657efd Adrian Moreno  2024-04-24  1094  } else if (last) {
ccc0b9e4657efd Adrian Moreno  2024-04-24  1095  
ovs_kfree_skb_reason(skb, OVS_DROP_LAST_ACTION);

ret is uninitialized on this last path.

ccc0b9e4657efd Adrian Moreno  2024-04-24  1096  }
ccc0b9e4657efd Adrian Moreno  2024-04-24 @1097  return ret;
971427f353f3c4 Andy Zhou  2014-09-15  1098  }

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[ovs-dev] [PATCH ovn v3] controller: Track individual address set constants.

2024-04-30 Thread Ales Musil 
Instead of tracking address set per struct expr_constant_set track it
per individual struct expr_constant. This allows more fine grained
control for I-P processing of address sets in controller. It helps with
scenarios like matching on two address sets in one expression e.g.
"ip4.src == {$as1, $as2}". This allows any addition or removal of
individual adress from the set to be incrementally processed instead
of reprocessing all the flows.

This unfortunately doesn't help with the following flows:
"ip4.src == $as1 && ip4.dst == $as2"
"ip4.src == $as1 || ip4.dst == $as2"

The memory impact should be minimal as there is only increase of 8 bytes
per the struct expr_constant.

Reported-at: https://issues.redhat.com/browse/FDP-509
Signed-off-by: Ales Musil 
---
v3: Rebase on top of current main.
Address comments from Han:
- Adjust the comment for "lflow_handle_addr_set_update" to include remaning 
corner cases.
- Make sure that the flows are consistent between I-P and recompute.
v2: Rebase on top of current main.
Adjust the comment for I-P optimization.
---
 controller/lflow.c  |  7 ++-
 include/ovn/actions.h   |  2 +-
 include/ovn/expr.h  | 46 ++-
 lib/actions.c   | 20 -
 lib/expr.c  | 99 +
 tests/ovn-controller.at | 79 +---
 6 files changed, 153 insertions(+), 100 deletions(-)

diff --git a/controller/lflow.c b/controller/lflow.c
index 760ec0b41..06e839cbe 100644
--- a/controller/lflow.c
+++ b/controller/lflow.c
@@ -278,7 +278,7 @@ lflow_handle_changed_flows(struct lflow_ctx_in *l_ctx_in,
 }
 
 static bool
-as_info_from_expr_const(const char *as_name, const union expr_constant *c,
+as_info_from_expr_const(const char *as_name, const struct expr_constant *c,
 struct addrset_info *as_info)
 {
 as_info->name = as_name;
@@ -647,9 +647,8 @@ as_update_can_be_handled(const char *as_name, struct 
addr_set_diff *as_diff,
  *expressions/constants, usually because of disjunctions between
  *sub-expressions/constants, e.g.:
  *
+ *  ip.src == $as1 && ip.dst == $as2
  *  ip.src == $as1 || ip.dst == $as2
- *  ip.src == {$as1, $as2}
- *  ip.src == {$as1, ip1}
  *
  *All these could have been split into separate lflows.
  *
@@ -714,7 +713,7 @@ lflow_handle_addr_set_update(const char *as_name,
 if (as_diff->deleted) {
 struct addrset_info as_info;
 for (size_t i = 0; i < as_diff->deleted->n_values; i++) {
-union expr_constant *c = &as_diff->deleted->values[i];
+struct expr_constant *c = &as_diff->deleted->values[i];
 if (!as_info_from_expr_const(as_name, c, &as_info)) {
 continue;
 }
diff --git a/include/ovn/actions.h b/include/ovn/actions.h
index ae0864fdd..88cf4de79 100644
--- a/include/ovn/actions.h
+++ b/include/ovn/actions.h
@@ -241,7 +241,7 @@ struct ovnact_next {
 struct ovnact_load {
 struct ovnact ovnact;
 struct expr_field dst;
-union expr_constant imm;
+struct expr_constant imm;
 };
 
 /* OVNACT_MOVE, OVNACT_EXCHANGE. */
diff --git a/include/ovn/expr.h b/include/ovn/expr.h
index c48f82398..e54edb5bf 100644
--- a/include/ovn/expr.h
+++ b/include/ovn/expr.h
@@ -368,7 +368,7 @@ bool expr_relop_from_token(enum lex_type type, enum 
expr_relop *relop);
 struct expr {
 struct ovs_list node;   /* In parent EXPR_T_AND or EXPR_T_OR if any. */
 enum expr_type type;/* Expression type. */
-char *as_name;  /* Address set name. Null if it is not an
+const char *as_name;/* Address set name. Null if it is not an
address set. */
 
 union {
@@ -505,40 +505,42 @@ enum expr_constant_type {
 };
 
 /* A string or integer constant (one must know which from context). */
-union expr_constant {
-/* Integer constant.
- *
- * The width of a constant isn't always clear, e.g. if you write "1",
- * there's no way to tell whether you mean for that to be a 1-bit constant
- * or a 128-bit constant or somewhere in between. */
-struct {
-union mf_subvalue value;
-union mf_subvalue mask; /* Only initialized if 'masked'. */
-bool masked;
-
-enum lex_format format; /* From the constant's lex_token. */
-};
+struct expr_constant {
+const char *as_name;
 
-/* Null-terminated string constant. */
-char *string;
+union {
+/* Integer constant.
+ *
+ * The width of a constant isn't always clear, e.g. if you write "1",
+ * there's no way to tell whether you mean for that to be a 1-bit
+ * constant or a 128-bit constant or somewhere in between. */
+struct {
+union mf_subvalue value;
+union mf_subvalue mask; /* Only initialized if 'masked'. */
+bool masked;
+
+enum le

Re: [ovs-dev] [PATCH ovn v3] ci: Keep the container version pinned.

2024-04-30 Thread Mark Michelson 

Acked-by: Mark Michelson 

I pushed this to main and branch-24.03.

On 4/30/24 10:44, Ales Musil wrote:

The Ubuntu 24.04 brought some issues that are not really straight
forward to fix. Keep the Ubuntu version on 22.04 for now to keep
the CI working.

At the same time Fedora updated Clang to version 18, which is
throwing compilation error that need to be fixed in OvS first.

Signed-off-by: Ales Musil 
---
  utilities/containers/fedora/Dockerfile | 2 +-
  utilities/containers/ubuntu/Dockerfile | 2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/utilities/containers/fedora/Dockerfile 
b/utilities/containers/fedora/Dockerfile
index bf3c293fc..9b8386aae 100755
--- a/utilities/containers/fedora/Dockerfile
+++ b/utilities/containers/fedora/Dockerfile
@@ -1,4 +1,4 @@
-FROM quay.io/fedora/fedora:latest
+FROM quay.io/fedora/fedora:39
  
  ARG CONTAINERS_PATH
  
diff --git a/utilities/containers/ubuntu/Dockerfile b/utilities/containers/ubuntu/Dockerfile

index 1371b3f70..ac1e6a5bf 100755
--- a/utilities/containers/ubuntu/Dockerfile
+++ b/utilities/containers/ubuntu/Dockerfile
@@ -1,4 +1,4 @@
-FROM registry.hub.docker.com/library/ubuntu:latest
+FROM registry.hub.docker.com/library/ubuntu:22.04
  
  ARG CONTAINERS_PATH
  


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

Re: [ovs-dev] [PATCH ovn v3] ci: Keep the container version pinned.

2024-04-30 Thread Ilya Maximets 
On 4/30/24 16:44, Ales Musil wrote:
> The Ubuntu 24.04 brought some issues that are not really straight
> forward to fix. Keep the Ubuntu version on 22.04 for now to keep
> the CI working.
> 
> At the same time Fedora updated Clang to version 18, which is
> throwing compilation error that need to be fixed in OvS first.
> 
> Signed-off-by: Ales Musil 
> ---
>  utilities/containers/fedora/Dockerfile | 2 +-
>  utilities/containers/ubuntu/Dockerfile | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/utilities/containers/fedora/Dockerfile 
> b/utilities/containers/fedora/Dockerfile
> index bf3c293fc..9b8386aae 100755
> --- a/utilities/containers/fedora/Dockerfile
> +++ b/utilities/containers/fedora/Dockerfile
> @@ -1,4 +1,4 @@
> -FROM quay.io/fedora/fedora:latest
> +FROM quay.io/fedora/fedora:39
>  
>  ARG CONTAINERS_PATH
>  
> diff --git a/utilities/containers/ubuntu/Dockerfile 
> b/utilities/containers/ubuntu/Dockerfile
> index 1371b3f70..ac1e6a5bf 100755
> --- a/utilities/containers/ubuntu/Dockerfile
> +++ b/utilities/containers/ubuntu/Dockerfile
> @@ -1,4 +1,4 @@
> -FROM registry.hub.docker.com/library/ubuntu:latest
> +FROM registry.hub.docker.com/library/ubuntu:22.04
>  
>  ARG CONTAINERS_PATH
>  

I think, we should also pin the version in build-linux-rpm
job in .github/workflows/test.yml.

Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovn v3] ci: Keep the container version pinned.

2024-04-30 Thread Ales Musil 
The Ubuntu 24.04 brought some issues that are not really straight
forward to fix. Keep the Ubuntu version on 22.04 for now to keep
the CI working.

At the same time Fedora updated Clang to version 18, which is
throwing compilation error that need to be fixed in OvS first.

Signed-off-by: Ales Musil 
---
 utilities/containers/fedora/Dockerfile | 2 +-
 utilities/containers/ubuntu/Dockerfile | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/utilities/containers/fedora/Dockerfile 
b/utilities/containers/fedora/Dockerfile
index bf3c293fc..9b8386aae 100755
--- a/utilities/containers/fedora/Dockerfile
+++ b/utilities/containers/fedora/Dockerfile
@@ -1,4 +1,4 @@
-FROM quay.io/fedora/fedora:latest
+FROM quay.io/fedora/fedora:39
 
 ARG CONTAINERS_PATH
 
diff --git a/utilities/containers/ubuntu/Dockerfile 
b/utilities/containers/ubuntu/Dockerfile
index 1371b3f70..ac1e6a5bf 100755
--- a/utilities/containers/ubuntu/Dockerfile
+++ b/utilities/containers/ubuntu/Dockerfile
@@ -1,4 +1,4 @@
-FROM registry.hub.docker.com/library/ubuntu:latest
+FROM registry.hub.docker.com/library/ubuntu:22.04
 
 ARG CONTAINERS_PATH
 
-- 
2.44.0

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

Re: [ovs-dev] [PATCH] sparse: Add immintrin.h header.

2024-04-30 Thread Ales Musil 
On Tue, Apr 30, 2024 at 4:36 PM Ilya Maximets  wrote:

> Sparse doesn't understand _Float16 and some other types used by
> immintrin.h from GCC 13.  This breaks sparse builds with DPDK on
> Fedora 38+ and Ubuntu 24.04.
>
> Add another sparse-specific header to workaround the problem.  We do
> need some of the functions and types defined in these headers, so we
> can't really stab out the whole header.  Carving out the main offenders
> instead by defining the inclusion guards.
>
> This is fragile and depends on internals of immintrin and underlying
> headers, but I'm not sure what the better way to solve the issue
> would be.  This approach should be more or less portable between
> compilers, because it only defines a few specific variables.  We may
> have to add more as GCC headers change over time.
>
> This fixes the build with a following config on F38 and Ubuntu 24.04:
>
>   ./configure --enable-sparse --with-dpdk=yes --enable-Werror
>
> Signed-off-by: Ilya Maximets 
> ---
>  include/sparse/automake.mk |  1 +
>  include/sparse/immintrin.h | 30 ++
>  2 files changed, 31 insertions(+)
>  create mode 100644 include/sparse/immintrin.h
>
> diff --git a/include/sparse/automake.mk b/include/sparse/automake.mk
> index c1229870b..45e6202c5 100644
> --- a/include/sparse/automake.mk
> +++ b/include/sparse/automake.mk
> @@ -1,5 +1,6 @@
>  noinst_HEADERS += \
>  include/sparse/rte_byteorder.h \
> +include/sparse/immintrin.h \
>  include/sparse/xmmintrin.h \
>  include/sparse/arpa/inet.h \
>  include/sparse/bits/floatn.h \
> diff --git a/include/sparse/immintrin.h b/include/sparse/immintrin.h
> new file mode 100644
> index 0..dd742be9f
> --- /dev/null
> +++ b/include/sparse/immintrin.h
> @@ -0,0 +1,30 @@
> +/* Copyright (c) 2024 Red Hat, Inc.
> + *
> + * 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.
> + */
> +
> +#ifndef __CHECKER__
> +#error "Use this header only with sparse.  It is not a correct
> implementation."
> +#endif
> +
> +/* Sparse doesn't know some types used by AVX512 and some other headers.
> + * Mark those headers as already included to avoid failures.  This is
> fragile,
> + * so may need adjustments with compiler changes. */
> +#define _AVX512BF16INTRIN_H_INCLUDED
> +#define _AVX512BF16VLINTRIN_H_INCLUDED
> +#define _AVXNECONVERTINTRIN_H_INCLUDED
> +#define _KEYLOCKERINTRIN_H_INCLUDED
> +#define __AVX512FP16INTRIN_H_INCLUDED
> +#define __AVX512FP16VLINTRIN_H_INCLUDED
> +
> +#include_next 
> --
> 2.44.0
>
>
Looks good to me, thanks!

Acked-by: Ales Musil 

-- 

Ales Musil

Senior Software Engineer - OVN Core

Red Hat EMEA 

amu...@redhat.com

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

[ovs-dev] [PATCH] sparse: Add immintrin.h header.

2024-04-30 Thread Ilya Maximets 
Sparse doesn't understand _Float16 and some other types used by
immintrin.h from GCC 13.  This breaks sparse builds with DPDK on
Fedora 38+ and Ubuntu 24.04.

Add another sparse-specific header to workaround the problem.  We do
need some of the functions and types defined in these headers, so we
can't really stab out the whole header.  Carving out the main offenders
instead by defining the inclusion guards.

This is fragile and depends on internals of immintrin and underlying
headers, but I'm not sure what the better way to solve the issue
would be.  This approach should be more or less portable between
compilers, because it only defines a few specific variables.  We may
have to add more as GCC headers change over time.

This fixes the build with a following config on F38 and Ubuntu 24.04:

  ./configure --enable-sparse --with-dpdk=yes --enable-Werror

Signed-off-by: Ilya Maximets 
---
 include/sparse/automake.mk |  1 +
 include/sparse/immintrin.h | 30 ++
 2 files changed, 31 insertions(+)
 create mode 100644 include/sparse/immintrin.h

diff --git a/include/sparse/automake.mk b/include/sparse/automake.mk
index c1229870b..45e6202c5 100644
--- a/include/sparse/automake.mk
+++ b/include/sparse/automake.mk
@@ -1,5 +1,6 @@
 noinst_HEADERS += \
 include/sparse/rte_byteorder.h \
+include/sparse/immintrin.h \
 include/sparse/xmmintrin.h \
 include/sparse/arpa/inet.h \
 include/sparse/bits/floatn.h \
diff --git a/include/sparse/immintrin.h b/include/sparse/immintrin.h
new file mode 100644
index 0..dd742be9f
--- /dev/null
+++ b/include/sparse/immintrin.h
@@ -0,0 +1,30 @@
+/* Copyright (c) 2024 Red Hat, Inc.
+ *
+ * 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.
+ */
+
+#ifndef __CHECKER__
+#error "Use this header only with sparse.  It is not a correct implementation."
+#endif
+
+/* Sparse doesn't know some types used by AVX512 and some other headers.
+ * Mark those headers as already included to avoid failures.  This is fragile,
+ * so may need adjustments with compiler changes. */
+#define _AVX512BF16INTRIN_H_INCLUDED
+#define _AVX512BF16VLINTRIN_H_INCLUDED
+#define _AVXNECONVERTINTRIN_H_INCLUDED
+#define _KEYLOCKERINTRIN_H_INCLUDED
+#define __AVX512FP16INTRIN_H_INCLUDED
+#define __AVX512FP16VLINTRIN_H_INCLUDED
+
+#include_next 
-- 
2.44.0

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

Re: [ovs-dev] [PATCH ovn v2] ci: Keep the the container version pinned.

2024-04-30 Thread Ales Musil 
On Tue, Apr 30, 2024 at 3:31 PM Ales Musil  wrote:

> The Ubuntu 24.04 brought some issues that are not realyl stright
>

typo: s/realyl stright/really straight :(

forward to fix. Keep the Ubuntu version on 22.04 for now to keep
> the CI working.
>
> At the same time Fedora updated Clang to version 18, which is
> throwing compilation error that need to be fixed in OvS first.
>
> Signed-off-by: Ales Musil 
> ---
>  utilities/containers/fedora/Dockerfile | 2 +-
>  utilities/containers/ubuntu/Dockerfile | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/utilities/containers/fedora/Dockerfile
> b/utilities/containers/fedora/Dockerfile
> index bf3c293fc..9b8386aae 100755
> --- a/utilities/containers/fedora/Dockerfile
> +++ b/utilities/containers/fedora/Dockerfile
> @@ -1,4 +1,4 @@
> -FROM quay.io/fedora/fedora:latest
> +FROM quay.io/fedora/fedora:39
>
>  ARG CONTAINERS_PATH
>
> diff --git a/utilities/containers/ubuntu/Dockerfile
> b/utilities/containers/ubuntu/Dockerfile
> index 1371b3f70..ac1e6a5bf 100755
> --- a/utilities/containers/ubuntu/Dockerfile
> +++ b/utilities/containers/ubuntu/Dockerfile
> @@ -1,4 +1,4 @@
> -FROM registry.hub.docker.com/library/ubuntu:latest
> +FROM registry.hub.docker.com/library/ubuntu:22.04
>
>  ARG CONTAINERS_PATH
>
> --
> 2.44.0
>
>

-- 

Ales Musil

Senior Software Engineer - OVN Core

Red Hat EMEA 

amu...@redhat.com

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

[ovs-dev] [PATCH ovn v2] ci: Keep the the container version pinned.

2024-04-30 Thread Ales Musil 
The Ubuntu 24.04 brought some issues that are not realyl stright
forward to fix. Keep the Ubuntu version on 22.04 for now to keep
the CI working.

At the same time Fedora updated Clang to version 18, which is
throwing compilation error that need to be fixed in OvS first.

Signed-off-by: Ales Musil 
---
 utilities/containers/fedora/Dockerfile | 2 +-
 utilities/containers/ubuntu/Dockerfile | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/utilities/containers/fedora/Dockerfile 
b/utilities/containers/fedora/Dockerfile
index bf3c293fc..9b8386aae 100755
--- a/utilities/containers/fedora/Dockerfile
+++ b/utilities/containers/fedora/Dockerfile
@@ -1,4 +1,4 @@
-FROM quay.io/fedora/fedora:latest
+FROM quay.io/fedora/fedora:39
 
 ARG CONTAINERS_PATH
 
diff --git a/utilities/containers/ubuntu/Dockerfile 
b/utilities/containers/ubuntu/Dockerfile
index 1371b3f70..ac1e6a5bf 100755
--- a/utilities/containers/ubuntu/Dockerfile
+++ b/utilities/containers/ubuntu/Dockerfile
@@ -1,4 +1,4 @@
-FROM registry.hub.docker.com/library/ubuntu:latest
+FROM registry.hub.docker.com/library/ubuntu:22.04
 
 ARG CONTAINERS_PATH
 
-- 
2.44.0

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

Re: [ovs-dev] [PATCH] netdev-tc-offloads: Don't offload header modification on ip fragments.

2024-04-30 Thread Eelco Chaudron 
Greetings, Intel team!

The self-test conducted as part of this patch has revealed an issue with the 
AVX512 checksumming code. Since it was agreed upon that your team would 
maintain this code upon its inclusion, could you please review the problem and 
provide a patch?

Details on the problem can be found in this mail link:

  https://mail.openvswitch.org/pipermail/ovs-build/2024-April/038590.html

Cheers,

Eelco

On 29 Apr 2024, at 16:48, Eelco Chaudron wrote:

> While offloading header modifications to TC, OVS is using {TCA_PEDIT} +
> {TCA_CSUM} combination as that it the only way to represent header
> rewrite.  However, {TCA_CSUM} is unable to calculate L4 checksums for
> IP fragments.
>
> Since TC already applies fragmentation bit masking, this patch simply
> needs to prevent these packets from being processed through TC.
>
> Signed-off-by: Eelco Chaudron 
> ---
>  lib/netdev-offload-tc.c | 32 +
>  tests/system-traffic.at | 62 +
>  2 files changed, 94 insertions(+)
>
> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> index 921d52317..7e915d419 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
> @@ -1488,6 +1488,30 @@ parse_put_flow_ct_action(struct tc_flower *flower,
>  return 0;
>  }
>
> +static bool
> +will_tc_add_l4_checksum(struct tc_flower *flower, int type)
> +{
> +/* This function returns true if the tc layer will add a l4 checksum 
> action
> + * for this set action. Refer to the csum_update_flag() function for
> + * detailed logic. Note that even the kernel only supports updating TCP,
> + * UDP and ICMPv6. */
> +switch (type) {
> +case OVS_KEY_ATTR_IPV4:
> +case OVS_KEY_ATTR_IPV6:
> +case OVS_KEY_ATTR_TCP:
> +case OVS_KEY_ATTR_UDP:
> +switch (flower->key.ip_proto) {
> +case IPPROTO_TCP:
> +case IPPROTO_UDP:
> +case IPPROTO_ICMPV6:
> +case IPPROTO_UDPLITE:
> +return true;
> +}
> +break;
> +}
> +return false;
> +}
> +
>  static int
>  parse_put_flow_set_masked_action(struct tc_flower *flower,
>   struct tc_action *action,
> @@ -1520,6 +1544,14 @@ parse_put_flow_set_masked_action(struct tc_flower 
> *flower,
>  return EOPNOTSUPP;
>  }
>
> +if (flower->key.flags & TCA_FLOWER_KEY_FLAGS_IS_FRAGMENT
> +&& will_tc_add_l4_checksum(flower, type)) {
> +VLOG_DBG_RL(&rl, "set action type %d not supported on fragments "
> +"due to checksum limitation", type);
> +ofpbuf_uninit(&set_buf);
> +return EOPNOTSUPP;
> +}
> +
>  for (i = 0; i < ARRAY_SIZE(set_flower_map[type]); i++) {
>  struct netlink_field *f = &set_flower_map[type][i];
>
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index bd7647cbe..d06d2f66a 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -8977,3 +8977,65 @@ OVS_WAIT_UNTIL([cat p2.pcap | grep -E "0x0050: * 
> * *5002 *2000 *b85e *00
>
>  OVS_TRAFFIC_VSWITCHD_STOP
>  AT_CLEANUP
> +
> +
> +AT_SETUP([datapath - IP mod_nw_src/set_field on fragments])
> +AT_SKIP_IF([test $HAVE_TCPDUMP = no])
> +OVS_TRAFFIC_VSWITCHD_START()
> +
> +ADD_NAMESPACES(at_ns0, at_ns1)
> +
> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24", 36:b1:ee:7c:01:03)
> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24", 36:b1:ee:7c:01:02)
> +
> +AT_DATA([flows.txt], [dnl
> +  in_port=ovs-p0,ip,nw_src=10.1.1.1 actions=mod_nw_src=11.1.1.1,ovs-p1
> +  in_port=ovs-p0,ipv6,ipv6_src=fc00::1 
> actions=set_field:fc00::100->ipv6_src,ovs-p1
> +])
> +
> +AT_CHECK([ovs-ofctl del-flows br0])
> +AT_CHECK([ovs-ofctl -Oopenflow13 add-flows br0 flows.txt])
> +
> +NETNS_DAEMONIZE([at_ns1], [tcpdump -l -nn -xx -U -i p1 > p1.out],
> +  [tcpdump.pid])
> +sleep 1
> +
> +dnl We send each packet multiple times, ones for learning which will flow
> +dnl through the used datapath for learning, and the others will go through 
> the
> +dnl actuall datapath.
> +for i in 1 2 3 4 5; do
> +  NS_CHECK_EXEC([at_ns0], [$PYTHON3 $srcdir/sendpkt.py p0 \
> +36 b1 ee 7c 01 02 36 b1 ee 7c 01 03 08 00 45 00 00 26 00 01 20 00 40 11 \
> +44 c2 0a 01 01 01 0a 01 01 02 0b c4 08 84 00 26 e9 64 01 02 03 04 05 06 \
> +07 08 09 0a > /dev/null])
> +done
> +
> +OVS_WAIT_UNTIL([test $(grep -c -E \
> +  "0x:  *36b1 *ee7c *0102 *36b1 *ee7c *0103 *0800 *4500" p1.out) -eq 5])
> +OVS_WAIT_UNTIL([test $(grep -c -E \
> +  "0x0010:  *0026 *0001 *2000 *4011 *43c2 *0b01 *0101 *0a01" p1.out) -eq 5])
> +OVS_WAIT_UNTIL([test $(grep -c -E \
> +  "0x0020:  *0102 *0bc4 *0884 *0026 *e864 *0102 *0304 *0506" p1.out) -eq 5])
> +
> +dnl Repeat similar test with IPv6.
> +for i in 1 2 3 4 5; do
> +  NS_CHECK_EXEC([at_ns0], [$PYTHON3 $srcdir/sendpkt.py p0 \
> +36 b1 ee 7c 01 02 36 b1 ee 7c 01 03 86 dd 60 00 00 00 00 18 2c 40 fc 00 \
> +00 00 00 00 00 00 00 00 00 00 00 00 00 01 fc 00 00 00 00 00 00 00 00 00 \
> +00

Re: [ovs-dev] [PATCH] tests: Fix build failure with Clang 18 due to -Wformat-truncation.

2024-04-30 Thread Eelco Chaudron 



On 30 Apr 2024, at 11:53, Ilya Maximets wrote:

> On 4/26/24 18:35, Ilya Maximets wrote:
>> Cirrus CI is broken on FreeBSD 13.3 due to clang version update.
>> It now complains about snprintf truncation the same way GCC does:
>>
>>   tests/test-util.c:1129:16: error: 'snprintf' will always be truncated;
>>   specified size is 5, but format string expands to at least 6
>>   [-Werror,-Wformat-truncation]
>>
>>   1129 | ovs_assert(snprintf(s, 5, "abcde") == 5);
>>|^
>>
>> Clang 17 on FreeBSD 14.0 works fine, but new Clang 18.1.4 on 13.3
>> fails to build.
>>
>> Fix that by disabling Clang diagnostic the same way as we do for GCC.
>>
>> Unfortunately, the pragma's are compiler-specific, so cannot be
>> combined, AFAIK.
>>
>> Signed-off-by: Ilya Maximets 
>> ---
>
> Hi, Eelco and Simon.  May I ask you to take a look at this patch?
>
> It's blocking Cirrus CI and I don't think anything else should be
> merged until CI is fixed.
>
> Best regards, Ilya Maximets.

Thanks Ilya, I agree! This looks good to me.

Acked-by: Eelco Chaudron 


>>  tests/test-util.c | 13 ++---
>>  1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/tests/test-util.c b/tests/test-util.c
>> index 7d899fbbf..5d88d38f2 100644
>> --- a/tests/test-util.c
>> +++ b/tests/test-util.c
>> @@ -1116,12 +1116,16 @@ test_snprintf(struct ovs_cmdl_context *ctx 
>> OVS_UNUSED)
>>  {
>>  char s[16];
>>
>> +/* GCC 7+ and Clang 18+ warn about the following calls that truncate
>> + * a string using snprintf().  We're testing that truncation works
>> + * properly, so temporarily disable the warning. */
>>  #if __GNUC__ >= 7
>> -/* GCC 7+ warns about the following calls that truncate a string using
>> - * snprintf().  We're testing that truncation works properly, so
>> - * temporarily disable the warning. */
>>  #pragma GCC diagnostic push
>>  #pragma GCC diagnostic ignored "-Wformat-truncation"
>> +#endif
>> +#if __clang_major__ >= 18
>> +#pragma clang diagnostic push
>> +#pragma clang diagnostic ignored "-Wformat-truncation"
>>  #endif
>>  ovs_assert(snprintf(s, 4, "abcde") == 5);
>>  ovs_assert(!strcmp(s, "abc"));
>> @@ -1130,6 +1134,9 @@ test_snprintf(struct ovs_cmdl_context *ctx OVS_UNUSED)
>>  ovs_assert(!strcmp(s, "abcd"));
>>  #if __GNUC__ >= 7
>>  #pragma GCC diagnostic pop
>> +#endif
>> +#if __clang_major__ >= 18
>> +#pragma clang diagnostic pop
>>  #endif
>>
>>  ovs_assert(snprintf(s, 6, "abcde") == 5);

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

[ovs-dev] [PATCH ovn] ci: Keep the Ubuntu container on 22.04.

2024-04-30 Thread Ales Musil 
The Ubuntu 24.04 brought some issues that are not realyl stright
forward to fix. Keep the Ubuntu version on 22.04 for now to keep
the CI working.

Signed-off-by: Ales Musil 
---
 utilities/containers/ubuntu/Dockerfile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/utilities/containers/ubuntu/Dockerfile 
b/utilities/containers/ubuntu/Dockerfile
index 1371b3f70..ac1e6a5bf 100755
--- a/utilities/containers/ubuntu/Dockerfile
+++ b/utilities/containers/ubuntu/Dockerfile
@@ -1,4 +1,4 @@
-FROM registry.hub.docker.com/library/ubuntu:latest
+FROM registry.hub.docker.com/library/ubuntu:22.04
 
 ARG CONTAINERS_PATH
 
-- 
2.44.0

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

Re: [ovs-dev] [PATCH ovn v2] controller: Track individual address set constants.

2024-04-30 Thread Ales Musil 
On Mon, Apr 29, 2024 at 11:01 PM Han Zhou  wrote:

> On Fri, Apr 12, 2024 at 8:20 AM Ales Musil  wrote:
> >
> > Instead of tracking address set per struct expr_constant_set track it
> > per individual struct expr_constant. This allows more fine grained
> > control for I-P processing of address sets in controller. It helps with
> > scenarios like matching on two address sets in one expression e.g.
> > "ip4.src == {$as1, $as2}". This allows any addition or removal of
> > individual adress from the set to be incrementally processed instead
> > of reprocessing all the flows.
> >
> > This unfortunately doesn't help with the following flows:
> > "ip4.src == $as1 && ip4.dst == $as2"
> > "ip4.src == $as1 || ip4.dst == $as2"
> >
> > The memory impact should be minimal as there is only increase of 8 bytes
> > per the struct expr_constant.
> >
> > Reported-at: https://issues.redhat.com/browse/FDP-509
> > Signed-off-by: Ales Musil 
> > ---
> > v2: Rebase on top of current main.
> > Adjust the comment for I-P optimization.
>
> Thanks Ales for v2, and sorry for taking so long to review. Please see my
> comments below.
>
>
Hi Han,

thank you for the review.


>
> > ---
> >  controller/lflow.c  |  4 +-
> >  include/ovn/actions.h   |  2 +-
> >  include/ovn/expr.h  | 46 ++-
> >  lib/actions.c   | 20 -
> >  lib/expr.c  | 98 -
> >  tests/ovn-controller.at | 14 +++---
> >  6 files changed, 84 insertions(+), 100 deletions(-)
> >
> > diff --git a/controller/lflow.c b/controller/lflow.c
>
> In lflow.c, I noticed that the comments for the function
> lflow_handle_addr_set_update needs to be updated, because these are not
> true any more, and also better to mention what's still not supported such
> as the examples in your commit message.
> 
>  *  - The sub expression of the address set is combined with other
> sub-
>  *expressions/constants, usually because of disjunctions between
>
>  *sub-expressions/constants, e.g.:
>  *
>  *  ip.src == $as1 || ip.dst == $as2
>
>  *  ip.src == {$as1, $as2}
>
>  *  ip.src == {$as1, ip1}
>
>  *
>  *All these could have been split into separate lflows.
> 
>

Good point, I'll update that comment in v3 to reflect this.


>
> > index 895d17d19..730dc879d 100644
> > --- a/controller/lflow.c
> > +++ b/controller/lflow.c
> > @@ -278,7 +278,7 @@ lflow_handle_changed_flows(struct lflow_ctx_in
> *l_ctx_in,
> >  }
> >
> >  static bool
> > -as_info_from_expr_const(const char *as_name, const union expr_constant
> *c,
> > +as_info_from_expr_const(const char *as_name, const struct expr_constant
> *c,
> >  struct addrset_info *as_info)
> >  {
> >  as_info->name = as_name;
> > @@ -714,7 +714,7 @@ lflow_handle_addr_set_update(const char *as_name,
> >  if (as_diff->deleted) {
> >  struct addrset_info as_info;
> >  for (size_t i = 0; i < as_diff->deleted->n_values; i++) {
> > -union expr_constant *c = &as_diff->deleted->values[i];
> > +struct expr_constant *c = &as_diff->deleted->values[i];
> >  if (!as_info_from_expr_const(as_name, c, &as_info)) {
> >  continue;
> >  }
> > diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> > index 8e794450c..39c62bb66 100644
> > --- a/include/ovn/actions.h
> > +++ b/include/ovn/actions.h
> > @@ -238,7 +238,7 @@ struct ovnact_next {
> >  struct ovnact_load {
> >  struct ovnact ovnact;
> >  struct expr_field dst;
> > -union expr_constant imm;
> > +struct expr_constant imm;
> >  };
> >
> >  /* OVNACT_MOVE, OVNACT_EXCHANGE. */
> > diff --git a/include/ovn/expr.h b/include/ovn/expr.h
> > index c48f82398..e54edb5bf 100644
> > --- a/include/ovn/expr.h
> > +++ b/include/ovn/expr.h
> > @@ -368,7 +368,7 @@ bool expr_relop_from_token(enum lex_type type, enum
> expr_relop *relop);
> >  struct expr {
> >  struct ovs_list node;   /* In parent EXPR_T_AND or EXPR_T_OR if
> any. */
> >  enum expr_type type;/* Expression type. */
> > -char *as_name;  /* Address set name. Null if it is not
> an
> > +const char *as_name;/* Address set name. Null if it is not
> an
> > address set. */
> >
> >  union {
> > @@ -505,40 +505,42 @@ enum expr_constant_type {
> >  };
> >
> >  /* A string or integer constant (one must know which from context). */
> > -union expr_constant {
> > -/* Integer constant.
> > - *
> > - * The width of a constant isn't always clear, e.g. if you write
> "1",
> > - * there's no way to tell whether you mean for that to be a 1-bit
> constant
> > - * or a 128-bit constant or somewhere in between. */
> > -struct {
> > -union mf_subvalue value;
> > -union mf_subvalue mask; /* Only initialized if 'masked'. */
> > -bool masked;
> > -
>

Re: [ovs-dev] [PATCH] tc: Fix -Wgnu-variable-sized-type-not-at-end warning with Clang 18.

2024-04-30 Thread Ales Musil 
On Fri, Apr 26, 2024 at 7:44 PM Ilya Maximets  wrote:

> Clang 18.1.3-2.fc41 throws a warning:
>
>   lib/tc.c:3060:25: error: field 'sel' with variable sized type
> 'struct tc_pedit_sel' not at the end of a struct or class is a
> GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
>
>3060 | struct tc_pedit sel;
> | ^
>
> Refactor the structure into a proper union to avoid the build failure.
>
> Interestingly, clang 18.1.3-2.fc41 on Fedora throws a warning, but
> relatively the same version 18.1.3 (1) on Ubuntu 23.04 does not.
>
> Signed-off-by: Ilya Maximets 
> ---
>  lib/tc.c | 22 +++---
>  1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/lib/tc.c b/lib/tc.c
> index e9bcae4e4..e55ba3b1b 100644
> --- a/lib/tc.c
> +++ b/lib/tc.c
> @@ -3056,17 +3056,17 @@ nl_msg_put_flower_rewrite_pedits(struct ofpbuf
> *request,
>   struct tc_action *action,
>   uint32_t action_pc)
>  {
> -struct {
> +union {
>  struct tc_pedit sel;
> -struct tc_pedit_key keys[MAX_PEDIT_OFFSETS];
> -struct tc_pedit_key_ex keys_ex[MAX_PEDIT_OFFSETS];
> -} sel = {
> -.sel = {
> -.nkeys = 0
> -}
> -};
> +uint8_t buffer[sizeof(struct tc_pedit)
> +   + MAX_PEDIT_OFFSETS * sizeof(struct tc_pedit_key)];
> +} sel;
> +struct tc_pedit_key_ex keys_ex[MAX_PEDIT_OFFSETS];
>  int i, j, err;
>
> +memset(&sel, 0, sizeof sel);
> +memset(keys_ex, 0, sizeof keys_ex);
> +
>  for (i = 0; i < ARRAY_SIZE(flower_pedit_map); i++) {
>  struct flower_key_to_pedit *m = &flower_pedit_map[i];
>  struct tc_pedit_key *pedit_key = NULL;
> @@ -3100,8 +3100,8 @@ nl_msg_put_flower_rewrite_pedits(struct ofpbuf
> *request,
>  return EOPNOTSUPP;
>  }
>
> -pedit_key = &sel.keys[sel.sel.nkeys];
> -pedit_key_ex = &sel.keys_ex[sel.sel.nkeys];
> +pedit_key = &sel.sel.keys[sel.sel.nkeys];
> +pedit_key_ex = &keys_ex[sel.sel.nkeys];
>  pedit_key_ex->cmd = TCA_PEDIT_KEY_EX_CMD_SET;
>  pedit_key_ex->htype = m->htype;
>  pedit_key->off = cur_offset;
> @@ -3121,7 +3121,7 @@ nl_msg_put_flower_rewrite_pedits(struct ofpbuf
> *request,
>  }
>  }
>  }
> -nl_msg_put_act_pedit(request, &sel.sel, sel.keys_ex,
> +nl_msg_put_act_pedit(request, &sel.sel, keys_ex,
>   flower->csum_update_flags ? TC_ACT_PIPE :
> action_pc);
>
>  return 0;
> --
> 2.44.0
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Looks good to me, thanks.

Acked-by: Ales Musil 

-- 

Ales Musil

Senior Software Engineer - OVN Core

Red Hat EMEA 

amu...@redhat.com

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

Re: [ovs-dev] [PATCH] tests: Fix build failure with Clang 18 due to -Wformat-truncation.

2024-04-30 Thread Ales Musil 
On Fri, Apr 26, 2024 at 6:35 PM Ilya Maximets  wrote:

> Cirrus CI is broken on FreeBSD 13.3 due to clang version update.
> It now complains about snprintf truncation the same way GCC does:
>
>   tests/test-util.c:1129:16: error: 'snprintf' will always be truncated;
>   specified size is 5, but format string expands to at least 6
>   [-Werror,-Wformat-truncation]
>
>   1129 | ovs_assert(snprintf(s, 5, "abcde") == 5);
>|^
>
> Clang 17 on FreeBSD 14.0 works fine, but new Clang 18.1.4 on 13.3
> fails to build.
>
> Fix that by disabling Clang diagnostic the same way as we do for GCC.
>
> Unfortunately, the pragma's are compiler-specific, so cannot be
> combined, AFAIK.
>
> Signed-off-by: Ilya Maximets 
> ---
>  tests/test-util.c | 13 ++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/tests/test-util.c b/tests/test-util.c
> index 7d899fbbf..5d88d38f2 100644
> --- a/tests/test-util.c
> +++ b/tests/test-util.c
> @@ -1116,12 +1116,16 @@ test_snprintf(struct ovs_cmdl_context *ctx
> OVS_UNUSED)
>  {
>  char s[16];
>
> +/* GCC 7+ and Clang 18+ warn about the following calls that truncate
> + * a string using snprintf().  We're testing that truncation works
> + * properly, so temporarily disable the warning. */
>  #if __GNUC__ >= 7
> -/* GCC 7+ warns about the following calls that truncate a string using
> - * snprintf().  We're testing that truncation works properly, so
> - * temporarily disable the warning. */
>  #pragma GCC diagnostic push
>  #pragma GCC diagnostic ignored "-Wformat-truncation"
> +#endif
> +#if __clang_major__ >= 18
> +#pragma clang diagnostic push
> +#pragma clang diagnostic ignored "-Wformat-truncation"
>  #endif
>  ovs_assert(snprintf(s, 4, "abcde") == 5);
>  ovs_assert(!strcmp(s, "abc"));
> @@ -1130,6 +1134,9 @@ test_snprintf(struct ovs_cmdl_context *ctx
> OVS_UNUSED)
>  ovs_assert(!strcmp(s, "abcd"));
>  #if __GNUC__ >= 7
>  #pragma GCC diagnostic pop
> +#endif
> +#if __clang_major__ >= 18
> +#pragma clang diagnostic pop
>  #endif
>
>  ovs_assert(snprintf(s, 6, "abcde") == 5);
> --
> 2.44.0
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Looks good to me, thanks.

Acked-by: Ales Musil 
-- 

Ales Musil

Senior Software Engineer - OVN Core

Red Hat EMEA 

amu...@redhat.com

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

Re: [ovs-dev] [PATCH] tests: Fix build failure with Clang 18 due to -Wformat-truncation.

2024-04-30 Thread Ilya Maximets 
On 4/26/24 18:35, Ilya Maximets wrote:
> Cirrus CI is broken on FreeBSD 13.3 due to clang version update.
> It now complains about snprintf truncation the same way GCC does:
> 
>   tests/test-util.c:1129:16: error: 'snprintf' will always be truncated;
>   specified size is 5, but format string expands to at least 6
>   [-Werror,-Wformat-truncation]
> 
>   1129 | ovs_assert(snprintf(s, 5, "abcde") == 5);
>|^
> 
> Clang 17 on FreeBSD 14.0 works fine, but new Clang 18.1.4 on 13.3
> fails to build.
> 
> Fix that by disabling Clang diagnostic the same way as we do for GCC.
> 
> Unfortunately, the pragma's are compiler-specific, so cannot be
> combined, AFAIK.
> 
> Signed-off-by: Ilya Maximets 
> ---

Hi, Eelco and Simon.  May I ask you to take a look at this patch?

It's blocking Cirrus CI and I don't think anything else should be
merged until CI is fixed.

Best regards, Ilya Maximets.


>  tests/test-util.c | 13 ++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/test-util.c b/tests/test-util.c
> index 7d899fbbf..5d88d38f2 100644
> --- a/tests/test-util.c
> +++ b/tests/test-util.c
> @@ -1116,12 +1116,16 @@ test_snprintf(struct ovs_cmdl_context *ctx OVS_UNUSED)
>  {
>  char s[16];
>  
> +/* GCC 7+ and Clang 18+ warn about the following calls that truncate
> + * a string using snprintf().  We're testing that truncation works
> + * properly, so temporarily disable the warning. */
>  #if __GNUC__ >= 7
> -/* GCC 7+ warns about the following calls that truncate a string using
> - * snprintf().  We're testing that truncation works properly, so
> - * temporarily disable the warning. */
>  #pragma GCC diagnostic push
>  #pragma GCC diagnostic ignored "-Wformat-truncation"
> +#endif
> +#if __clang_major__ >= 18
> +#pragma clang diagnostic push
> +#pragma clang diagnostic ignored "-Wformat-truncation"
>  #endif
>  ovs_assert(snprintf(s, 4, "abcde") == 5);
>  ovs_assert(!strcmp(s, "abc"));
> @@ -1130,6 +1134,9 @@ test_snprintf(struct ovs_cmdl_context *ctx OVS_UNUSED)
>  ovs_assert(!strcmp(s, "abcd"));
>  #if __GNUC__ >= 7
>  #pragma GCC diagnostic pop
> +#endif
> +#if __clang_major__ >= 18
> +#pragma clang diagnostic pop
>  #endif
>  
>  ovs_assert(snprintf(s, 6, "abcde") == 5);

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

[ovs-dev] [PATCH v2] ofproto-dpif-rid: Fix duplicate entries.

2024-04-30 Thread wushaohua 
From: Shaohua Wu 

In scenarios with multiple PMDs, there may be
simultaneous requests for recirc_id from multiple
PMD threads.In recirc_alloc_id_ctx, we first check
if there is a duplicate entry in the metadata_map
for the same frozen_state field. If successful,
we directly retrieve the recirc_id. If unsuccessful,
we create a new recirc_node and insert it into
id_map and metadata_map. There is no locking mechanism
to prevent the possibility of two threads with the same
state simultaneously inserting, meaning their IDs are
different, but their frozen_states are the same.

trace log:
static struct recirc_id_node *
recirc_alloc_id__(const struct frozen_state *state, uint32_t hash)
{
ovs_assert(state->action_set_len <= state->ofpacts_len);

struct recirc_id_node *node = xzalloc(sizeof *node);

node->hash = hash;
ovs_refcount_init(&node->refcount);
node->is_hotupgrade = false;
frozen_state_clone(CONST_CAST(struct frozen_state *, &node->state), state);
struct recirc_id_node *hash_recirc_node = recirc_find_equal(&node->state, 
state);
if (hash_recirc_node) {
VLOG_INFO("wsh:hash equal:hash_recirc_node %p id:%u, hash_recirc_node 
hash:%u,node %p hash:%u\n",
   hash_recirc_node， hash_recirc_node->id, 
hash_recirc_node->hash, node, node->hash);
}
ovs_mutex_lock(&mutex);
...
}

Log recording:
2024-04-27T12:28:47.973Z|6|ofproto_dpif_rid(pmd-c08/id:13)|INFO|wsh:hash 
equal:
hash_recirc_node 0x7fb29c0276a0 id:27,hash_recirc_node hash:3224122528,node 
0x7fb2900194d0 hash:3224122528
2024-04-27T12:28:47.973Z|9|ofproto_dpif_rid(pmd-c02/id:15)|INFO|wsh:hash 
equal:
hash_recirc_node 0x7fb29c0276a0 id:27,hash_recirc_node hash:3224122528,node 
0x7fb288025270 hash:3224122528
2024-04-27T12:28:47.973Z|6|ofproto_dpif_rid(pmd-c03/id:14)|INFO|wsh:hash 
equal:
hash_recirc_node 0x7fb29c0276a0 id:27,hash_recirc_node hash:3224122528,node 
0x7fb29401d4e0 hash:3224122528
2024-04-27T12:28:47.973Z|4|ofproto_dpif_rid(pmd-c09/id:12)|INFO|node->id:28,hash:4019648042,table_id:75
2024-04-27T12:28:47.973Z|7|ofproto_dpif_rid(pmd-c08/id:13)|INFO|wsh:hash 
equal:
hash_recirc_node 0x7fb29c028d40 id:28,hash_recirc_node hash:4019648042,node 
0x7fb29001ac30 hash:4019648042
2024-04-27T12:28:48.065Z|5|ofproto_dpif_rid(pmd-c09/id:12)|INFO|node->id:29,hash:3800776147,table_id:30
2024-04-27T12:28:48.101Z|7|ofproto_dpif_rid(pmd-c03/id:14)|INFO|node->id:30,hash:1580334976,table_id:75
Signed-off-by: Shaohua Wu 

---
v1->v2:modify log recording , add trace code.
---
 ofproto/ofproto-dpif-rid.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/ofproto/ofproto-dpif-rid.c b/ofproto/ofproto-dpif-rid.c
index f01468025..651db6c53 100644
--- a/ofproto/ofproto-dpif-rid.c
+++ b/ofproto/ofproto-dpif-rid.c
@@ -234,6 +234,7 @@ recirc_alloc_id__(const struct frozen_state *state, 
uint32_t hash)
 {
 ovs_assert(state->action_set_len <= state->ofpacts_len);
 
+struct recirc_id_node *hash_recirc_node = NULL;
 struct recirc_id_node *node = xzalloc(sizeof *node);
 
 node->hash = hash;
@@ -241,6 +242,12 @@ recirc_alloc_id__(const struct frozen_state *state, 
uint32_t hash)
 frozen_state_clone(CONST_CAST(struct frozen_state *, &node->state), state);
 
 ovs_mutex_lock(&mutex);
+hash_recirc_node = recirc_ref_equal(&node->state, node->hash);
+if (hash_recirc_node) {
+ovs_mutex_unlock(&mutex);
+recirc_id_node_free(node);
+return hash_recirc_node;
+}
 for (;;) {
 /* Claim the next ID.  The ID space should be sparse enough for the
allocation to succeed at the first try.  We do skip the first
-- 
2.30.2

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