Re: [ovs-dev] [PATCH v7] bond: Always revalidate unbalanced bonds when active member changes.

[Eelco Chaudron]

On 3 Oct 2024, at 23:00, Mike Pattrick wrote:

> Currently a bond will not always revalidate when an active member
> changes. This can result in counter-intuitive behaviors like the fact
> that using ovs-appctl bond/set-active-member will cause the bond to
> revalidate but changing other_config:bond-primary will not trigger a
> revalidate in the bond.
>
> When revalidation is not set but the active member changes in an
> unbalanced bond, OVS may send traffic out of previously active member
> instead of the new active member.
>
> This change will always mark unbalanced bonds for revalidation if the
> active member changes.
>
> Reported-at: https://issues.redhat.com/browse/FDP-845
> Signed-off-by: Mike Pattrick 

Thanks for the updated revision. The patch looks good to me. I'm not sure if we 
still need the DP test now that we have the unit test. I'll leave it to Ilya to 
decide, as he originally requested it.

//Eelco

Acked-by: Eelco Chaudron 

> ---
> v2: Added a test
> v3: Made the test more reliable
> v4: Made test much more reliable
> v5: Improved test performance
> v6: Improved system test by removing waits on ping.
> v7: Added a unit test.
> ---
>  ofproto/bond.c  |  8 +--
>  tests/ofproto-dpif.at   | 40 
>  tests/system-traffic.at | 51 +
>  3 files changed, 97 insertions(+), 2 deletions(-)
>
> diff --git a/ofproto/bond.c b/ofproto/bond.c
> index 0858de374..b9e2282f0 100644
> --- a/ofproto/bond.c
> +++ b/ofproto/bond.c
> @@ -193,6 +193,7 @@ static void bond_update_post_recirc_rules__(struct bond 
> *, bool force)
>  static bool bond_is_falling_back_to_ab(const struct bond *);
>  static void bond_add_lb_output_buckets(const struct bond *);
>  static void bond_del_lb_output_buckets(const struct bond *);
> +static bool bond_is_balanced(const struct bond *bond) OVS_REQ_RDLOCK(rwlock);
>
>
>  /* Attempts to parse 's' as the name of a bond balancing mode.  If 
> successful,
> @@ -549,11 +550,15 @@ bond_find_member_by_mac(const struct bond *bond, const 
> struct eth_addr mac)
>
>  static void
>  bond_active_member_changed(struct bond *bond)
> +OVS_REQ_WRLOCK(rwlock)
>  {
>  if (bond->active_member) {
>  struct eth_addr mac;
>  netdev_get_etheraddr(bond->active_member->netdev, &mac);
>  bond->active_member_mac = mac;
> +if (!bond_is_balanced(bond)) {
> +bond->bond_revalidate = true;
> +}
>  } else {
>  bond->active_member_mac = eth_addr_zero;
>  }
> @@ -1121,7 +1126,7 @@ bond_get_recirc_id_and_hash_basis(struct bond *bond, 
> uint32_t *recirc_id,
>  /* Rebalancing. */
>
>  static bool
> -bond_is_balanced(const struct bond *bond) OVS_REQ_RDLOCK(rwlock)
> +bond_is_balanced(const struct bond *bond)
>  {
>  return bond->rebalance_interval
>  && (bond->balance == BM_SLB || bond->balance == BM_TCP)
> @@ -1725,7 +1730,6 @@ bond_unixctl_set_active_member(struct unixctl_conn 
> *conn,
>  }
>
>  if (bond->active_member != member) {
> -bond->bond_revalidate = true;
>  bond->active_member = member;
>  VLOG_INFO("bond %s: active member is now %s",
>bond->name, member->name);
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 12cb7f7a6..7a916de54 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -351,6 +351,46 @@ 
> recirc_id(0),in_port(4),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:0b,dst=ff:
>  OVS_VSWITCHD_STOP
>  AT_CLEANUP
>
> +AT_SETUP([ofproto-dpif - active-backup bonding set primary])
> +
> +OVS_VSWITCHD_START(
> +  [add-bond br0 bond0 p1 p2 bond_mode=active-backup \
> +other_config:bond-primary=p1 -- \
> +   set bridge br0 other-config:hwaddr=aa:66:aa:66:aa:00 -- \
> +   set interface p1 type=dummy options:pstream=punix:$OVS_RUNDIR/p1.sock 
> ofport_request=1 -- \
> +   set interface p2 type=dummy options:pstream=punix:$OVS_RUNDIR/p2.sock 
> ofport_request=2 -- \
> +   add-port br0 p7 -- set interface p7 ofport_request=7 type=dummy -- \
> +   add-br br1 -- \
> +   set bridge br1 other-config:hwaddr=aa:66:aa:66:00:00 -- \
> +   set bridge br1 datapath-type=dummy -- \
> +   add-bond br1 bond1 p3 p4 bond_mode=active-backup \
> +other_config:bond-primary=p3 -- \
> +   set interface p3 type=dummy options:stream=unix:$OVS_RUNDIR/p1.sock 
> ofport_request=3 -- \
> +   set interface p4 type=dummy options:stream=unix:$OVS_RUNDIR/p2.sock 
> ofport_request=4 -- \
> +   add-port br1 p8 -- set interface p8 ofport_request=8 t

Re: [ovs-dev] [PATCH v2] ofproto-dpif-upcall: Fix redundant mirror on metadata modification.

[Eelco Chaudron]

On 2 Oct 2024, at 18:02, Mike Pattrick wrote:

> Previously a commit attempted to reset the mirror context when packets
> were modified. However, this commit erroneously also reset the mirror
> context when only a packet's metadata was modified. An intermediate
> commit corrected this for tunnel metadata, but now that correction is
> extended to other forms of metadata as well.
>
> Fixes: feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are modified.")
> Reported-at: https://issues.redhat.com/browse/FDP-699
> Acked-by: Eelco Chaudron 
> Acked-by: Simon Horman 
> Signed-off-by: Mike Pattrick 

Two comments below on the test case, the rest looks good to me.

//Eelco


> ---
> v2:
>  - Added extra whitespace
>  - Moved N_IDS to never reached section
>  - Added unit test
> ---
>  include/openvswitch/meta-flow.h |   1 +
>  lib/meta-flow.c | 109 
>  ofproto/ofproto-dpif-xlate.c|   2 +-
>  tests/ofproto-dpif.at   |  24 +++
>  4 files changed, 135 insertions(+), 1 deletion(-)
>
> diff --git a/include/openvswitch/meta-flow.h b/include/openvswitch/meta-flow.h
> index aff917bcf..65d8b01fe 100644
> --- a/include/openvswitch/meta-flow.h
> +++ b/include/openvswitch/meta-flow.h
> @@ -2308,6 +2308,7 @@ void mf_set_flow_value_masked(const struct mf_field *,
>const union mf_value *mask,
>struct flow *);
>  bool mf_is_tun_metadata(const struct mf_field *);
> +bool mf_is_metadata(const struct mf_field *);
>  bool mf_is_frozen_metadata(const struct mf_field *);
>  bool mf_is_pipeline_field(const struct mf_field *);
>  bool mf_is_set(const struct mf_field *, const struct flow *);
> diff --git a/lib/meta-flow.c b/lib/meta-flow.c
> index 499be04b6..e11fa67e4 100644
> --- a/lib/meta-flow.c
> +++ b/lib/meta-flow.c
> @@ -1790,6 +1790,115 @@ mf_is_tun_metadata(const struct mf_field *mf)
> mf->id < MFF_TUN_METADATA0 + TUN_METADATA_NUM_OPTS;
>  }
>
> +bool
> +mf_is_metadata(const struct mf_field *mf)
> +{
> +switch (mf->id) {
> +case MFF_ARP_OP:
> +case MFF_ARP_SHA:
> +case MFF_ARP_SPA:
> +case MFF_ARP_THA:
> +case MFF_ARP_TPA:
> +case MFF_DL_VLAN:
> +case MFF_DL_VLAN_PCP:
> +case MFF_ETH_DST:
> +case MFF_ETH_SRC:
> +case MFF_ETH_TYPE:
> +case MFF_ICMPV4_CODE:
> +case MFF_ICMPV4_TYPE:
> +case MFF_ICMPV6_CODE:
> +case MFF_ICMPV6_TYPE:
> +case MFF_IPV4_DST:
> +case MFF_IPV4_SRC:
> +case MFF_IPV6_DST:
> +case MFF_IPV6_LABEL:
> +case MFF_IPV6_SRC:
> +case MFF_IP_DSCP:
> +case MFF_IP_DSCP_SHIFTED:
> +case MFF_IP_ECN:
> +case MFF_IP_FRAG:
> +case MFF_IP_PROTO:
> +case MFF_IP_TTL:
> +case MFF_MPLS_BOS:
> +case MFF_MPLS_LABEL:
> +case MFF_MPLS_TC:
> +case MFF_MPLS_TTL:
> +case MFF_ND_OPTIONS_TYPE:
> +case MFF_ND_RESERVED:
> +case MFF_ND_SLL:
> +case MFF_ND_TARGET:
> +case MFF_ND_TLL:
> +case MFF_NSH_C1:
> +case MFF_NSH_C2:
> +case MFF_NSH_C3:
> +case MFF_NSH_C4:
> +case MFF_NSH_FLAGS:
> +case MFF_NSH_MDTYPE:
> +case MFF_NSH_NP:
> +case MFF_NSH_SI:
> +case MFF_NSH_SPI:
> +case MFF_NSH_TTL:
> +case MFF_PACKET_TYPE:
> +case MFF_SCTP_DST:
> +case MFF_SCTP_SRC:
> +case MFF_TCP_DST:
> +case MFF_TCP_FLAGS:
> +case MFF_TCP_SRC:
> +case MFF_TUN_DST:
> +case MFF_TUN_ERSPAN_HWID:
> +case MFF_TUN_ERSPAN_IDX:
> +case MFF_TUN_ERSPAN_VER:
> +case MFF_TUN_FLAGS:
> +case MFF_TUN_GBP_FLAGS:
> +case MFF_TUN_GBP_ID:
> +case MFF_TUN_GTPU_FLAGS:
> +case MFF_TUN_GTPU_MSGTYPE:
> +case MFF_TUN_ID:
> +case MFF_TUN_IPV6_DST:
> +case MFF_TUN_IPV6_SRC:
> +case MFF_TUN_SRC:
> +case MFF_TUN_TOS:
> +case MFF_TUN_TTL:
> +case MFF_UDP_DST:
> +case MFF_UDP_SRC:
> +case MFF_VLAN_PCP:
> +case MFF_VLAN_TCI:
> +case MFF_VLAN_VID:
> +return false;
> +
> +CASE_MFF_REGS:
> +CASE_MFF_TUN_METADATA:
> +CASE_MFF_XREGS:
> +CASE_MFF_XXREGS:
> +case MFF_ACTSET_OUTPUT:
> +case MFF_CONJ_ID:
> +case MFF_CT_IPV6_DST:
> +case MFF_CT_IPV6_SRC:
> +case MFF_CT_LABEL:
> +case MFF_CT_MARK:
> +case MFF_CT_NW_DST:
> +case MFF_CT_NW_PROTO:
> +case MFF_CT_NW_SRC:
> +case MFF_CT_STATE:
> +case MFF_CT_TP_DST:
> +case MFF_CT_TP_SRC:
> +case MFF_CT_ZONE:
> +case MFF_DP_HASH:
> +case MFF_IN_PORT:
> +case MFF_IN_PORT_OXM:
> +case MFF_METADATA:
> +case

Re: [ovs-dev] [PATCH v3 1/2] system-traffic: Do not rely on conncount for already tracked packets.

[Eelco Chaudron]

On 2 Oct 2024, at 18:01, Paolo Valerio wrote:

> As Long reported, kernels built without CONFIG_NETFILTER_CONNCOUNT
> result in the unexpected failure of the following tests:
>
> conntrack - multiple zones, local
> conntrack - multi-stage pipeline, local
> conntrack - can match and clear ct_state from outside OVS
>
> this happens because the nf_conncount turns on connection tracking and
> the above tests rely on this side effect. However, this behavior may
> be corrected in the kernel, which could, in turn, cause the tests to
> fail.
>
> The patch removes the assumption by adding iptables rules to attach
> an nf_conn template to the skb resulting tracked once hit the OvS
> pipeline.
>
> While at it, introduce $HAVE_IPTABLES and skip tests if iptables
> binary is not present.
>
> Reported-by: Xin Long 
> Reported-at: https://issues.redhat.com/browse/FDP-708
> Signed-off-by: Paolo Valerio 

Thanks for the new revision. The changes look good to me.

Cheers,

Eelco

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

Re: [ovs-dev] [PATCH v3 1/1] debian: Allow passing DEB_BUILD_OPTIONS.

[Eelco Chaudron]

On 30 Sep 2024, at 16:49, Roi Dayan wrote:

> Allow passing different DEB_BUILD_OPTIONS to make debian-deb.
>
> Signed-off-by: Roi Dayan 
> ---

Applied with the $() instead of curly ones ${}.

Cheers,

Eelco

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

Re: [ovs-dev] [PATCH v2] python: Index references.

[Eelco Chaudron]

On 4 Oct 2024, at 12:23, Eelco Chaudron wrote:

> On 1 Oct 2024, at 11:20, Luca Czesla via dev wrote:
>
>> For an index on a reference to work, we should not resolve the reference to
>> to the corresponding row object but instead use the uuid for indexing.
>>
>> an index on Datapath in the Port_Binding Table reduces the lookup in the
>> neutron metadata agent for each request from approx. 400ms per request to
>> 980μs with approx. 40k port bindings.
>>
>> Signed-off-by: Luca Czesla 
>
>
> Hi Luca,
>
> The v2 looks good to me.
>
> //Eelco
>
> Acked-by: Eelco Chaudron 


In addition, Terry would you be able to have a look before we apply the patch?

Also, we might need to adjust the title a bit, probably something like:

  python: Use index references in the MultiColumnIndex class.

Cheers,

Eelco

>> ---
>>  python/ovs/db/custom_index.py | 14 +++---
>>  python/ovs/db/idl.py  |  9 ++---
>>  2 files changed, 17 insertions(+), 6 deletions(-)
>>
>> diff --git a/python/ovs/db/custom_index.py b/python/ovs/db/custom_index.py
>> index 3fa03d3c9..803baaccd 100644
>> --- a/python/ovs/db/custom_index.py
>> +++ b/python/ovs/db/custom_index.py
>> @@ -11,7 +11,7 @@ try:
>>  except ImportError:
>>  from ovs.compat import sortedcontainers
>>
>> -from ovs.db import data
>> +from ovs.db import data, idl
>>
>>  OVSDB_INDEX_ASC = "ASC"
>>  OVSDB_INDEX_DESC = "DESC"
>> @@ -43,7 +43,7 @@ class MultiColumnIndex(object):
>>
>>  def _cmp(self, a, b):
>>  for col, direction, key in self.columns:
>> -aval, bval = key(a), key(b)
>> +aval, bval = idl._row_to_uuid(key(a)), idl._row_to_uuid(key(b))
>>  if aval == bval:
>>  continue
>>  result = (aval > bval) - (aval < bval)
>> @@ -53,7 +53,8 @@ class MultiColumnIndex(object):
>>  def index_entry_from_row(self, row):
>>  return row._table.rows.IndexEntry(
>>  uuid=row.uuid,
>> -**{c.column: getattr(row, c.column) for c in self.columns})
>> +**{c.column: row.get_column(c.column, self._uuid_to_uuid)
>> +for c in self.columns})
>>
>>  def add(self, row):
>>  if not all(hasattr(row, col.column) for col in self.columns):
>> @@ -76,6 +77,13 @@ class MultiColumnIndex(object):
>>  def __iter__(self):
>>  return iter(r._table.rows[r.uuid] for r in self.values)
>>
>> +# For indexing we do not want to resolve the references as we cannot
>> +# ensure that the referenced table is already filled, the uuid is
>> +# sufficient for the index.
>> +@staticmethod
>> +def _uuid_to_uuid(atom, base):
>> +return atom
>> +
>>
>>  class IndexedRows(DictBase, object):
>>  def __init__(self, table, *args, **kwargs):
>> diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
>> index c8cc54346..3feb068b8 100644
>> --- a/python/ovs/db/idl.py
>> +++ b/python/ovs/db/idl.py
>> @@ -1351,7 +1351,7 @@ class Row(object):
>>  else:
>>  return atom
>>
>> -def __getattr__(self, column_name):
>> +def get_column(self, column_name, uuid_to_row):
>>  assert self._changes is not None
>>  assert self._mutations is not None
>>
>> @@ -1392,7 +1392,7 @@ class Row(object):
>>  datum = data.Datum.from_python(column.type, dlist,
>> _row_to_uuid)
>>  elif column.type.is_map():
>> -dmap = datum.to_python(self._uuid_to_row)
>> +dmap = datum.to_python(uuid_to_row)
>>  if inserts is not None:
>>  dmap.update(inserts)
>>  if removes is not None:
>> @@ -1409,7 +1409,10 @@ class Row(object):
>>  else:
>>  datum = inserts
>>
>> -return datum.to_python(self._uuid_to_row)
>> +return datum.to_python(uuid_to_row)
>> +
>> +def __getattr__(self, column_name):
>> +return self.get_column(column_name, self._uuid_to_row)
>>
>>  def __setattr__(self, column_name, value):
>>  assert self._changes is not None
>>
>> base-commit: 1b99649025ae767974d0a7cea88d1e9f46f4378d
>> -- 
>> 2.39.5
>>
>> ___
>> dev mailing list
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

Re: [ovs-dev] [PATCH v2] python: Index references.

[Eelco Chaudron]

On 1 Oct 2024, at 11:20, Luca Czesla via dev wrote:

> For an index on a reference to work, we should not resolve the reference to
> to the corresponding row object but instead use the uuid for indexing.
>
> an index on Datapath in the Port_Binding Table reduces the lookup in the
> neutron metadata agent for each request from approx. 400ms per request to
> 980μs with approx. 40k port bindings.
>
> Signed-off-by: Luca Czesla 


Hi Luca,

The v2 looks good to me.

//Eelco

Acked-by: Eelco Chaudron 

> ---
>  python/ovs/db/custom_index.py | 14 +++---
>  python/ovs/db/idl.py  |  9 ++---
>  2 files changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/python/ovs/db/custom_index.py b/python/ovs/db/custom_index.py
> index 3fa03d3c9..803baaccd 100644
> --- a/python/ovs/db/custom_index.py
> +++ b/python/ovs/db/custom_index.py
> @@ -11,7 +11,7 @@ try:
>  except ImportError:
>  from ovs.compat import sortedcontainers
>
> -from ovs.db import data
> +from ovs.db import data, idl
>
>  OVSDB_INDEX_ASC = "ASC"
>  OVSDB_INDEX_DESC = "DESC"
> @@ -43,7 +43,7 @@ class MultiColumnIndex(object):
>
>  def _cmp(self, a, b):
>  for col, direction, key in self.columns:
> -aval, bval = key(a), key(b)
> +aval, bval = idl._row_to_uuid(key(a)), idl._row_to_uuid(key(b))
>  if aval == bval:
>  continue
>  result = (aval > bval) - (aval < bval)
> @@ -53,7 +53,8 @@ class MultiColumnIndex(object):
>  def index_entry_from_row(self, row):
>  return row._table.rows.IndexEntry(
>  uuid=row.uuid,
> -**{c.column: getattr(row, c.column) for c in self.columns})
> +**{c.column: row.get_column(c.column, self._uuid_to_uuid)
> +for c in self.columns})
>
>  def add(self, row):
>  if not all(hasattr(row, col.column) for col in self.columns):
> @@ -76,6 +77,13 @@ class MultiColumnIndex(object):
>  def __iter__(self):
>  return iter(r._table.rows[r.uuid] for r in self.values)
>
> +# For indexing we do not want to resolve the references as we cannot
> +# ensure that the referenced table is already filled, the uuid is
> +# sufficient for the index.
> +@staticmethod
> +def _uuid_to_uuid(atom, base):
> +return atom
> +
>
>  class IndexedRows(DictBase, object):
>  def __init__(self, table, *args, **kwargs):
> diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
> index c8cc54346..3feb068b8 100644
> --- a/python/ovs/db/idl.py
> +++ b/python/ovs/db/idl.py
> @@ -1351,7 +1351,7 @@ class Row(object):
>  else:
>  return atom
>
> -def __getattr__(self, column_name):
> +def get_column(self, column_name, uuid_to_row):
>  assert self._changes is not None
>  assert self._mutations is not None
>
> @@ -1392,7 +1392,7 @@ class Row(object):
>  datum = data.Datum.from_python(column.type, dlist,
> _row_to_uuid)
>  elif column.type.is_map():
> -dmap = datum.to_python(self._uuid_to_row)
> +dmap = datum.to_python(uuid_to_row)
>  if inserts is not None:
>  dmap.update(inserts)
>  if removes is not None:
> @@ -1409,7 +1409,10 @@ class Row(object):
>  else:
>  datum = inserts
>
> -return datum.to_python(self._uuid_to_row)
> +return datum.to_python(uuid_to_row)
> +
> +def __getattr__(self, column_name):
> +return self.get_column(column_name, self._uuid_to_row)
>
>  def __setattr__(self, column_name, value):
>  assert self._changes is not None
>
> base-commit: 1b99649025ae767974d0a7cea88d1e9f46f4378d
> -- 
> 2.39.5
>
> ___
> 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 v3 1/1] debian: Allow passing DEB_BUILD_OPTIONS.

[Eelco Chaudron]

On 2 Oct 2024, at 13:35, Ilya Maximets wrote:

> On 9/30/24 16:49, Roi Dayan via dev wrote:
>> Allow passing different DEB_BUILD_OPTIONS to make debian-deb.
>>
>> Signed-off-by: Roi Dayan 
>> ---
>>
>> Notes:
>> v3
>> - Remove unneeded export call.
>> - Move assignment to an existing DPDK_NETDEV check.
>>
>> v2
>> - Fix export of DEB_BUILD_OPTIONS in the Makefile
>>
>>  debian/automake.mk | 9 +++--
>>  1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/debian/automake.mk b/debian/automake.mk
>> index 7b2afafae1a2..7607a2cd5b3a 100644
>> --- a/debian/automake.mk
>> +++ b/debian/automake.mk
>> @@ -98,10 +98,12 @@ if DPDK_NETDEV
>>  update_deb_control = \
>>  $(AM_V_GEN) sed -e 's/^\# DPDK_NETDEV //' \
>>  < $(srcdir)/debian/control.in > debian/control
>> +DEB_BUILD_OPTIONS ?= nocheck parallel=`nproc`
>>  else
>>  update_deb_control = \
>>  $(AM_V_GEN) grep -v '^\# DPDK_NETDEV' \
>>  < $(srcdir)/debian/control.in > debian/control
>> +DEB_BUILD_OPTIONS ?= nocheck parallel=`nproc` nodpdk
>>  endif
>>
>>  debian/control: $(srcdir)/debian/control.in Makefile
>> @@ -123,10 +125,5 @@ debian-deb: debian
>>  $(update_deb_copyright)
>>  $(update_deb_control)
>>  $(AM_V_GEN) fakeroot debian/rules clean
>> -if DPDK_NETDEV
>> -$(AM_V_GEN) DEB_BUILD_OPTIONS="nocheck parallel=`nproc`" \
>> -fakeroot debian/rules binary
>> -else
>> -$(AM_V_GEN) DEB_BUILD_OPTIONS="nocheck parallel=`nproc` nodpdk" \
>> +$(AM_V_GEN) DEB_BUILD_OPTIONS="${DEB_BUILD_OPTIONS}" \
>
> I think some confusion is coming from env variables vs make variables.
> While make imports all the env variables, the definitions in the file
> may still be a little confusing, since they are not shell definitions
> ( ?= is not a shell operator).
>
> Can we maybe use plain braces $() instead of curly ones here ${} ?
> That may probably make the code a little clearer.

Did a quick test and the $() approach works also. I can apply this during 
commit time, Roi please confirm if you are ok with this change.

Cheers,

Eelco

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

[ovs-dev] [PATCH v5] checkpatch: Add new check-authors-file option to checkpatch.py.

[Eelco Chaudron]

This patch adds a new option, --check-authors-file, to the checkpatch
tool to help OVS maintainers check for missing authors in the
AUTHORS.rst file.

Co-authored-by: Aaron Conole 
Signed-off-by: Aaron Conole 
Signed-off-by: Eelco Chaudron 
---
v3: - Also include co-authors in the check.
- Only report the end, when all patches are checked.
- Fixed spelling mistake.
- Determine git root directory for AUTHORS.rst location.
v4: - Added unit test.
v5: - Incorporated Aaron's addition to check for updated
  AUTHORS.rst in the patches.
---
 tests/checkpatch.at | 21 ++
 utilities/checkpatch.py | 87 +++--
 2 files changed, 105 insertions(+), 3 deletions(-)

diff --git a/tests/checkpatch.at b/tests/checkpatch.at
index 34971c514..fa179c707 100755
--- a/tests/checkpatch.at
+++ b/tests/checkpatch.at
@@ -634,3 +634,24 @@ try_checkpatch \
 "--skip-committer-signoff"
 
 AT_CLEANUP
+
+AT_SETUP([checkpatch - AUTHORS.rst existence])
+
+try_checkpatch \
+   "Author: A 
+Commit: A 
+Subject: netdev: Subject.
+
+Signed-off-by: A " \
+""
+
+try_checkpatch \
+   "Author: A 
+Commit: A 
+Subject: netdev: Subject.
+
+Signed-off-by: A " \
+"WARNING: Author 'A ' is not in the AUTHORS.rst file!" \
+"-a"
+
+AT_CLEANUP
diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index 742a0bc47..53b13bcf2 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -28,12 +28,14 @@ __errors = 0
 __warnings = 0
 empty_return_check_state = 0
 print_file_name = None
+check_authors_file = False
 checking_file = False
 total_line = 0
 colors = False
 spellcheck = False
 quiet = False
 spell_check_dict = None
+missing_authors = []
 
 
 def open_spell_check_dict():
@@ -666,6 +668,10 @@ checks = [
  'check': lambda x: has_efgrep(x),
  'print':
  lambda: print_error("grep -E/-F should be used instead of egrep/fgrep")},
+
+{'regex': 'AUTHORS.rst$', 'match_name': None,
+ 'check': lambda x: update_missing_authors(x),
+ 'print': None},
 ]
 
 
@@ -860,9 +866,56 @@ def run_subject_checks(subject, spellcheck=False):
 return warnings
 
 
+def get_top_directory():
+with os.popen('git rev-parse --show-toplevel') as pipe:
+path = pipe.read()
+
+if path:
+return path.strip()
+
+return "."
+
+
+def update_missing_authors(diffed_line):
+global missing_authors
+for author in missing_authors:
+m = re.search(r'<(.*?)>', author)
+if not m:
+continue
+pattern = r'\b' + re.escape(m.group(1)) + r'\b'
+if re.search(pattern, diffed_line) is None:
+continue
+else:
+missing_authors.remove(author)
+
+return False
+
+
+def do_authors_exist(authors):
+authors = list(set(authors))
+missing_authors = []
+
+try:
+with open(get_top_directory() + "/AUTHORS.rst", "r") as file:
+file_content = file.read()
+for author in authors:
+m = re.search(r'<(.*?)>', author)
+if not m:
+continue
+pattern = r'\b' + re.escape(m.group(1)) + r'\b'
+if re.search(pattern, file_content) is None:
+missing_authors.append(author)
+
+except FileNotFoundError:
+print_error("Could not open AUTHORS.rst in '%s/'!" %
+get_top_directory())
+
+return missing_authors
+
+
 def ovs_checkpatch_parse(text, filename, author=None, committer=None):
 global print_file_name, total_line, checking_file, \
-empty_return_check_state
+empty_return_check_state, missing_authors
 
 PARSE_STATE_HEADING = 0
 PARSE_STATE_DIFF_HEADER = 1
@@ -977,6 +1030,11 @@ def ovs_checkpatch_parse(text, filename, author=None, 
committer=None):
   "who are not authors or co-authors or "
   "committers: %s"
   % ", ".join(extra_sigs))
+
+if check_authors_file:
+missing_authors = do_authors_exist(missing_authors +
+   co_authors + [author])
+
 elif is_committer.match(line):
 committer = is_committer.match(line).group(2)
 elif is_author.match(line):
@@ -1067,6 +1125,8 @@ Input options:
 
 Check options:
 -h|--help  This help message
+-a|--check-authors-fileCheck AUTHORS file for existence of the authors.
+   Should be used by commiters only!
 -b|--skip-block-whitespace Skips th

Re: [ovs-dev] [PATCH v4] checkpatch: Add new check-authors-file option to checkpatch.py.

[Eelco Chaudron]

On 1 Oct 2024, at 15:38, Aaron Conole wrote:

> Eelco Chaudron  writes:
>
>> This patch adds a new option, --check-authors-file, to the checkpatch
>> tool to help OVS maintainers check for missing authors in the
>> AUTHORS.rst file.
>>
>> Signed-off-by: Eelco Chaudron 
>> ---
>> v3: - Also include co-authors in the check.
>> - Only report the end, when all patches are checked.
>> - Fixed spelling mistake.
>> - Determine git root directory for AUTHORS.rst location.
>>
>> v4: - Added a test case.
>> ---
>
> Thanks for the quick follow up Eelco.
>
>>  tests/checkpatch.at | 21 +
>>  utilities/checkpatch.py | 68 +++--
>>  2 files changed, 86 insertions(+), 3 deletions(-)
>>
>> diff --git a/tests/checkpatch.at b/tests/checkpatch.at
>> index 34971c514..fa179c707 100755
>> --- a/tests/checkpatch.at
>> +++ b/tests/checkpatch.at
>> @@ -634,3 +634,24 @@ try_checkpatch \
>>  "--skip-committer-signoff"
>>
>>  AT_CLEANUP
>> +
>> +AT_SETUP([checkpatch - AUTHORS.rst existence])
>> +
>> +try_checkpatch \
>> +   "Author: A 
>> +Commit: A 
>> +Subject: netdev: Subject.
>> +
>> +Signed-off-by: A " \
>> +""
>> +
>> +try_checkpatch \
>> +   "Author: A 
>> +Commit: A 
>> +Subject: netdev: Subject.
>> +
>> +Signed-off-by: A " \
>> +"WARNING: Author 'A ' is not in the AUTHORS.rst file!" \
>> +"-a"
>> +
>> +AT_CLEANUP
>> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
>> index 742a0bc47..73c20f804 100755
>> --- a/utilities/checkpatch.py
>> +++ b/utilities/checkpatch.py
>> @@ -28,12 +28,14 @@ __errors = 0
>>  __warnings = 0
>>  empty_return_check_state = 0
>>  print_file_name = None
>> +check_authors_file = False
>>  checking_file = False
>>  total_line = 0
>>  colors = False
>>  spellcheck = False
>>  quiet = False
>>  spell_check_dict = None
>> +missing_authors = []
>>
>>
>>  def open_spell_check_dict():
>> @@ -860,9 +862,41 @@ def run_subject_checks(subject, spellcheck=False):
>>  return warnings
>>
>>
>> +def get_top_directory():
>> +with os.popen('git rev-parse --show-toplevel') as pipe:
>> +path = pipe.read()
>> +
>> +if path:
>> +return path.strip()
>> +
>> +return "."
>> +
>> +
>> +def do_authors_exist(authors):
>> +authors = list(set(authors))
>> +missing_authors = []
>> +
>> +try:
>> +with open(get_top_directory() + "/AUTHORS.rst", "r") as file:
>> +file_content = file.read()
>> +for author in authors:
>> +m = re.search(r'<(.*?)>', author)
>> +if not m:
>> +continue
>> +pattern = r'\b' + re.escape(m.group(1)) + r'\b'
>> +if re.search(pattern, file_content) is None:
>> +missing_authors.append(author)
>> +
>> +except FileNotFoundError:
>> +print_error("Could not open AUTHORS.rst in '%s/'!" %
>> +get_top_directory())
>> +
>> +return missing_authors
>> +
>> +
>>  def ovs_checkpatch_parse(text, filename, author=None, committer=None):
>>  global print_file_name, total_line, checking_file, \
>> -empty_return_check_state
>> +empty_return_check_state, missing_authors
>>
>>  PARSE_STATE_HEADING = 0
>>  PARSE_STATE_DIFF_HEADER = 1
>> @@ -977,6 +1011,11 @@ def ovs_checkpatch_parse(text, filename, author=None, 
>> committer=None):
>>"who are not authors or co-authors or 
>> "
>>"committers: %s"
>>% ", ".join(extra_sigs))
>> +
>> +if check_authors_file:
>> +missing_authors = do_authors_exist(missing_authors +
>> +   co_authors + 
>> [author])
>> +
>>  elif is_committer.match(line):
>>  committer = is_committer.match(line).group(2)
>>  elif is_author.match(line):
>> @@ -1067,6 +1106,8 @@ Input

Re: [ovs-dev] [PATCH v1] ofproto-dpif-upcall: Fix redundant mirror on metadata modification.

[Eelco Chaudron]

On 1 Oct 2024, at 20:47, Mike Pattrick wrote:

> Previously a commit attempted to reset the mirror context when packets
> were modified. However, this commit erroneously also reset the mirror
> context when only a packet's metadata was modified. An intermediate
> commit corrected this for tunnel metadata, but now that correction is
> extended to other forms of metadata as well.
>
> Fixes: feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are modified.")
> Reported-at: https://issues.redhat.com/browse/FDP-699
> Signed-off-by: Mike Pattrick 

Small nits, which I think we could apply during the commit. With nits fixed

Acked-by: Eelco Chaudron 

//Eelco

> ---
>  include/openvswitch/meta-flow.h |   1 +
>  lib/meta-flow.c | 107 
>  ofproto/ofproto-dpif-xlate.c|   2 +-
>  3 files changed, 109 insertions(+), 1 deletion(-)
>
> diff --git a/include/openvswitch/meta-flow.h b/include/openvswitch/meta-flow.h
> index aff917bcf..65d8b01fe 100644
> --- a/include/openvswitch/meta-flow.h
> +++ b/include/openvswitch/meta-flow.h
> @@ -2308,6 +2308,7 @@ void mf_set_flow_value_masked(const struct mf_field *,
>const union mf_value *mask,
>struct flow *);
>  bool mf_is_tun_metadata(const struct mf_field *);
> +bool mf_is_metadata(const struct mf_field *);
>  bool mf_is_frozen_metadata(const struct mf_field *);
>  bool mf_is_pipeline_field(const struct mf_field *);
>  bool mf_is_set(const struct mf_field *, const struct flow *);
> diff --git a/lib/meta-flow.c b/lib/meta-flow.c
> index 499be04b6..c0cc5c164 100644
> --- a/lib/meta-flow.c
> +++ b/lib/meta-flow.c
> @@ -1790,6 +1790,113 @@ mf_is_tun_metadata(const struct mf_field *mf)
> mf->id < MFF_TUN_METADATA0 + TUN_METADATA_NUM_OPTS;
>  }
>
> +bool
> +mf_is_metadata(const struct mf_field *mf)
> +{
> +switch (mf->id) {
> +case MFF_ARP_OP:
> +case MFF_ARP_SHA:
> +case MFF_ARP_SPA:
> +case MFF_ARP_THA:
> +case MFF_ARP_TPA:
> +case MFF_DL_VLAN:
> +case MFF_DL_VLAN_PCP:
> +case MFF_ETH_DST:
> +case MFF_ETH_SRC:
> +case MFF_ETH_TYPE:
> +case MFF_ICMPV4_CODE:
> +case MFF_ICMPV4_TYPE:
> +case MFF_ICMPV6_CODE:
> +case MFF_ICMPV6_TYPE:
> +case MFF_IPV4_DST:
> +case MFF_IPV4_SRC:
> +case MFF_IPV6_DST:
> +case MFF_IPV6_LABEL:
> +case MFF_IPV6_SRC:
> +case MFF_IP_DSCP:
> +case MFF_IP_DSCP_SHIFTED:
> +case MFF_IP_ECN:
> +case MFF_IP_FRAG:
> +case MFF_IP_PROTO:
> +case MFF_IP_TTL:
> +case MFF_MPLS_BOS:
> +case MFF_MPLS_LABEL:
> +case MFF_MPLS_TC:
> +case MFF_MPLS_TTL:
> +case MFF_ND_OPTIONS_TYPE:
> +case MFF_ND_RESERVED:
> +case MFF_ND_SLL:
> +case MFF_ND_TARGET:
> +case MFF_ND_TLL:
> +case MFF_NSH_C1:
> +case MFF_NSH_C2:
> +case MFF_NSH_C3:
> +case MFF_NSH_C4:
> +case MFF_NSH_FLAGS:
> +case MFF_NSH_MDTYPE:
> +case MFF_NSH_NP:
> +case MFF_NSH_SI:
> +case MFF_NSH_SPI:
> +case MFF_NSH_TTL:
> +case MFF_PACKET_TYPE:
> +case MFF_SCTP_DST:
> +case MFF_SCTP_SRC:
> +case MFF_TCP_DST:
> +case MFF_TCP_FLAGS:
> +case MFF_TCP_SRC:
> +case MFF_TUN_DST:
> +case MFF_TUN_ERSPAN_HWID:
> +case MFF_TUN_ERSPAN_IDX:
> +case MFF_TUN_ERSPAN_VER:
> +case MFF_TUN_FLAGS:
> +case MFF_TUN_GBP_FLAGS:
> +case MFF_TUN_GBP_ID:
> +case MFF_TUN_GTPU_FLAGS:
> +case MFF_TUN_GTPU_MSGTYPE:
> +case MFF_TUN_ID:
> +case MFF_TUN_IPV6_DST:
> +case MFF_TUN_IPV6_SRC:
> +case MFF_TUN_SRC:
> +case MFF_TUN_TOS:
> +case MFF_TUN_TTL:
> +case MFF_UDP_DST:
> +case MFF_UDP_SRC:
> +case MFF_VLAN_PCP:
> +case MFF_VLAN_TCI:
> +case MFF_VLAN_VID:
> +return false;

Add a new line, to be inline with mf_is_pipeline_field()

> +CASE_MFF_REGS:
> +CASE_MFF_TUN_METADATA:
> +CASE_MFF_XREGS:
> +CASE_MFF_XXREGS:
> +case MFF_ACTSET_OUTPUT:
> +case MFF_CONJ_ID:
> +case MFF_CT_IPV6_DST:
> +case MFF_CT_IPV6_SRC:
> +case MFF_CT_LABEL:
> +case MFF_CT_MARK:
> +case MFF_CT_NW_DST:
> +case MFF_CT_NW_PROTO:
> +case MFF_CT_NW_SRC:
> +case MFF_CT_STATE:
> +case MFF_CT_TP_DST:
> +case MFF_CT_TP_SRC:
> +case MFF_CT_ZONE:
> +case MFF_DP_HASH:
> +case MFF_IN_PORT:
> +case MFF_IN_PORT_OXM:
> +case MFF_METADATA:
> +case MFF_N_IDS:
> +case MFF_PKT_MARK:
> +case MFF_RECIRC_ID:
> +case MFF_SKB_PRIORITY:
> +case MFF_TUN_ERSPAN_D

Re: [ovs-dev] [PATCH v2 6/6] dpctl: Do not allow out of range values in ct-set-limits.

[Eelco Chaudron]

On 30 Sep 2024, at 22:50, Paolo Valerio wrote:

> The ovs_scan() doesn't enforce in-range values and so
> lsbits are stored in case of out-of-range or negative values.
>
> This way negative or values greater than MAX_UINT32 for "default" are
> all accepted in dpctl_ct_set_limits(), but they will eventually be
> casted to uint32_t, whereas for zones all the values above are
> considered invalid.
>
> Align their behaviors and extend the tests for checking values out of
> the range.
>
> Signed-off-by: Paolo Valerio 

Recheck-request: github-robot

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

Re: [ovs-dev] [PATCH v3 1/1] debian: Allow passing DEB_BUILD_OPTIONS.

[Eelco Chaudron]

On 30 Sep 2024, at 16:49, Roi Dayan wrote:

> Allow passing different DEB_BUILD_OPTIONS to make debian-deb.
>
> Signed-off-by: Roi Dayan 

Thanks Roi for keeping up with my comments :) This revision looks good to me.

Acked-by: Eelco Chaudron 

//Eelco

> ---
>
> Notes:
> v3
> - Remove unneeded export call.
> - Move assignment to an existing DPDK_NETDEV check.
>
> v2
> - Fix export of DEB_BUILD_OPTIONS in the Makefile
>
>  debian/automake.mk | 9 +++--
>  1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/debian/automake.mk b/debian/automake.mk
> index 7b2afafae1a2..7607a2cd5b3a 100644
> --- a/debian/automake.mk
> +++ b/debian/automake.mk
> @@ -98,10 +98,12 @@ if DPDK_NETDEV
>  update_deb_control = \
>   $(AM_V_GEN) sed -e 's/^\# DPDK_NETDEV //' \
>   < $(srcdir)/debian/control.in > debian/control
> +DEB_BUILD_OPTIONS ?= nocheck parallel=`nproc`
>  else
>  update_deb_control = \
>   $(AM_V_GEN) grep -v '^\# DPDK_NETDEV' \
>   < $(srcdir)/debian/control.in > debian/control
> +DEB_BUILD_OPTIONS ?= nocheck parallel=`nproc` nodpdk
>  endif
>
>  debian/control: $(srcdir)/debian/control.in Makefile
> @@ -123,10 +125,5 @@ debian-deb: debian
>   $(update_deb_copyright)
>   $(update_deb_control)
>   $(AM_V_GEN) fakeroot debian/rules clean
> -if DPDK_NETDEV
> - $(AM_V_GEN) DEB_BUILD_OPTIONS="nocheck parallel=`nproc`" \
> - fakeroot debian/rules binary
> -else
> - $(AM_V_GEN) DEB_BUILD_OPTIONS="nocheck parallel=`nproc` nodpdk" \
> + $(AM_V_GEN) DEB_BUILD_OPTIONS="${DEB_BUILD_OPTIONS}" \
>   fakeroot debian/rules binary
> -endif
> -- 
> 2.46.1

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

Re: [ovs-dev] [PATCH] testing: Document the 0-day robot support.

[Eelco Chaudron]

On 30 Sep 2024, at 19:54, Aaron Conole wrote:

> Eelco Chaudron  writes:
>
>> On 28 Sep 2024, at 15:03, Aaron Conole wrote:
>>
>>> The 0-day robot has been testing patches for 6 years, and we've
>>> had support for other labs to integrate for 3.  However, this
>>> isn't well documented, and has made it difficult for others to
>>> know how they can contribute to testing.  To that end, this
>>> patch introduces some documentation for the 0-day robot and
>>> how to integrate into the patch reporting process.
>>
>> Thanks for adding this documentation, some small comments below.
>>
>> Cheers,
>>
>> Eelco
>>
>>> Signed-off-by: Aaron Conole 
>>> ---
>>>  .../contributing/submitting-patches.rst   |   3 +-
>>>  Documentation/topics/testing.rst  | 105 ++
>>>  2 files changed, 107 insertions(+), 1 deletion(-)
>>>
>>> diff --git
>>> a/Documentation/internals/contributing/submitting-patches.rst
>>> b/Documentation/internals/contributing/submitting-patches.rst
>>> index 8a8bc11b0a..c01ac7bbdc 100644
>>> --- a/Documentation/internals/contributing/submitting-patches.rst
>>> +++ b/Documentation/internals/contributing/submitting-patches.rst
>>> @@ -70,7 +70,8 @@ Testing is also important:
>>>
>>>  If you are using GitHub, then you may utilize the GitHub Actions CI build
>>>  systems.  They will run some of the above tests automatically
>>> -when you push changes to your repository.
>>> +when you push changes to your repository.  See the "Continuous Integration"
>>> +section in :doc:`/topics/testing` for details on continuous integration.
>>>
>>>  Email Subject
>>>  -
>>> diff --git a/Documentation/topics/testing.rst
>>> b/Documentation/topics/testing.rst
>>> index dcf10a4db2..49ee7d7ffa 100644
>>> --- a/Documentation/topics/testing.rst
>>> +++ b/Documentation/topics/testing.rst
>>> @@ -545,3 +545,108 @@ Once you are done with experimenting you can
>>> tear down setup with::
>>>
>>>  Sometimes deployment of Proof of Concept may fail, if, for example, VMs
>>>  don't have network reachability to the Internet.
>>> +
>>> +
>>> +Continuous Integration
>>> +--
>>> +
>>> +The Open vSwitch project can make use of multiple public and hosted
>>> +CI services to help developers ensure their patches don't introduce
>>> +additional regressions.  Each service requires different forms of
>>> +configuration, and for the supported services the configuration
>>> +file(s) to automatically build Open vSwitch with various build
>>> +configurations and run the testsuites is/are provided in the
>>
>> I think test suites are two words.
>
> I guess it could be either.  From what I see in the documentation /
> comments, we use 'testsuite' more often than 'test suite'.
>
> I can switch it in v2.

Looks like we are not consistent in this document :) We use testsuite more than 
test suite, so I can you can keep it like this.

>>> +repostiory.  For example, the GitHub Actions builds will be performed
>>> +with gcc, clang, sparse, including DPDK, etc. with the -Werror
>>> +compiler flag included.  Therefore, the build will fail if a new
>>> +warning gets introduced by a change.
>>> +
>>> +Each ci system uses a different method of enablement, but most are 
>>> available
>>
>> CI with captials as in the rest of the text?
>
> Ack.
>
>>> +from the GitHub settings page.  By default, Open vSwitch includes a GitHub
>>> +actions running configuration, and this will automatically email
>>> the repository
>>> +owner.
>>> +
>>> +Currently, Open vSwitch project enables the following public CI
>>> services along
>>> +with the appropriate configuration settings::
>>> +
>>> + - AppVeyor: appveyor.yml
>>> + - Cirrus-CI: .cirrus.yml
>>> + - GitHub Actions: .github/workflows
>>> +
>>> +GitHub Actions is available without any additional configuration.
>>> +
>>> +Additionally, as each patch is posted to the mailing list, the public CI
>>> +machinery will run additional tests on the patches and report results.
>>> +
>>> +Public report / Private lab hybrid testing
>>> +--
>>> +
>>> +The Open vSwitch project make

[ovs-dev] [PATCH v4] checkpatch: Add new check-authors-file option to checkpatch.py.

[Eelco Chaudron]

This patch adds a new option, --check-authors-file, to the checkpatch
tool to help OVS maintainers check for missing authors in the
AUTHORS.rst file.

Signed-off-by: Eelco Chaudron 
---
v3: - Also include co-authors in the check.
- Only report the end, when all patches are checked.
- Fixed spelling mistake.
- Determine git root directory for AUTHORS.rst location.

v4: - Added a test case.
---
 tests/checkpatch.at | 21 +
 utilities/checkpatch.py | 68 +++--
 2 files changed, 86 insertions(+), 3 deletions(-)

diff --git a/tests/checkpatch.at b/tests/checkpatch.at
index 34971c514..fa179c707 100755
--- a/tests/checkpatch.at
+++ b/tests/checkpatch.at
@@ -634,3 +634,24 @@ try_checkpatch \
 "--skip-committer-signoff"
 
 AT_CLEANUP
+
+AT_SETUP([checkpatch - AUTHORS.rst existence])
+
+try_checkpatch \
+   "Author: A 
+Commit: A 
+Subject: netdev: Subject.
+
+Signed-off-by: A " \
+""
+
+try_checkpatch \
+   "Author: A 
+Commit: A 
+Subject: netdev: Subject.
+
+Signed-off-by: A " \
+"WARNING: Author 'A ' is not in the AUTHORS.rst file!" \
+"-a"
+
+AT_CLEANUP
diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index 742a0bc47..73c20f804 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -28,12 +28,14 @@ __errors = 0
 __warnings = 0
 empty_return_check_state = 0
 print_file_name = None
+check_authors_file = False
 checking_file = False
 total_line = 0
 colors = False
 spellcheck = False
 quiet = False
 spell_check_dict = None
+missing_authors = []
 
 
 def open_spell_check_dict():
@@ -860,9 +862,41 @@ def run_subject_checks(subject, spellcheck=False):
 return warnings
 
 
+def get_top_directory():
+with os.popen('git rev-parse --show-toplevel') as pipe:
+path = pipe.read()
+
+if path:
+return path.strip()
+
+return "."
+
+
+def do_authors_exist(authors):
+authors = list(set(authors))
+missing_authors = []
+
+try:
+with open(get_top_directory() + "/AUTHORS.rst", "r") as file:
+file_content = file.read()
+for author in authors:
+m = re.search(r'<(.*?)>', author)
+if not m:
+continue
+pattern = r'\b' + re.escape(m.group(1)) + r'\b'
+if re.search(pattern, file_content) is None:
+missing_authors.append(author)
+
+except FileNotFoundError:
+print_error("Could not open AUTHORS.rst in '%s/'!" %
+get_top_directory())
+
+return missing_authors
+
+
 def ovs_checkpatch_parse(text, filename, author=None, committer=None):
 global print_file_name, total_line, checking_file, \
-empty_return_check_state
+empty_return_check_state, missing_authors
 
 PARSE_STATE_HEADING = 0
 PARSE_STATE_DIFF_HEADER = 1
@@ -977,6 +1011,11 @@ def ovs_checkpatch_parse(text, filename, author=None, 
committer=None):
   "who are not authors or co-authors or "
   "committers: %s"
   % ", ".join(extra_sigs))
+
+if check_authors_file:
+missing_authors = do_authors_exist(missing_authors +
+   co_authors + [author])
+
 elif is_committer.match(line):
 committer = is_committer.match(line).group(2)
 elif is_author.match(line):
@@ -1067,6 +1106,8 @@ Input options:
 
 Check options:
 -h|--help  This help message
+-a|--check-authors-fileCheck AUTHORS file for existence of the authors.
+   Should be used by commiters only!
 -b|--skip-block-whitespace Skips the if/while/for whitespace tests
 -l|--skip-leading-whitespace   Skips the leading whitespace test
 -q|--quiet Only print error and warning information
@@ -1089,6 +1130,19 @@ def ovs_checkpatch_print_result():
 print("Lines checked: %d, no obvious problems found\n" % (total_line))
 
 
+def ovs_checkpatch_print_missing_authors():
+if missing_authors:
+if len(missing_authors) == 1:
+print_warning("Author '%s' is not in the AUTHORS.rst file!"
+  % missing_authors[0])
+else:
+print_warning("Authors '%s' are not in the AUTHORS.rst file!"
+  % ', '.join(missing_authors))
+return True
+
+return False
+
+
 def ovs_checkpatch_file(filename):
 try:
 mail = email.message_from_file(open(filename, 'r', encoding='utf8'))
@@ -1116,6 +1170,8 @@ def ovs_checkpat

Re: [ovs-dev] [PATCH v3] checkpatch: Add new check-authors-file option to checkpatch.py.

[Eelco Chaudron]

On 30 Sep 2024, at 14:39, Eelco Chaudron wrote:

> This patch adds a new option, --check-authors-file, to the checkpatch
> tool to help OVS maintainers check for missing authors in the
> AUTHORS.rst file.
>
> Signed-off-by: Eelco Chaudron 
> ---
> v3: - Also include co-authors in the check.
> - Only report the end, when all patches are checked.
> - Fixed spelling mistake.
> - Determine git root directory for AUTHORS.rst location.
> ---

Ignore this version I forgot to add a test case, and refresh the flake8 changes 
:(

//Eelco


>  utilities/checkpatch.py | 61 +++--
>  1 file changed, 59 insertions(+), 2 deletions(-)
>
> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
> index 742a0bc47..31b791d8f 100755
> --- a/utilities/checkpatch.py
> +++ b/utilities/checkpatch.py
> @@ -28,12 +28,14 @@ __errors = 0
>  __warnings = 0
>  empty_return_check_state = 0
>  print_file_name = None
> +check_authors_file = False
>  checking_file = False
>  total_line = 0
>  colors = False
>  spellcheck = False
>  quiet = False
>  spell_check_dict = None
> +missing_authors = []
>
>
>  def open_spell_check_dict():
> @@ -860,9 +862,41 @@ def run_subject_checks(subject, spellcheck=False):
>  return warnings
>
>
> +def get_top_directory():
> +with os.popen('git rev-parse --show-toplevel') as pipe:
> +path = pipe.read()
> +
> +if path:
> +return path.strip()
> +
> +return "."
> +
> +
> +def do_authors_exist(authors):
> +authors = list(set(authors))
> +missing_authors = []
> +
> +try:
> +with open(get_top_directory() + "/AUTHORS.rst", "r") as file:
> +file_content = file.read()
> +for author in authors:
> +m = re.search(r'<(.*?)>', author)
> +if not m:
> +continue
> +pattern = r'\b' + re.escape(m.group(1)) + r'\b'
> +if re.search(pattern, file_content) is None:
> +missing_authors.append(author)
> +
> +except FileNotFoundError:
> +print_error("Could not open AUTHORS.rst in '%s/'!" %
> +get_top_directory())
> +
> +return missing_authors
> +
> +
>  def ovs_checkpatch_parse(text, filename, author=None, committer=None):
>  global print_file_name, total_line, checking_file, \
> -empty_return_check_state
> +empty_return_check_state, missing_authors
>
>  PARSE_STATE_HEADING = 0
>  PARSE_STATE_DIFF_HEADER = 1
> @@ -977,6 +1011,11 @@ def ovs_checkpatch_parse(text, filename, author=None, 
> committer=None):
>"who are not authors or co-authors or "
>"committers: %s"
>% ", ".join(extra_sigs))
> +
> +if check_authors_file:
> +missing_authors = do_authors_exist(missing_authors +
> +   co_authors + [author])
> +
>  elif is_committer.match(line):
>  committer = is_committer.match(line).group(2)
>  elif is_author.match(line):
> @@ -1067,6 +1106,8 @@ Input options:
>
>  Check options:
>  -h|--help  This help message
> +-a|--check-authors-fileCheck AUTHORS file for existence of the 
> authors.
> +   Should be used by commiters only!
>  -b|--skip-block-whitespace Skips the if/while/for whitespace tests
>  -l|--skip-leading-whitespace   Skips the leading whitespace test
>  -q|--quiet Only print error and warning information
> @@ -1089,6 +1130,16 @@ def ovs_checkpatch_print_result():
>  print("Lines checked: %d, no obvious problems found\n" % 
> (total_line))
>
>
> +def ovs_checkpatch_print_missing_authors():
> +if missing_authors:
> +if len(missing_authors) == 1:
> +print_warning("Author '%s' is not in the AUTHORS.rst file!" \
> +  % missing_authors[0])
> +else:
> +print_warning("Authors '%s' are not in the AUTHORS.rst file!" \
> +  % ', '.join(missing_authors))
> +
> +
>  def ovs_checkpatch_file(filename):
>  try:
>  mail = email.message_from_file(open(filename, 'r', encoding='utf8'))
> @@ -1116,6 +1167,7 @@ def ovs_checkpatch_file

Re: [ovs-dev] [PATCH v2] checkpatch: Add new check-authors-file option to checkpatch.py.

[Eelco Chaudron]

On 27 Sep 2024, at 14:35, Aaron Conole wrote:

> Eelco Chaudron  writes:
>
>> This patch adds a new option, --check-authors-file, to the checkpatch
>> tool to help OVS maintainers check for missing authors in the
>> AUTHORS.rst file.
>>
>> Signed-off-by: Eelco Chaudron 
>> ---
>> v2: Fixed partial match, and long argument check.
>
> Hi Eelco,
>
> During the review I had some other thoughts about the way this feature
> would behave, and I think it would be good to add a test to the
> checkpatch tests for it to cover the behavior.

Oops!! I sent out a v3 addressing all your comments, but I missed the
test case part. Will send out a v4 with a test case.

//Eelco

>> ---
>>  utilities/checkpatch.py | 22 +-
>>  1 file changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
>> index 742a0bc47..636634472 100755
>> --- a/utilities/checkpatch.py
>> +++ b/utilities/checkpatch.py
>> @@ -28,6 +28,7 @@ __errors = 0
>>  __warnings = 0
>>  empty_return_check_state = 0
>>  print_file_name = None
>> +check_authors_file = False
>>  checking_file = False
>>  total_line = 0
>>  colors = False
>> @@ -977,6 +978,21 @@ def ovs_checkpatch_parse(text, filename, author=None, 
>> committer=None):
>>"who are not authors or co-authors or 
>> "
>>"committers: %s"
>>% ", ".join(extra_sigs))
>> +
>> +if check_authors_file or author:
>
> Should this be 'and' instead of 'or'?  All patches we receive will have
> an author so from what I can tell this check is always running.
>
> Also, we probably want to do the below for all the co_authors as well.
>
>> +m = re.search(r'<(.*?)>', author)
>> +if m:
>> +try:
>> +with open("AUTHORS.rst", "r") as file:
>> +file_content = file.read()
>> +pattern = r'\b' + re.escape(m.group(1)) + 
>> r'\b'
>> +if re.search(pattern, file_content) is None:
>> +print_error("Author '%s' is not in the "
>> +"AUTHORS.rst file!" % author)
>
> Maybe print_warn instead.  I can imagine someone running this and adding
> a patch where they add themselves to the AUTHORS.rst file in another
> commit.
>
>> +except FileNotFoundError:
>> +print_error("Can't open AUTHORS.rst file in "
>> +"current path!")
>> +
>
> This one I don't know - maybe we should make it so that we can use git
> to find the AUTHORS.rst file.  See the code snippet I give at the end
> (it's not fully tested).
>
> Also, maybe we want to delay printing these until the end of the patch.
> For example, if someone does run this and adds themselves to AUTHORS.rst
> as part of the patch the scan may not pick it up.  None of the other
> checks behave so differently between patch scan and file scan mode,
> iiuc.  If we delay scanning until the end we can avoid printing this in
> the case that the AUTHORS.rst file gets modified in a later hunk.
>
> WDYT?
>
>>  elif is_committer.match(line):
>>  committer = is_committer.match(line).group(2)
>>  elif is_author.match(line):
>> @@ -1067,6 +1083,7 @@ Input options:
>>
>>  Check options:
>>  -h|--help  This help message
>> +-a|--check-authors-fileCheck AUTHORS file for existense of author
>
> s/existense/existence/
>
> Maybe we should also flag somehow that this check should generally be
> run by a committer.
>
>>  -b|--skip-block-whitespace Skips the if/while/for whitespace tests
>>  -l|--skip-leading-whitespace   Skips the leading whitespace test
>>  -q|--quiet Only print error and warning information
>> @@ -1138,9 +1155,10 @@ if __name__ == '__main__':
>>sys.argv[1:])
>>  n_patches = int(numeric_options[-1][1:]) if numeric_options else 0
>>
>> -optlist, args = getopt.getopt(args, 'bhlstfSq',
>> +optlist, args = getopt.getopt(args, 

[ovs-dev] [PATCH v3] checkpatch: Add new check-authors-file option to checkpatch.py.

[Eelco Chaudron]

This patch adds a new option, --check-authors-file, to the checkpatch
tool to help OVS maintainers check for missing authors in the
AUTHORS.rst file.

Signed-off-by: Eelco Chaudron 
---
v3: - Also include co-authors in the check.
- Only report the end, when all patches are checked.
- Fixed spelling mistake.
- Determine git root directory for AUTHORS.rst location.
---
 utilities/checkpatch.py | 61 +++--
 1 file changed, 59 insertions(+), 2 deletions(-)

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index 742a0bc47..31b791d8f 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -28,12 +28,14 @@ __errors = 0
 __warnings = 0
 empty_return_check_state = 0
 print_file_name = None
+check_authors_file = False
 checking_file = False
 total_line = 0
 colors = False
 spellcheck = False
 quiet = False
 spell_check_dict = None
+missing_authors = []
 
 
 def open_spell_check_dict():
@@ -860,9 +862,41 @@ def run_subject_checks(subject, spellcheck=False):
 return warnings
 
 
+def get_top_directory():
+with os.popen('git rev-parse --show-toplevel') as pipe:
+path = pipe.read()
+
+if path:
+return path.strip()
+
+return "."
+
+
+def do_authors_exist(authors):
+authors = list(set(authors))
+missing_authors = []
+
+try:
+with open(get_top_directory() + "/AUTHORS.rst", "r") as file:
+file_content = file.read()
+for author in authors:
+m = re.search(r'<(.*?)>', author)
+if not m:
+continue
+pattern = r'\b' + re.escape(m.group(1)) + r'\b'
+if re.search(pattern, file_content) is None:
+missing_authors.append(author)
+
+except FileNotFoundError:
+print_error("Could not open AUTHORS.rst in '%s/'!" %
+get_top_directory())
+
+return missing_authors
+
+
 def ovs_checkpatch_parse(text, filename, author=None, committer=None):
 global print_file_name, total_line, checking_file, \
-empty_return_check_state
+empty_return_check_state, missing_authors
 
 PARSE_STATE_HEADING = 0
 PARSE_STATE_DIFF_HEADER = 1
@@ -977,6 +1011,11 @@ def ovs_checkpatch_parse(text, filename, author=None, 
committer=None):
   "who are not authors or co-authors or "
   "committers: %s"
   % ", ".join(extra_sigs))
+
+if check_authors_file:
+missing_authors = do_authors_exist(missing_authors +
+   co_authors + [author])
+
 elif is_committer.match(line):
 committer = is_committer.match(line).group(2)
 elif is_author.match(line):
@@ -1067,6 +1106,8 @@ Input options:
 
 Check options:
 -h|--help  This help message
+-a|--check-authors-fileCheck AUTHORS file for existence of the authors.
+   Should be used by commiters only!
 -b|--skip-block-whitespace Skips the if/while/for whitespace tests
 -l|--skip-leading-whitespace   Skips the leading whitespace test
 -q|--quiet Only print error and warning information
@@ -1089,6 +1130,16 @@ def ovs_checkpatch_print_result():
 print("Lines checked: %d, no obvious problems found\n" % (total_line))
 
 
+def ovs_checkpatch_print_missing_authors():
+if missing_authors:
+if len(missing_authors) == 1:
+print_warning("Author '%s' is not in the AUTHORS.rst file!" \
+  % missing_authors[0])
+else:
+print_warning("Authors '%s' are not in the AUTHORS.rst file!" \
+  % ', '.join(missing_authors))
+
+
 def ovs_checkpatch_file(filename):
 try:
 mail = email.message_from_file(open(filename, 'r', encoding='utf8'))
@@ -1116,6 +1167,7 @@ def ovs_checkpatch_file(filename):
 result = True
 
 ovs_checkpatch_print_result()
+ovs_checkpatch_print_missing_authors()
 return result
 
 
@@ -1138,9 +1190,10 @@ if __name__ == '__main__':
   sys.argv[1:])
 n_patches = int(numeric_options[-1][1:]) if numeric_options else 0
 
-optlist, args = getopt.getopt(args, 'bhlstfSq',
+optlist, args = getopt.getopt(args, 'abhlstfSq',
   ["check-file",
"help",
+   "check-authors-file",
"skip-block-whitespace",
 

Re: [ovs-dev] [PATCH] testing: Document the 0-day robot support.

[Eelco Chaudron]

On 28 Sep 2024, at 15:03, Aaron Conole wrote:

> The 0-day robot has been testing patches for 6 years, and we've
> had support for other labs to integrate for 3.  However, this
> isn't well documented, and has made it difficult for others to
> know how they can contribute to testing.  To that end, this
> patch introduces some documentation for the 0-day robot and
> how to integrate into the patch reporting process.

Thanks for adding this documentation, some small comments below.

Cheers,

Eelco

> Signed-off-by: Aaron Conole 
> ---
>  .../contributing/submitting-patches.rst   |   3 +-
>  Documentation/topics/testing.rst  | 105 ++
>  2 files changed, 107 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/internals/contributing/submitting-patches.rst 
> b/Documentation/internals/contributing/submitting-patches.rst
> index 8a8bc11b0a..c01ac7bbdc 100644
> --- a/Documentation/internals/contributing/submitting-patches.rst
> +++ b/Documentation/internals/contributing/submitting-patches.rst
> @@ -70,7 +70,8 @@ Testing is also important:
>
>  If you are using GitHub, then you may utilize the GitHub Actions CI build
>  systems.  They will run some of the above tests automatically
> -when you push changes to your repository.
> +when you push changes to your repository.  See the "Continuous Integration"
> +section in :doc:`/topics/testing` for details on continuous integration.
>
>  Email Subject
>  -
> diff --git a/Documentation/topics/testing.rst 
> b/Documentation/topics/testing.rst
> index dcf10a4db2..49ee7d7ffa 100644
> --- a/Documentation/topics/testing.rst
> +++ b/Documentation/topics/testing.rst
> @@ -545,3 +545,108 @@ Once you are done with experimenting you can tear down 
> setup with::
>
>  Sometimes deployment of Proof of Concept may fail, if, for example, VMs
>  don't have network reachability to the Internet.
> +
> +
> +Continuous Integration
> +--
> +
> +The Open vSwitch project can make use of multiple public and hosted
> +CI services to help developers ensure their patches don't introduce
> +additional regressions.  Each service requires different forms of
> +configuration, and for the supported services the configuration
> +file(s) to automatically build Open vSwitch with various build
> +configurations and run the testsuites is/are provided in the

I think test suites are two words.

> +repostiory.  For example, the GitHub Actions builds will be performed
> +with gcc, clang, sparse, including DPDK, etc. with the -Werror
> +compiler flag included.  Therefore, the build will fail if a new
> +warning gets introduced by a change.
> +
> +Each ci system uses a different method of enablement, but most are available

CI with captials as in the rest of the text?

> +from the GitHub settings page.  By default, Open vSwitch includes a GitHub
> +actions running configuration, and this will automatically email the 
> repository
> +owner.
> +
> +Currently, Open vSwitch project enables the following public CI services 
> along
> +with the appropriate configuration settings::
> +
> + - AppVeyor: appveyor.yml
> + - Cirrus-CI: .cirrus.yml
> + - GitHub Actions: .github/workflows
> +
> +GitHub Actions is available without any additional configuration.
> +
> +Additionally, as each patch is posted to the mailing list, the public CI
> +machinery will run additional tests on the patches and report results.
> +
> +Public report / Private lab hybrid testing
> +--
> +
> +The Open vSwitch project makes use of the ozlabs patchwork instance

Maybe put in a link to the instance?

> +to track patch status and management.  This patch tracking tool
> +provides information to maintainers on the state of patches proposed
> +for Open vSwitch.  The CI process for Open vSwitch makes use of the
> +checks feature of the ozlabs patchwork instance.  These allow developers

OzLabs

> +and maintainers to see what tests have been run, and report pass / fail
> +criteria.
> +
> +In order to know that a patch or series is ready for testing, the
> +Open vSwitch project makes use of the "0-day Robot", which is a hosted
> +jenkins instance running the pw-ci_ scripts.  These can monitor a

Jenkins

> +running patchwork instance for new patches and submit the patch details
> +to other build systems, like jenkins.
> +
> +Once a patch is tested, it would be good to report the results. To this
> +end, the Open vSwitch "0-day Robot" will accept emails sent to
> +ovs-bu...@openvswitch.org formatted in the correct way to be reflected
> +on this page.  This allows any lab to contribute to the testing and
> +validation of patches.  Note that the ovs-build list participation
> +requires subscribing the reporting email account to the list.
> +
> +To report a test status to a particular patch, send exactly one email to
> +the mailing formatted as such::
> +
> +  From: your email 
> +  To: ovs-bu...@openvswitch.org
> +  Date: Mon, 28 Jun 2021 00:00:

Re: [ovs-dev] [PATCH v2 1/1] debian: Allow passing DEB_BUILD_OPTIONS.

[Eelco Chaudron]

On 29 Sep 2024, at 11:06, Roi Dayan wrote:

> Allow passing different DEB_BUILD_OPTIONS to make debian-deb.

Hi Roi,

I did some experiments and it does not need the export part (or is it needed 
for something else?).

Also, you could move the definitions to the existing ‘if DPDK_NETDEV’ something 
like this:


@@ -95,10 +95,12 @@ CLEANFILES += debian/copyright


 if DPDK_NETDEV
+DEB_BUILD_OPTIONS ?= nocheck parallel=`nproc`
 update_deb_control = \
$(AM_V_GEN) sed -e 's/^\# DPDK_NETDEV //' \
< $(srcdir)/debian/control.in > debian/control
 else
+DEB_BUILD_OPTIONS ?= nocheck parallel=`nproc` nodpdk
 update_deb_control = \
$(AM_V_GEN) grep -v '^\# DPDK_NETDEV' \
< $(srcdir)/debian/control.in > debian/control
@@ -113,12 +115,6 @@ CLEANFILES += debian/control
 debian: debian/copyright debian/control
 .PHONY: debian

-if DPDK_NETDEV
-export DEB_BUILD_OPTIONS ?= nocheck parallel=`nproc`
-else
-export DEB_BUILD_OPTIONS ?= nocheck parallel=`nproc` nodpdk
-endif
-
 debian-deb: debian


//Eelco
> Signed-off-by: Roi Dayan 
> ---
>
> Notes:
> v2
> - Fix export of DEB_BUILD_OPTIONS in the Makefile
>
>  debian/automake.mk | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/debian/automake.mk b/debian/automake.mk
> index 7b2afafae1a2..7da8b041226a 100644
> --- a/debian/automake.mk
> +++ b/debian/automake.mk
> @@ -113,6 +113,11 @@ CLEANFILES += debian/control
>  debian: debian/copyright debian/control
>  .PHONY: debian
>
> +if DPDK_NETDEV
> +export DEB_BUILD_OPTIONS ?= nocheck parallel=`nproc`
> +else
> +export DEB_BUILD_OPTIONS ?= nocheck parallel=`nproc` nodpdk
> +endif
>
>  debian-deb: debian
>   @if test X"$(srcdir)" != X"$(top_builddir)"; then   
> \
> @@ -123,10 +128,5 @@ debian-deb: debian
>   $(update_deb_copyright)
>   $(update_deb_control)
>   $(AM_V_GEN) fakeroot debian/rules clean
> -if DPDK_NETDEV
> - $(AM_V_GEN) DEB_BUILD_OPTIONS="nocheck parallel=`nproc`" \
> + $(AM_V_GEN) DEB_BUILD_OPTIONS="${DEB_BUILD_OPTIONS}" \
>   fakeroot debian/rules binary
> -else
> - $(AM_V_GEN) DEB_BUILD_OPTIONS="nocheck parallel=`nproc` nodpdk" \
> - fakeroot debian/rules binary
> -endif
> -- 
> 2.46.1

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

Re: [ovs-dev] [PATCH] Revert "ci: Use sarif-tools v3.0.1 due to issues in earlier versions."

[Eelco Chaudron]

On 28 Sep 2024, at 14:36, Aaron Conole wrote:

> It seems that the sarif-tools package version 3.0+ cause some kind
> of instability with the build system.  Until it has a proper root
> cause, we shouldn't try to apply any fixes.
>
> This reverts commit f2ab45c66e2fe536e98f7f45d107d9f8c3209437.
>
> Signed-off-by: Aaron Conole 

Sound good to me, and sorry for all this hassle :(

//Eelco

Acked-by: Eelco Chaudron 

> ---
>  .ci/linux-prepare.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/.ci/linux-prepare.sh b/.ci/linux-prepare.sh
> index b537163b8e..5f8a1db6af 100755
> --- a/.ci/linux-prepare.sh
> +++ b/.ci/linux-prepare.sh
> @@ -23,7 +23,7 @@ cd ..
>  # https://github.com/pypa/pip/issues/10655
>  pip3 install --disable-pip-version-check --user wheel
>  pip3 install --disable-pip-version-check --user \
> -flake8 netaddr pyparsing sarif-tools>=3.0.1 sphinx setuptools
> +flake8 netaddr pyparsing sarif-tools==2.0.0 sphinx setuptools
>
>  # Install python test dependencies
>  pip3 install -r python/test_requirements.txt
> -- 
> 2.46.1

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

Re: [ovs-dev] [PATCH] ci: Use sarif-tools v3.0.1 due to issues in earlier versions.

[Eelco Chaudron]

On 27 Sep 2024, at 14:51, Aaron Conole wrote:

> Eelco Chaudron  writes:
>
>> Sarif-tools v3.0 introduced an issue that has been resolved in v3.0.1.
>> Ensure that v3.0.1 or higher is installed via pip.
>>
>> Fixes: 234e626198a4 ("ci: Use previous sarif-tools release due to
>> issue in latest release.")
>> Signed-off-by: Eelco Chaudron 
>> ---
>
> Thanks, Eelco.  Applied and backported to branch-3.4 and branch-3.3.

Thanks Aaron for applying and doing the backport.

//Eelco

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

Re: [ovs-dev] [PATCH v1] netdev-dpdk: Added xstat statistics interface.

[Eelco Chaudron]

On 19 Sep 2024, at 12:46, Jun Wang wrote:

>> On Wed, Sep 18, 2024 at 4:01 AM Jun Wang  wrote:
>>>
>>>> On Mon, Sep 16, 2024 at 9:46 AM Eelco Chaudron  wrote:
>>>>> On 14 Sep 2024, at 7:26, Jun Wang wrote:
>>>>>
>>>>>> For many network cards, xstat statistics cannot be queried in the
>>>>>> ovs interface. A new interface is added to retrieve all xstat
>>>>>> information of the network card.
>>>>> Hi Jun,
>>>>>
>>>>> Isn’t this already handled with netdev_dpdk_get_custom_stats()? I think 
>>>>> you can see them with ‘ovs-vsctl get Interface  statistics.
>>>>>
>>>>> If this is not the case this is what we should enhance.
>>>>
>>>> xstats are currently filtered to only save per queue stats and some
>>>> common counters.
>>>
>>>> static bool
>>>> is_queue_stat(const char *s)
>>>> {
>>>> uint16_t tmp;
>>>>
>>>> return (s[0] == 'r' || s[0] == 't') &&
>>>> (ovs_scan(s + 1, "x_q%"SCNu16"_packets", &tmp) ||
>>>>  ovs_scan(s + 1, "x_q%"SCNu16"_bytes", &tmp));
>>>> }
>>>>
>>>> static void
>>>> netdev_dpdk_configure_xstats(struct netdev_dpdk *dev)
>>>> OVS_REQUIRES(dev->mutex)
>>>> {
>>>> ...
>>>>
>>>> /* For custom stats, we filter out everything except per
>>>> rxq/txq basic
>>>>  * stats, and dropped, error and management counters. */
>>>> if (is_queue_stat(name) ||
>>>> string_ends_with(name, "_errors") ||
>>>> strstr(name, "_management_") ||
>>>> string_ends_with(name, "_dropped")) {
>>>>
>>>> dev->rte_xstats_ids[dev->rte_xstats_ids_size] = id;
>>>> dev->rte_xstats_ids_size++;
>>>> }
>>>>
>>>>
>>>> Could you explain which stats you are missing?
>>>
>>> Hi David,
>>> As you mentioned, I also found that the results obtained by
>>> netdev_dpdk_get_custom_stats are filtered. However, I noticed that in
>>> DPDK, different net drivers handle xstats commands in various ways.
>>> If we apply a unified filtering process, it may result in not being
>>> able to retrieve the desired results for different net drivers.
>>> Therefore, I believe adding a new interface might be a suitable
>>> solution. For example, the xstats of the ixgbe/i40e/txgbe driver.
>>
>> Well, the problem is that drivers came up with all the stats they
>> could get from the hw with no unification.
>> This is why I was requesting some example of the stats you need, to
>> see if DPDK can align drivers, and then OVS filter can be extended.
>>
>> On the other hand, for *debugging*, you may query those vendor
>> specific stats via the DPDK telemetry socket.
>
> I tried using DPDK telemetry with the X710 network card, and it was
> indeed able to retrieve all xstat statistics. However, there seems to
> be an issue with retrieving xstat data from the Wangxun network card
> using the txgbe driver.
> This might be an issue with the specific driver.
>
> X710 i40e driver:
> --> /ethdev/xstats,0
> {
>   "/ethdev/xstats": {
> "rx_good_packets": 4158196,
> "tx_good_packets": 817922,
> "rx_good_bytes": 307686767,
> "tx_good_bytes": 64879395,
> "rx_missed_errors": 0,
> "rx_errors": 0,
> "tx_errors": 0,
> "rx_mbuf_allocation_errors": 0,
> "rx_unicast_packets": 804771,
> "rx_multicast_packets": 387605,
> "rx_broadcast_packets": 2947488,
> "rx_dropped_packets": 0,
> "rx_unknown_protocol_packets": 4158200,
> "rx_size_error_packets": 0,
> "tx_unicast_packets": 799583,
> "tx_multicast_packets": 18288,
> "tx_broadcast_packets": 51,
> "tx_dropped_packets": 0,
> "tx_link_down_dropped": 0,
> "rx_crc_errors": 0,
> "rx_illegal_byte_errors": 0,
> "rx_error_bytes": 0,
> "mac_local_errors": 1,
> "mac_remote_errors": 1,
> "rx_length_errors": 0,
> "

Re: [ovs-dev] [RFC PATCH 1/3] exact-match-offload: Introducing Exact-Match HW Offload Support for OVS

[Eelco Chaudron]

On 25 Sep 2024, at 11:46, Farhat Ullah wrote:

> 
> From: dev  on behalf of Eelco Chaudron 
> 
> Sent: Friday, September 20, 2024 4:46 PM
> To: Farhan Tariq 
> Cc: ovs-dev@openvswitch.org 
> Subject: Re: [ovs-dev] [RFC PATCH 1/3] exact-match-offload: Introducing 
> Exact-Match HW Offload Support for OVS
>
>
>
> On 12 Sep 2024, at 14:51, Farhan Tariq wrote:
>
>> This patch introduces exact-match offload support in OVS to enhance
>> optimization for hardware datapath. It's being submitted as a RFC for early
>> feedback.
>>
>> Problem Statement:
>> Currently, OVS offloads megaflows using tc-flower. However, hardware
>> support for megaflows has certain limitations:
>> a. TCAMs are used, which aren't scalable and support fewer rules.
>> b. Using hash tables to emulate TCAMs results in complex and slow
>> performance with many tables.
>>
>> Solution:
>> This patch aims to add exact-match offload support to OVS, addressing
>> these issues. This feature is an optional enhancement and is disabled by
>> default.
>>
>> Implementation Details:
>> 1. Added a new flag, "exact-match-offload" to "ovs-vsctl...other_config",
>>which is disabled by default but can be enabled at runtime. It works only
>>if megaflows are disabled.
>> 2. Introduced the "enable_exact_match_offload()" API in
>>"/lib/netdev-offload-tc.c" to zero-out unused masks.
>> 3. Moved "enable_megaflows" from "ofproto/ofproto-dpif-upcall.c" to "lib/-
>>netdev-offload-tc.c" to make it visible in the "netdev-offload-tc.c"
>>file. An alternate implementation could be to use a getter api.
>>
>> Testing results:
>> Verified the basic functionality of the patch with different flows.
>>
>> Future Work:
>> 1. Add support for connection tracking.
>> 2. Add support for OVS-DPDK rte_flow.
>>
>> Note: This feature does not affect user OpenFlow tables.
>> ---
>> Potential Issue:
>> This patch might not handle short-lived large numbers of new connections
>> effectively. The last proposal (i.e. proposal #4 listed below) can address
>> this issue. For now this feature is beneficial for many use cases, with
>> further optimization for handling large numbers of short-lived connections
>> planned for future work.
>>
>> Other Possible Solutions:
>> Several approaches to implementing exact-match offload are being
>> considered. Comments are welcome on the best approach:
>> 1. Offload all fields except mutable and unpredictable ones (e.g., ttl,
>>tos, ip_frag, checksum). Megaflows are disabled.
>> 2. Offload only essential fields (e.g., 5-tuple + VLAN IDs + tunnel info
>>etc). Megaflows are disabled.
>> 3. Provide configuration options for users to select which fields to
>>offload. Megaflows are disabled.
>> 4. Introduce an offloading path from the OVS datapath (kernel) module to
>>integrate megaflows and exact-match offload, addressing short-lived new
>>connection performance. Suggestions on this approach are welcome.
>> --
>
> Thanks, Eelco,
>
> Thanks, Farhat, for submitting the RFC series. I noticed something that might 
> be a mistake in the subject line—it says "patch 1/3,” but only patch 1 was 
> sent.
> We are planning to submit 2 more patches in the future. I think I 
> misunderstood the use of series of patches.
>
> Regarding your patch, I see that it relies on disabling mega-flow support. 
> However, this isn’t something typically done in a production environment. The 
> ‘ovs-appctl upcall/disable-megaflows’ command was originally introduced for 
> debugging purposes when megaflows were first implemented. Since no one runs 
> or tests with this disabled, it’s generally considered a debug-only command.
>
> While I understand the value of your patch in simplifying hardware offloading 
> of datapath flows, disabling megaflows doesn’t seem like a practical solution 
> for production use. Perhaps it would be beneficial to collaborate with other 
> hardware vendors within the OVS community to find a more viable approach. It 
> might be worth reaching out to other vendors like, Broadcom, Marvell, NVIDIA, 
> etc. on this mailing list.

> What's your take on introducing offload path from megaflows to microflow and 
> microflow to tc-flower in the datapath (kernel) module.

I’m not entirely sure what you're proposing from an OVS architecture 
perspective, but it seems like it might require a new dpif implementation. 
Alternatively, we could consider adding t

Re: [ovs-dev] [PATCH 1/1] debian: Allow passing DEB_BUILD_OPTIONS.

[Eelco Chaudron]

On 26 Sep 2024, at 9:24, Roi Dayan via dev wrote:

> Allow passing different DEB_BUILD_OPTIONS to make debian-deb.
>
> Signed-off-by: Roi Dayan 

Hi Roy,

Not sure if you noticed, but the robot failed to build you change, i.e. the 
github CI failed to build debian.

https://github.com/ovsrobot/ovs/actions/runs/11047719338

I’ll mark this patch, as needs a new revision for now.

Cheers,

Eelco


> ---
>  debian/automake.mk | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/debian/automake.mk b/debian/automake.mk
> index 7b2afafae1a2..ebdcb022b3ef 100644
> --- a/debian/automake.mk
> +++ b/debian/automake.mk
> @@ -113,6 +113,11 @@ CLEANFILES += debian/control
>  debian: debian/copyright debian/control
>  .PHONY: debian
>
> +if DPDK_NETDEV
> + DEB_BUILD_OPTIONS ?= "nocheck parallel=`nproc`"
> +else
> + DEB_BUILD_OPTIONS ?= "nocheck parallel=`nproc` nodpdk"
> +endif
>
>  debian-deb: debian
>   @if test X"$(srcdir)" != X"$(top_builddir)"; then   
> \
> @@ -123,10 +128,5 @@ debian-deb: debian
>   $(update_deb_copyright)
>   $(update_deb_control)
>   $(AM_V_GEN) fakeroot debian/rules clean
> -if DPDK_NETDEV
> - $(AM_V_GEN) DEB_BUILD_OPTIONS="nocheck parallel=`nproc`" \
> + $(AM_V_GEN) DEB_BUILD_OPTIONS="${DEB_BUILD_OPTIONS}" \
>   fakeroot debian/rules binary
> -else
> - $(AM_V_GEN) DEB_BUILD_OPTIONS="nocheck parallel=`nproc` nodpdk" \
> - fakeroot debian/rules binary
> -endif
> -- 
> 2.46.1
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

Re: [ovs-dev] [PATCH] ofproto-dpif: Improve load balancing in dp_hash select groups.

[Eelco Chaudron]

s for cases with low number of backends.  The standard
> deviation also doesn't go down that much with higher coefficient.
>
> The data is aggregated for up to 64 backends because with higher
> numbers we're getting close to the cap of 256 hashes, so deviation
> increases.  However, the load difference with so many backends should
> have lower impact on a cluster in general.  The change improves the
> cases with 64+ backends, but to get very good numbers we'll need to
> consider moving the upper limit for the hash space, which is a separate
> change to think about.
>
> Added test checks that standard deviation of the load distribution
> between buckets is below 12.5% for all cases up to 64 buckets.  It's
> just a pretty number that I've chosen that should be IMO good enough.
>
> Note: when number of buckets is a power of 2, then we have an ideal
> distribution with zero deviation, because hashes can be evenly mapped
> to buckets.
>
> Note 2: The change doesn't affect distribution for 4 or less backends,
> since there are still minimum 16 hashes for those groups.
>
> Signed-off-by: Ilya Maximets 

Thanks for the patch and the extensive log message description.
The change looks good to me. Did some basic testing, but I do not see how this 
would break any real traffic.

Cheers,

Eelco

Acked-by: Eelco Chaudron 


> ---
>
> Not sure if this should be considered as a bug fix and be backported.
> Thoughts on this are welcome.
>
>  ofproto/ofproto-dpif.c |  6 +++--
>  tests/ofproto-dpif.at  | 54 ++
>  2 files changed, 58 insertions(+), 2 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 4659d8a3b..bf43d5d4b 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -5341,8 +5341,10 @@ group_setup_dp_hash_table(struct group_dpif *group, 
> size_t max_hash)
>   min_weight, total_weight);
>
>  uint64_t min_slots = DIV_ROUND_UP(total_weight, min_weight);
> -uint64_t min_slots2 = ROUND_UP_POW2(min_slots);
> -uint64_t n_hash = MAX(16, min_slots2);
> +uint64_t min_slots2 =
> +MAX(min_slots, MIN(n_buckets * 4, MAX_SELECT_GROUP_HASH_VALUES));
> +uint64_t min_slots3 = ROUND_UP_POW2(min_slots2);
> +uint64_t n_hash = MAX(16, min_slots3);
>  if (n_hash > MAX_SELECT_GROUP_HASH_VALUES ||
>  (max_hash != 0 && n_hash > max_hash)) {
>  VLOG_DBG("  Too many hash values required: %"PRIu64, n_hash);
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 12cb7f7a6..1c99dc3bb 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -1238,6 +1238,60 @@ bucket3 >= 500
>  OVS_VSWITCHD_STOP
>  AT_CLEANUP
>
> +AT_SETUP([ofproto-dpif - select group with dp_hash and equal weights])
> +
> +OVS_VSWITCHD_START
> +add_of_ports br0 1 10
> +
> +AT_CHECK([ovs-appctl vlog/set ofproto_dpif:file:dbg vconn:file:info])
> +
> +AT_DATA([stddev.awk], [
> +  {
> +# $1 (target) is a mean value, because all weights are the same.
> +# $2 (hits) is an actual number of hashes assigned to this bucket.
> +n_hashes += $2
> +n_buckets++
> +sum_sq_diff += ($2 - $1) * ($2 - $1)
> +  }
> +  END {
> +mean = n_hashes / n_buckets
> +stddev = sqrt(sum_sq_diff / n_buckets)
> +stddevp = stddev * 100 / mean
> +
> +print "hashes:", n_hashes, "buckets:", n_buckets
> +print "mean:", mean, "stddev:", stddev, "(", stddevp, "% )"
> +
> +# Make sure that standard deviation of load between buckets is below 
> 12.5%.
> +# Note: it's not a strict requirement, but a good number that passes 
> tests.
> +if (stddevp <= 12.5) { print "PASS" }
> +else { print "FAIL" }
> +  }
> +])
> +
> +m4_define([CHECK_DISTRIBUTION], [
> +  AT_CHECK([tail -n $1 ovs-vswitchd.log | grep 'ofproto_dpif|DBG|.*Bucket' \
> +| sed 's/.*target=\([[0-9\.]]*\) hits=\([[0-9]]*\)/\1 \2/' \
> +| awk -f stddev.awk], [0], [stdout])
> +  AT_CHECK([grep -q "buckets: $2" stdout])
> +  AT_CHECK([grep -q 'PASS' stdout])
> +])
> +
> +m4_define([OF_GROUP], [group_id=$1,type=select,selection_method=dp_hash])
> +m4_define([OFG_BUCKET], [bucket=weight=$1,output:10])
> +
> +dnl Test load distribution in groups with up to 64 equally weighted buckets.
> +m4_define([OFG_BUCKETS], [OFG_BUCKET(100)])
> +m4_for([id], [1], [64], [1], [
> +  get_log_next_line_num
> +  AT_CHECK([ovs-ofctl -O OpenFlow15 add-group br0 \
> +"OF_GROUP(id),OFG_BUCKETS()"])
> +  CHECK_DISTRIBUTION([+$LINENUM], [id])
> +  m4_append([OFG_BUCKETS], [,OFG_BUCKET(100)])
> +])
> +
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
>  AT_SETUP([ofproto-dpif - select group with explicit dp_hash selection 
> method])
>
>  OVS_VSWITCHD_START
> -- 
> 2.46.0
>
> ___
> 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 v4] netdev-offload: Fix incorrect comments.

[Eelco Chaudron]

On 24 Sep 2024, at 16:16, Simon Horman wrote:

> On Mon, Sep 23, 2024 at 03:30:21PM +0200, Eelco Chaudron wrote:
>>
>>
>> On 23 Sep 2024, at 14:59, Sunyang Wu via dev wrote:
>>
>>> This patch fixes incorrect comments.
>>>
>>> Signed-off-by: Sunyang Wu 
>>
>> Thanks for making all these changes, it looks good to me now.
>>
>> Acked-by: Eelco Chaudron 
>
> Likewise, this looks good to me.
>
> Acked-by: Simon Horman 

Thanks Simon and Sunyang. Patch applied to main.

//Eelco

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

Re: [ovs-dev] [PATCH 1/2] tests: ovsdb: Update missing ovsdb keywords.

[Eelco Chaudron]

On 24 Sep 2024, at 9:02, Roi Dayan via dev wrote:

> Some ovsdb tests were missing keywords that exists in all
> other ovsdb tests.
>
> Signed-off-by: Roi Dayan 

Thanks Roi for the patches, and Simon for the review.

Series applied to main.

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

[ovs-dev] [PATCH] ci: Use sarif-tools v3.0.1 due to issues in earlier versions.

[Eelco Chaudron]

Sarif-tools v3.0 introduced an issue that has been resolved in v3.0.1.
Ensure that v3.0.1 or higher is installed via pip.

Fixes: 234e626198a4 ("ci: Use previous sarif-tools release due to issue in 
latest release.")
Signed-off-by: Eelco Chaudron 
---
 .ci/linux-prepare.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.ci/linux-prepare.sh b/.ci/linux-prepare.sh
index 5f8a1db6a..b537163b8 100755
--- a/.ci/linux-prepare.sh
+++ b/.ci/linux-prepare.sh
@@ -23,7 +23,7 @@ cd ..
 # https://github.com/pypa/pip/issues/10655
 pip3 install --disable-pip-version-check --user wheel
 pip3 install --disable-pip-version-check --user \
-flake8 netaddr pyparsing sarif-tools==2.0.0 sphinx setuptools
+flake8 netaddr pyparsing sarif-tools>=3.0.1 sphinx setuptools
 
 # Install python test dependencies
 pip3 install -r python/test_requirements.txt
-- 
2.46.0

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

Re: [ovs-dev] [PATCH v2] checkpatch: Add new check-authors-file option to checkpatch.py.

[Eelco Chaudron]

On 25 Sep 2024, at 21:02, Aaron Conole wrote:

> Eelco Chaudron  writes:
>
>> This patch adds a new option, --check-authors-file, to the checkpatch
>> tool to help OVS maintainers check for missing authors in the
>> AUTHORS.rst file.
>>
>> Signed-off-by: Eelco Chaudron 
>> ---
>> v2: Fixed partial match, and long argument check.
>
> Not sure about it.  For example, is it really a problem with the patch
> if the author doesn't appear?  Maybe this could be a different checking
> utility?  I certainly don't think it would be an error in the patch.

It’s not an error in the patch itself, but an error to be resolved before we 
apply it.

As we have to duplicate a lot of infra for a stand-alone tool, and this option 
is not being enabled by default, I feel like it’s ok to enhance checkpatch. It 
also simplifies commiters the workflow, as now I can just run ‘checkpatch.py -3 
-S -a’.

Anyone else thoughts, feedback on this?

//Eelco

>> ---
>>  utilities/checkpatch.py | 22 +-
>>  1 file changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
>> index 742a0bc47..636634472 100755
>> --- a/utilities/checkpatch.py
>> +++ b/utilities/checkpatch.py
>> @@ -28,6 +28,7 @@ __errors = 0
>>  __warnings = 0
>>  empty_return_check_state = 0
>>  print_file_name = None
>> +check_authors_file = False
>>  checking_file = False
>>  total_line = 0
>>  colors = False
>> @@ -977,6 +978,21 @@ def ovs_checkpatch_parse(text, filename, author=None, 
>> committer=None):
>>"who are not authors or co-authors or 
>> "
>>"committers: %s"
>>% ", ".join(extra_sigs))
>> +
>> +if check_authors_file or author:
>> +m = re.search(r'<(.*?)>', author)
>> +if m:
>> +try:
>> +with open("AUTHORS.rst", "r") as file:
>> +file_content = file.read()
>> +pattern = r'\b' + re.escape(m.group(1)) + 
>> r'\b'
>> +if re.search(pattern, file_content) is None:
>> +print_error("Author '%s' is not in the "
>> +"AUTHORS.rst file!" % author)
>> +except FileNotFoundError:
>> +print_error("Can't open AUTHORS.rst file in "
>> +"current path!")
>> +
>>  elif is_committer.match(line):
>>  committer = is_committer.match(line).group(2)
>>  elif is_author.match(line):
>> @@ -1067,6 +1083,7 @@ Input options:
>>
>>  Check options:
>>  -h|--help  This help message
>> +-a|--check-authors-fileCheck AUTHORS file for existense of author
>>  -b|--skip-block-whitespace Skips the if/while/for whitespace tests
>>  -l|--skip-leading-whitespace   Skips the leading whitespace test
>>  -q|--quiet Only print error and warning information
>> @@ -1138,9 +1155,10 @@ if __name__ == '__main__':
>>sys.argv[1:])
>>  n_patches = int(numeric_options[-1][1:]) if numeric_options else 0
>>
>> -optlist, args = getopt.getopt(args, 'bhlstfSq',
>> +optlist, args = getopt.getopt(args, 'abhlstfSq',
>>["check-file",
>> "help",
>> +   "check-authors-file",
>> "skip-block-whitespace",
>> "skip-leading-whitespace",
>> "skip-signoff-lines",
>> @@ -1157,6 +1175,8 @@ if __name__ == '__main__':
>>  if o in ("-h", "--help"):
>>  usage()
>>  sys.exit(0)
>> +elif o in ("-a", "--check-authors-file"):
>> +check_authors_file = True
>>  elif o in ("-b", "--skip-block-whitespace"):
>>  skip_block_whitespace_check = True
>>  elif o in ("-l", "--skip-leading-whitespace"):

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

Re: [ovs-dev] [PATCH v6 13/13] documentation: Document ovs-flowviz.

[Eelco Chaudron]

On 25 Sep 2024, at 12:52, Adrian Moreno wrote:

> Add a man page for ovs-flowviz as well as a topic page with some more
> detailed examples.
>
> Signed-off-by: Adrian Moreno 

Thanks Adrian for fixing all the spelling mistakes! The series now 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] checkpatch: Add new check-authors-file option to checkpatch.py.

[Eelco Chaudron]

On 24 Sep 2024, at 16:43, Simon Horman wrote:

> On Mon, Sep 23, 2024 at 02:46:31PM +0200, Eelco Chaudron wrote:
>> This patch adds a new option, --check-authors-file, to the checkpatch
>> tool to help OVS maintainers check for missing authors in the
>> AUTHORS.rst file.
>
> Nice. I have missed checking this too many times.

Thanks for the review Simon! I’ve made the changes you suggested below and sent 
out a v2.

Cheers,

Eelco

>>
>> Signed-off-by: Eelco Chaudron 
>> ---
>>  utilities/checkpatch.py | 19 ++-
>>  1 file changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
>> index 742a0bc47..fe18b1674 100755
>> --- a/utilities/checkpatch.py
>> +++ b/utilities/checkpatch.py
>> @@ -28,6 +28,7 @@ __errors = 0
>>  __warnings = 0
>>  empty_return_check_state = 0
>>  print_file_name = None
>> +check_authors_file = False
>>  checking_file = False
>>  total_line = 0
>>  colors = False
>> @@ -977,6 +978,19 @@ def ovs_checkpatch_parse(text, filename, author=None, 
>> committer=None):
>>"who are not authors or co-authors or 
>> "
>>"committers: %s"
>>% ", ".join(extra_sigs))
>> +
>> +if check_authors_file or author:
>> +m = re.search(r'<(.*?)>', author)
>> +if m:
>> +try:
>> +with open("AUTHORS.rst", "r") as file:
>> +if m.group(1) not in file.read():
>> +print_error("Author '%s' is not in the "
>> +"AUTHORS.rst file!" % author)
>
> I suppose a false positive is unlikely, but this will pass
> on partial matches of email addresses.
>
> F.e. o...@ovn.or will match the entry for myself
>
>> +except FileNotFoundError:
>> +print_error("Can't open AUTHORS.rst file in "
>> +"current path!")
>> +
>>  elif is_committer.match(line):
>>  committer = is_committer.match(line).group(2)
>>  elif is_author.match(line):
>> @@ -1067,6 +1081,7 @@ Input options:
>>
>>  Check options:
>>  -h|--help  This help message
>> +-a|--check-authors-fileCheck AUTHORS file for existense of author
>>  -b|--skip-block-whitespace Skips the if/while/for whitespace tests
>>  -l|--skip-leading-whitespace   Skips the leading whitespace test
>>  -q|--quiet Only print error and warning information
>> @@ -1138,7 +1153,7 @@ if __name__ == '__main__':
>>sys.argv[1:])
>>  n_patches = int(numeric_options[-1][1:]) if numeric_options else 0
>>
>> -optlist, args = getopt.getopt(args, 'bhlstfSq',
>> +optlist, args = getopt.getopt(args, 'abhlstfSq',
>>["check-file",
>> "help",
>> "skip-block-whitespace",
>
> I think you need this too:
>
> @@ -1156,6 +1156,7 @@ if __name__ == '__main__':
>  optlist, args = getopt.getopt(args, 'abhlstfSq',
>["check-file",
> "help",
> +   "check-authors-file",
> "skip-block-whitespace",
> "skip-leading-whitespace",
> "skip-signoff-lines",
>
>> @@ -1157,6 +1172,8 @@ if __name__ == '__main__':
>>  if o in ("-h", "--help"):
>>  usage()
>>  sys.exit(0)
>> +elif o in ("-a", "--check-authors-file"):
>> +check_authors_file = True
>>  elif o in ("-b", "--skip-block-whitespace"):
>>  skip_block_whitespace_check = True
>>  elif o in ("-l", "--skip-leading-whitespace"):
>> -- 
>> 2.46.0
>>
>> ___
>> dev mailing list
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>

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

[ovs-dev] [PATCH v2] checkpatch: Add new check-authors-file option to checkpatch.py.

[Eelco Chaudron]

This patch adds a new option, --check-authors-file, to the checkpatch
tool to help OVS maintainers check for missing authors in the
AUTHORS.rst file.

Signed-off-by: Eelco Chaudron 
---
v2: Fixed partial match, and long argument check.

---
 utilities/checkpatch.py | 22 +-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index 742a0bc47..636634472 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -28,6 +28,7 @@ __errors = 0
 __warnings = 0
 empty_return_check_state = 0
 print_file_name = None
+check_authors_file = False
 checking_file = False
 total_line = 0
 colors = False
@@ -977,6 +978,21 @@ def ovs_checkpatch_parse(text, filename, author=None, 
committer=None):
   "who are not authors or co-authors or "
   "committers: %s"
   % ", ".join(extra_sigs))
+
+if check_authors_file or author:
+m = re.search(r'<(.*?)>', author)
+if m:
+try:
+with open("AUTHORS.rst", "r") as file:
+file_content = file.read()
+pattern = r'\b' + re.escape(m.group(1)) + r'\b'
+if re.search(pattern, file_content) is None:
+print_error("Author '%s' is not in the "
+"AUTHORS.rst file!" % author)
+except FileNotFoundError:
+print_error("Can't open AUTHORS.rst file in "
+"current path!")
+
 elif is_committer.match(line):
 committer = is_committer.match(line).group(2)
 elif is_author.match(line):
@@ -1067,6 +1083,7 @@ Input options:
 
 Check options:
 -h|--help  This help message
+-a|--check-authors-fileCheck AUTHORS file for existense of author
 -b|--skip-block-whitespace Skips the if/while/for whitespace tests
 -l|--skip-leading-whitespace   Skips the leading whitespace test
 -q|--quiet Only print error and warning information
@@ -1138,9 +1155,10 @@ if __name__ == '__main__':
   sys.argv[1:])
 n_patches = int(numeric_options[-1][1:]) if numeric_options else 0
 
-optlist, args = getopt.getopt(args, 'bhlstfSq',
+optlist, args = getopt.getopt(args, 'abhlstfSq',
   ["check-file",
"help",
+   "check-authors-file",
"skip-block-whitespace",
"skip-leading-whitespace",
"skip-signoff-lines",
@@ -1157,6 +1175,8 @@ if __name__ == '__main__':
 if o in ("-h", "--help"):
 usage()
 sys.exit(0)
+elif o in ("-a", "--check-authors-file"):
+check_authors_file = True
 elif o in ("-b", "--skip-block-whitespace"):
 skip_block_whitespace_check = True
 elif o in ("-l", "--skip-leading-whitespace"):
-- 
2.46.0

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

Re: [ovs-dev] [PATCH 2/2] tests: ovsdb: Add ovsdb prefix to related tests.

[Eelco Chaudron]

On 24 Sep 2024, at 9:02, Roi Dayan via dev wrote:

> Add ovsdb-idl prefix for tests in ovsdb-idl.at.
> Add ovsdb-log prefix for tests in ovsdb-log.at.
> Add ovsdb prefix for the rest of the ovsdb tests.
>
> Signed-off-by: Roi Dayan 

Thanks Roi for cleaning this up. The 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 1/2] tests: ovsdb: Update missing ovsdb keywords.

[Eelco Chaudron]

On 24 Sep 2024, at 9:02, Roi Dayan via dev wrote:

> Some ovsdb tests were missing keywords that exists in all
> other ovsdb tests.

Thanks Roi for cleaning this up. The 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] python: Index references.

[Eelco Chaudron]

On 23 Sep 2024, at 22:45, Luca Czesla via dev wrote:

> For an index on a reference to work, we should not resolve the reference to
> to the corresponding row object but instead use the uuid for indexing.
>
> an index on Datapath in the Port_Binding Table reduces the lookup in the
> neutron metadata agent for each request from approx. 400ms per request to
> 980μs with approx. 40k port bindings.
>
> Signed-off-by: Luca Czesla 

Recheck-request: github-robot

> ---
>  python/ovs/db/custom_index.py | 12 +---
>  python/ovs/db/idl.py  |  9 ++---
>  2 files changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/python/ovs/db/custom_index.py b/python/ovs/db/custom_index.py
> index 3fa03d3c9..2e10f9505 100644
> --- a/python/ovs/db/custom_index.py
> +++ b/python/ovs/db/custom_index.py
> @@ -11,7 +11,7 @@ try:
>  except ImportError:
>  from ovs.compat import sortedcontainers
>
> -from ovs.db import data
> +from ovs.db import data, idl
>
>  OVSDB_INDEX_ASC = "ASC"
>  OVSDB_INDEX_DESC = "DESC"
> @@ -43,7 +43,7 @@ class MultiColumnIndex(object):
>
>  def _cmp(self, a, b):
>  for col, direction, key in self.columns:
> -aval, bval = key(a), key(b)
> +aval, bval = idl._row_to_uuid(key(a)), idl._row_to_uuid(key(b))
>  if aval == bval:
>  continue
>  result = (aval > bval) - (aval < bval)
> @@ -53,7 +53,7 @@ class MultiColumnIndex(object):
>  def index_entry_from_row(self, row):
>  return row._table.rows.IndexEntry(
>  uuid=row.uuid,
> -**{c.column: getattr(row, c.column) for c in self.columns})
> +**{c.column: row.get_column(c.column, _uuid_to_uuid) for c in 
> self.columns})
>
>  def add(self, row):
>  if not all(hasattr(row, col.column) for col in self.columns):
> @@ -131,6 +131,12 @@ class IndexedRows(DictBase, object):
>  raise NotImplementedError()
>
>
> +# For indexing we do not want to resolve the references as we cannot
> +# ensure that the referenced table is already filled, the uuid is sufficient
> +# for the index.
> +def _uuid_to_uuid(atom, base):
> +return atom
> +
>  def IndexEntryClass(table):
>  """Create a class used represent Rows in indexes
>
> diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
> index c8cc54346..3feb068b8 100644
> --- a/python/ovs/db/idl.py
> +++ b/python/ovs/db/idl.py
> @@ -1351,7 +1351,7 @@ class Row(object):
>  else:
>  return atom
>
> -def __getattr__(self, column_name):
> +def get_column(self, column_name, uuid_to_row):
>  assert self._changes is not None
>  assert self._mutations is not None
>
> @@ -1392,7 +1392,7 @@ class Row(object):
>  datum = data.Datum.from_python(column.type, dlist,
> _row_to_uuid)
>  elif column.type.is_map():
> -dmap = datum.to_python(self._uuid_to_row)
> +dmap = datum.to_python(uuid_to_row)
>  if inserts is not None:
>  dmap.update(inserts)
>  if removes is not None:
> @@ -1409,7 +1409,10 @@ class Row(object):
>  else:
>  datum = inserts
>
> -return datum.to_python(self._uuid_to_row)
> +return datum.to_python(uuid_to_row)
> +
> +def __getattr__(self, column_name):
> +return self.get_column(column_name, self._uuid_to_row)
>
>  def __setattr__(self, column_name, value):
>  assert self._changes is not None
>
> base-commit: 1b99649025ae767974d0a7cea88d1e9f46f4378d
> -- 
> 2.39.5
>
> ___
> 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] checkpatch: Add new check-authors-file option to checkpatch.py.

[Eelco Chaudron]

On 23 Sep 2024, at 14:46, Eelco Chaudron wrote:

> This patch adds a new option, --check-authors-file, to the checkpatch
> tool to help OVS maintainers check for missing authors in the
> AUTHORS.rst file.
>
> Signed-off-by: Eelco Chaudron 

Recheck-request: github-robot

> ---
>  utilities/checkpatch.py | 19 ++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
> index 742a0bc47..fe18b1674 100755
> --- a/utilities/checkpatch.py
> +++ b/utilities/checkpatch.py
> @@ -28,6 +28,7 @@ __errors = 0
>  __warnings = 0
>  empty_return_check_state = 0
>  print_file_name = None
> +check_authors_file = False
>  checking_file = False
>  total_line = 0
>  colors = False
> @@ -977,6 +978,19 @@ def ovs_checkpatch_parse(text, filename, author=None, 
> committer=None):
>"who are not authors or co-authors or "
>"committers: %s"
>% ", ".join(extra_sigs))
> +
> +if check_authors_file or author:
> +m = re.search(r'<(.*?)>', author)
> +if m:
> +try:
> +with open("AUTHORS.rst", "r") as file:
> +if m.group(1) not in file.read():
> +print_error("Author '%s' is not in the "
> +"AUTHORS.rst file!" % author)
> +except FileNotFoundError:
> +print_error("Can't open AUTHORS.rst file in "
> +"current path!")
> +
>  elif is_committer.match(line):
>  committer = is_committer.match(line).group(2)
>  elif is_author.match(line):
> @@ -1067,6 +1081,7 @@ Input options:
>
>  Check options:
>  -h|--help  This help message
> +-a|--check-authors-fileCheck AUTHORS file for existense of author
>  -b|--skip-block-whitespace Skips the if/while/for whitespace tests
>  -l|--skip-leading-whitespace   Skips the leading whitespace test
>  -q|--quiet Only print error and warning information
> @@ -1138,7 +1153,7 @@ if __name__ == '__main__':
>sys.argv[1:])
>  n_patches = int(numeric_options[-1][1:]) if numeric_options else 0
>
> -optlist, args = getopt.getopt(args, 'bhlstfSq',
> +optlist, args = getopt.getopt(args, 'abhlstfSq',
>["check-file",
> "help",
> "skip-block-whitespace",
> @@ -1157,6 +1172,8 @@ if __name__ == '__main__':
>  if o in ("-h", "--help"):
>  usage()
>  sys.exit(0)
> +elif o in ("-a", "--check-authors-file"):
> +check_authors_file = True
>  elif o in ("-b", "--skip-block-whitespace"):
>  skip_block_whitespace_check = True
>  elif o in ("-l", "--skip-leading-whitespace"):
> -- 
> 2.46.0
>
> ___
> 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 v4] netdev-offload: Fix incorrect comments.

[Eelco Chaudron]

On 23 Sep 2024, at 14:59, Sunyang Wu via dev wrote:

> This patch fixes incorrect comments.
>
> Signed-off-by: Sunyang Wu 

Thanks for making all these changes, it looks good to me now.

Acked-by: Eelco Chaudron 

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

Re: [ovs-dev] [PATCH v3] netdev-offload: Fix incorrect comments.

[Eelco Chaudron]

On 23 Sep 2024, at 9:43, Sunyang Wu via dev wrote:

> This patch fixes incorrect comments.
>
> Signed-off-by: Sunyang Wu 

Thanks for the v3, however, there are two problems.

- You are using tabs, and you should use spaces.
- The lines are too long (if you replace the tabs with spaces).

Please run ./utilities/checkpatch.py before submitting the next rev of the 
patch.

Cheers,

Eelco

> ---
>  lib/netdev-offload.h | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/lib/netdev-offload.h b/lib/netdev-offload.h
> index 47f8e6f48..3c0130c7f 100644
> --- a/lib/netdev-offload.h
> +++ b/lib/netdev-offload.h
> @@ -46,11 +46,11 @@ struct ovs_action_push_tnl;
>
>  /* Offload-capable (HW) netdev information */
>  struct netdev_hw_info {
> -bool oor;/* Out of Offload Resources ? */
> -atomic_bool miss_api_supported;  /* hw_miss_packet_recover() supported.*/
> -int offload_count;  /* Pending (non-offloaded) flow count */
> -int pending_count;  /* Offloaded flow count */
> -OVSRCU_TYPE(void *) offload_data; /* Offload metadata. */
> +bool oor;/* Out of Offload Resources ? */
> +atomic_bool miss_api_supported;  /* hw_miss_packet_recover() supported.*/
> +int offload_count;   /* Offloaded flow count */
> +int pending_count;   /* Pending (non-offloaded) flow 
> count */
> +OVSRCU_TYPE(void *) offload_data;/* Offload metadata. */
>  };
>
>  enum hw_info_type {
> -- 
> 2.19.0.rc0.windows.1
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[ovs-dev] [PATCH] checkpatch: Add new check-authors-file option to checkpatch.py.

[Eelco Chaudron]

This patch adds a new option, --check-authors-file, to the checkpatch
tool to help OVS maintainers check for missing authors in the
AUTHORS.rst file.

Signed-off-by: Eelco Chaudron 
---
 utilities/checkpatch.py | 19 ++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index 742a0bc47..fe18b1674 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -28,6 +28,7 @@ __errors = 0
 __warnings = 0
 empty_return_check_state = 0
 print_file_name = None
+check_authors_file = False
 checking_file = False
 total_line = 0
 colors = False
@@ -977,6 +978,19 @@ def ovs_checkpatch_parse(text, filename, author=None, 
committer=None):
   "who are not authors or co-authors or "
   "committers: %s"
   % ", ".join(extra_sigs))
+
+if check_authors_file or author:
+m = re.search(r'<(.*?)>', author)
+if m:
+try:
+with open("AUTHORS.rst", "r") as file:
+if m.group(1) not in file.read():
+print_error("Author '%s' is not in the "
+"AUTHORS.rst file!" % author)
+except FileNotFoundError:
+print_error("Can't open AUTHORS.rst file in "
+"current path!")
+
 elif is_committer.match(line):
 committer = is_committer.match(line).group(2)
 elif is_author.match(line):
@@ -1067,6 +1081,7 @@ Input options:
 
 Check options:
 -h|--help  This help message
+-a|--check-authors-fileCheck AUTHORS file for existense of author
 -b|--skip-block-whitespace Skips the if/while/for whitespace tests
 -l|--skip-leading-whitespace   Skips the leading whitespace test
 -q|--quiet Only print error and warning information
@@ -1138,7 +1153,7 @@ if __name__ == '__main__':
   sys.argv[1:])
 n_patches = int(numeric_options[-1][1:]) if numeric_options else 0
 
-optlist, args = getopt.getopt(args, 'bhlstfSq',
+optlist, args = getopt.getopt(args, 'abhlstfSq',
   ["check-file",
"help",
"skip-block-whitespace",
@@ -1157,6 +1172,8 @@ if __name__ == '__main__':
 if o in ("-h", "--help"):
 usage()
 sys.exit(0)
+elif o in ("-a", "--check-authors-file"):
+check_authors_file = True
 elif o in ("-b", "--skip-block-whitespace"):
 skip_block_whitespace_check = True
 elif o in ("-l", "--skip-leading-whitespace"):
-- 
2.46.0

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

Re: [ovs-dev] [PATCH v1] netdev-dpdk: Disable outer udp checksum offload for txgbe driver.

[Eelco Chaudron]

On 23 Sep 2024, at 8:18, Eelco Chaudron wrote:

> On 20 Sep 2024, at 19:12, Ilya Maximets wrote:
>
>> On 9/16/24 12:08, Eelco Chaudron wrote:
>>>
>>>
>>> On 16 Sep 2024, at 12:03, Eelco Chaudron wrote:
>>>
>>>> On 14 Sep 2024, at 4:52, Jun Wang wrote:
>>>>
>>>>> Fixing the issue of incorrect outer UDP checksum in packets sent by
>>>>> the wangxun network card (driver is txgbe), we disabled
>>>>> RTE_ETH_TX_OFFLOAD_OUTER_UDP_CKSUM.
>>>>>
>>>>> Fixes: 084c8087292c ("userspace: Support VXLAN and GENEVE TSO.")
>>>>> Reported-by: Jun Wang 
>>>>>
>>>>> Signed-off-by: Jun Wang 
>>>>
>>>> Thanks for the patch Jun,
>>>>
>>>> Like David mentioned, you do not need the extra new line, and if you are 
>>>> the fixer and finder of the bug, the Signed-off-by is enough.
>>>>
>>>> I also do not have the specific hardware to test, so will leave the patch 
>>>> in the queue till the end of the week, maybe someone who has the hardware 
>>>> can double check…
>>>
>>> Forgot to add my ACK :)
>>>
>>> Acked-by: Eelco Chaudron 
>>>
>>
>> Hi, Eelco.  I see you applied the patch, but you didn't reply to the list.
>>
>> Also, any reason why you didn't backport it?  It sounds like it should
>> go at least down to 3.3.
>
> I was planning on doing this after Kevin committed his patches on Friday, but 
> it slipped my mind.
>
> Will do it today and report back once it’s done…

Patch has been applied to main, and backported to 3.3 and 3.4.

Cheers,

Eelco

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

Re: [ovs-dev] [PATCH] netdev-offload: Fix incorrect comments.

[Eelco Chaudron]

On 21 Sep 2024, at 8:39, Sunyang Wu via dev wrote:

> This patch fixes incorrect comments.
>
> Signed-off-by: Sunyang Wu 

Hi Sunyang,

Thanks for fixing this mistake. While you’re at it, maybe also align/fix the 
rest of the structure?
How about:

 /* Offload-capable (HW) netdev information */
 struct netdev_hw_info {
-bool oor;  /* Out of Offload Resources ? */
-atomic_bool miss_api_supported;  /* hw_miss_packet_recover() supported.*/
-int offload_count;  /* Pending (non-offloaded) flow count */
-int pending_count;  /* Offloaded flow count */
+bool oor; /* Out of Offload Resources? */
+atomic_bool miss_api_supported;   /* hw_miss_packet_recover() supported. */
+int offload_count;/* Offloaded flow count. */
+int pending_count;/* Pending (non-offloaded) flow count. */
 OVSRCU_TYPE(void *) offload_data; /* Offload metadata. */
 };

> ---
>  lib/netdev-offload.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/lib/netdev-offload.h b/lib/netdev-offload.h
> index 47f8e6f48..63d3ec5d3 100644
> --- a/lib/netdev-offload.h
> +++ b/lib/netdev-offload.h
> @@ -48,8 +48,8 @@ struct ovs_action_push_tnl;
>  struct netdev_hw_info {
>  bool oor;/* Out of Offload Resources ? */
>  atomic_bool miss_api_supported;  /* hw_miss_packet_recover() supported.*/
> -int offload_count;  /* Pending (non-offloaded) flow count */
> -int pending_count;  /* Offloaded flow count */
> +int offload_count;  /* Offloaded flow count */
> +int pending_count;  /* Pending (non-offloaded) flow count */
>  OVSRCU_TYPE(void *) offload_data; /* Offload metadata. */
>  };
>
> -- 
> 2.19.0.rc0.windows.1
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

Re: [ovs-dev] [PATCH v1] netdev-dpdk: Disable outer udp checksum offload for txgbe driver.

[Eelco Chaudron]

On 20 Sep 2024, at 19:12, Ilya Maximets wrote:

> On 9/16/24 12:08, Eelco Chaudron wrote:
>>
>>
>> On 16 Sep 2024, at 12:03, Eelco Chaudron wrote:
>>
>>> On 14 Sep 2024, at 4:52, Jun Wang wrote:
>>>
>>>> Fixing the issue of incorrect outer UDP checksum in packets sent by
>>>> the wangxun network card (driver is txgbe), we disabled
>>>> RTE_ETH_TX_OFFLOAD_OUTER_UDP_CKSUM.
>>>>
>>>> Fixes: 084c8087292c ("userspace: Support VXLAN and GENEVE TSO.")
>>>> Reported-by: Jun Wang 
>>>>
>>>> Signed-off-by: Jun Wang 
>>>
>>> Thanks for the patch Jun,
>>>
>>> Like David mentioned, you do not need the extra new line, and if you are 
>>> the fixer and finder of the bug, the Signed-off-by is enough.
>>>
>>> I also do not have the specific hardware to test, so will leave the patch 
>>> in the queue till the end of the week, maybe someone who has the hardware 
>>> can double check…
>>
>> Forgot to add my ACK :)
>>
>> Acked-by: Eelco Chaudron 
>>
>
> Hi, Eelco.  I see you applied the patch, but you didn't reply to the list.
>
> Also, any reason why you didn't backport it?  It sounds like it should
> go at least down to 3.3.

I was planning on doing this after Kevin committed his patches on Friday, but 
it slipped my mind.

Will do it today and report back once it’s done…

//Eelco

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

Re: [ovs-dev] [RFC PATCH 1/3] exact-match-offload: Introducing Exact-Match HW Offload Support for OVS

[Eelco Chaudron]

On 12 Sep 2024, at 14:51, Farhan Tariq wrote:

> This patch introduces exact-match offload support in OVS to enhance
> optimization for hardware datapath. It's being submitted as a RFC for early
> feedback.
>
> Problem Statement:
> Currently, OVS offloads megaflows using tc-flower. However, hardware
> support for megaflows has certain limitations:
> a. TCAMs are used, which aren't scalable and support fewer rules.
> b. Using hash tables to emulate TCAMs results in complex and slow
> performance with many tables.
>
> Solution:
> This patch aims to add exact-match offload support to OVS, addressing
> these issues. This feature is an optional enhancement and is disabled by
> default.
>
> Implementation Details:
> 1. Added a new flag, "exact-match-offload" to "ovs-vsctl...other_config",
>which is disabled by default but can be enabled at runtime. It works only
>if megaflows are disabled.
> 2. Introduced the "enable_exact_match_offload()" API in
>"/lib/netdev-offload-tc.c" to zero-out unused masks.
> 3. Moved "enable_megaflows" from "ofproto/ofproto-dpif-upcall.c" to "lib/-
>netdev-offload-tc.c" to make it visible in the "netdev-offload-tc.c"
>file. An alternate implementation could be to use a getter api.
>
> Testing results:
> Verified the basic functionality of the patch with different flows.
>
> Future Work:
> 1. Add support for connection tracking.
> 2. Add support for OVS-DPDK rte_flow.
>
> Note: This feature does not affect user OpenFlow tables.
> ---
> Potential Issue:
> This patch might not handle short-lived large numbers of new connections
> effectively. The last proposal (i.e. proposal #4 listed below) can address
> this issue. For now this feature is beneficial for many use cases, with
> further optimization for handling large numbers of short-lived connections
> planned for future work.
>
> Other Possible Solutions:
> Several approaches to implementing exact-match offload are being
> considered. Comments are welcome on the best approach:
> 1. Offload all fields except mutable and unpredictable ones (e.g., ttl,
>tos, ip_frag, checksum). Megaflows are disabled.
> 2. Offload only essential fields (e.g., 5-tuple + VLAN IDs + tunnel info
>etc). Megaflows are disabled.
> 3. Provide configuration options for users to select which fields to
>offload. Megaflows are disabled.
> 4. Introduce an offloading path from the OVS datapath (kernel) module to
>integrate megaflows and exact-match offload, addressing short-lived new
>connection performance. Suggestions on this approach are welcome.
> --

Thanks, Farhat, for submitting the RFC series. I noticed something that might 
be a mistake in the subject line—it says "patch 1/3,” but only patch 1 was sent.

Regarding your patch, I see that it relies on disabling mega-flow support. 
However, this isn’t something typically done in a production environment. The 
‘ovs-appctl upcall/disable-megaflows’ command was originally introduced for 
debugging purposes when megaflows were first implemented. Since no one runs or 
tests with this disabled, it’s generally considered a debug-only command.

While I understand the value of your patch in simplifying hardware offloading 
of datapath flows, disabling megaflows doesn’t seem like a practical solution 
for production use. Perhaps it would be beneficial to collaborate with other 
hardware vendors within the OVS community to find a more viable approach. It 
might be worth reaching out to other vendors like, Broadcom, Marvell, NVIDIA, 
etc. on this mailing list.

Cheers,

Eelco

> Co-authored-by: Farhat Ullah 
> mailto:far...@dreambigsemi.com>>
> Signed-off-by: Farhat Ullah 
> mailto:far...@dreambigsemi.com>>
> Signed-off-by: Farhan Tariq 
> mailto:fta...@dreambigsemi.com>>
>
> ---
>  Documentation/howto/tc-offload.rst |  56 +++
>  NEWS   |   6 +
>  lib/netdev-offload-tc.c| 236 +++--
>  lib/netdev-offload.c   |   5 +
>  lib/netdev-offload.h   |   2 +
>  ofproto/ofproto-dpif-upcall.c  |   2 +-
>  tests/system-offloads-traffic.at   |  24 
> +++
>  vswitchd/vswitch.xml   |  19 +++
>  8 files changed, 266 insertions(+), 84 deletions(-)
>
> diff --git a/Documentation/howto/tc-offload.rst 
> b/Documentation/howto/tc-offload.rst
> index ee7f73f8a..7e6e0e9d1 100644
> --- a/Documentation/howto/tc-offload.rst
> +++ b/Documentation/howto/tc-offload.rst
> @@ -37,6 +37,50 @@ The flow hardware offload is disabled by default and can 
> be enabled by::
>  TC flower has one additional configuration option caled ``tc-policy``.
>  For more details see ``man ovs-vswitchd.conf.db``.
>
> +
> +Exact Match Flow Hardware Offload
> +-
> +
> +The exact match flow hardware offload is disabled by default and can be
> +enabled by::
> +
> +$ ovs-appctl upcall/disable-megaflows
> +$ ovs-vsctl set Open_vSwitch . other_config:hw-offload=tru

Re: [ovs-dev] [PATCH] github: Skip FTP SNAT orig tuple tests due to broken Ubuntu kernel.

[Eelco Chaudron]

On 20 Sep 2024, at 11:48, Eelco Chaudron wrote:

> On 20 Sep 2024, at 0:57, Ilya Maximets wrote:
>
>> GitHub Actions moved to the new 6.8.0-1014-azure #16~22.04.1-Ubuntu
>> kernel that contains a known bug, but doesn't have a fix (presumably
>> because the fix is not in 6.8 branch, since 6.8 kernel version is not
>> supported upstream).
>>
>> The fix is in commit a23ac973f67f ("openvswitch: get related ct labels
>> from its master if it is not confirmed").  Turn off these tests in CI
>> until the kernel is fixed.
>>
>> This skips the tests for the userspace datapath as well, but hopefully
>> we'll be able to revert this change soon, so I didn't want to create
>> extra infrastructure for this workaround.
>
> Thanks, it looks good to me.
>
> Acked-by: Eelco Chaudron 

Thanks Ilya for the patch. I applied it to main, and the 3.3 and 3.4 branch.

//Eelco

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

Re: [ovs-dev] [PATCH branch-3.3 v1] ofproto-dpif-mirror: Always revalidate on mirror update.

[Eelco Chaudron]

On 20 Sep 2024, at 8:58, Eelco Chaudron wrote:

> On 17 Sep 2024, at 21:35, Mike Pattrick wrote:
>
>> Previously updating mirror settings would not trigger a revalidation,
>> this could result in impactful changes to mirrors taking a long time to
>> take effect.
>>
>> This change sets need_revalidate whenever a setting is successfully set
>> on a mirror.
>>
>> Fixes: ec7ceaed4f3e ("ofproto-dpif: Modularize mirror code.")
>> Reported-at: https://issues.redhat.com/browse/FDP-788
>> Tested-by: Kevin Traynor 
>> Acked-by: Kevin Traynor 
>> Signed-off-by: Mike Pattrick 
>
> Change looks good to me. Should we backport this all the way down to 2.17?
>
> Cheers,
>
> Eelco

Forgot to add my ACK :(

Acked-by: Eelco Chaudron 

>
>> ---
>>  ofproto/ofproto-dpif-mirror.c |  2 +-
>>  ofproto/ofproto-dpif.c| 10 ++
>>  2 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/ofproto/ofproto-dpif-mirror.c b/ofproto/ofproto-dpif-mirror.c
>> index 343b75f0e..45024580a 100644
>> --- a/ofproto/ofproto-dpif-mirror.c
>> +++ b/ofproto/ofproto-dpif-mirror.c
>> @@ -265,7 +265,7 @@ mirror_set(struct mbridge *mbridge, void *aux, const 
>> char *name,
>>  {
>>  hmapx_destroy(&srcs_map);
>>  hmapx_destroy(&dsts_map);
>> -return 0;
>> +return ECANCELED;
>>  }
>>
>>  /* XXX: Not sure if these need to be thread safe. */
>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>> index fe034f971..7e300c3f9 100644
>> --- a/ofproto/ofproto-dpif.c
>> +++ b/ofproto/ofproto-dpif.c
>> @@ -3669,6 +3669,16 @@ mirror_set__(struct ofproto *ofproto_, void *aux,
>> s->n_dsts, s->src_vlans,
>> bundle_lookup(ofproto, s->out_bundle),
>> s->snaplen, s->out_vlan);
>> +
>> +if (!error) {
>> +ofproto->backer->need_revalidate = REV_RECONFIGURE;
>> +} else if (error == ECANCELED) {
>> +/* The user requested a change that is identical to the current 
>> state,
>> + * the reconfiguration is canceled, but don't log an error message
>> + * about that. */
>> +error = 0;
>> +}
>> +
>>  free(srcs);
>>  free(dsts);
>>  return error;
>> -- 
>> 2.43.5
>>
>> ___
>> 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] ci: Run oss-fuzz build stage during CI.

[Eelco Chaudron]

On 20 Sep 2024, at 11:51, Ilya Maximets wrote:

> On 9/16/24 12:32, Eelco Chaudron wrote:
>> The oss-fuzz project builds specific OVS fuzzing code located in the
>> tests/oss-fuzz/ directory of our repository. However, this code is
>> not currently built as part of our CI pipeline, creating a potential
>> risk that changes in the main OVS code could break the oss-fuzz
>> integration. This commit addresses that by ensuring the fuzzing code
>> is built during CI, preventing potential issues. The additional build
>> step takes approximately 4 minutes.
>>
>> Signed-off-by: Eelco Chaudron 
>> ---
>>  .github/workflows/build-and-test.yml | 26 ++
>>  1 file changed, 26 insertions(+)
>>
>> diff --git a/.github/workflows/build-and-test.yml 
>> b/.github/workflows/build-and-test.yml
>> index 9d3a13ca1..5dd0cc2ad 100644
>> --- a/.github/workflows/build-and-test.yml
>> +++ b/.github/workflows/build-and-test.yml
>> @@ -388,6 +388,32 @@ jobs:
>>  - name: build
>>run:  ./.ci/linux-build.sh
>>
>> +  build-oss-fuzz:
>> +name: Build oss-fuzz fuzzers
>
> I'd change the 'Build' to lowercase here or remove it at all to be in line
> with other jobs.  But otherwise seems fine to me:
>
> Acked-by: Ilya Maximets 

Thanks Ilya (and Simon), applied the change on commit!

//Eelco


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

Re: [ovs-dev] [PATCH] github: Skip FTP SNAT orig tuple tests due to broken Ubuntu kernel.

[Eelco Chaudron]

On 20 Sep 2024, at 0:57, Ilya Maximets wrote:

> GitHub Actions moved to the new 6.8.0-1014-azure #16~22.04.1-Ubuntu
> kernel that contains a known bug, but doesn't have a fix (presumably
> because the fix is not in 6.8 branch, since 6.8 kernel version is not
> supported upstream).
>
> The fix is in commit a23ac973f67f ("openvswitch: get related ct labels
> from its master if it is not confirmed").  Turn off these tests in CI
> until the kernel is fixed.
>
> This skips the tests for the userspace datapath as well, but hopefully
> we'll be able to revert this change soon, so I didn't want to create
> extra infrastructure for this workaround.

Thanks, it 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 branch-3.3 v1] ofproto-dpif-mirror: Always revalidate on mirror update.

[Eelco Chaudron]

On 17 Sep 2024, at 21:35, Mike Pattrick wrote:

> Previously updating mirror settings would not trigger a revalidation,
> this could result in impactful changes to mirrors taking a long time to
> take effect.
>
> This change sets need_revalidate whenever a setting is successfully set
> on a mirror.
>
> Fixes: ec7ceaed4f3e ("ofproto-dpif: Modularize mirror code.")
> Reported-at: https://issues.redhat.com/browse/FDP-788
> Tested-by: Kevin Traynor 
> Acked-by: Kevin Traynor 
> Signed-off-by: Mike Pattrick 

Change looks good to me. Should we backport this all the way down to 2.17?

Cheers,

Eelco

> ---
>  ofproto/ofproto-dpif-mirror.c |  2 +-
>  ofproto/ofproto-dpif.c| 10 ++
>  2 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/ofproto/ofproto-dpif-mirror.c b/ofproto/ofproto-dpif-mirror.c
> index 343b75f0e..45024580a 100644
> --- a/ofproto/ofproto-dpif-mirror.c
> +++ b/ofproto/ofproto-dpif-mirror.c
> @@ -265,7 +265,7 @@ mirror_set(struct mbridge *mbridge, void *aux, const char 
> *name,
>  {
>  hmapx_destroy(&srcs_map);
>  hmapx_destroy(&dsts_map);
> -return 0;
> +return ECANCELED;
>  }
>
>  /* XXX: Not sure if these need to be thread safe. */
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index fe034f971..7e300c3f9 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -3669,6 +3669,16 @@ mirror_set__(struct ofproto *ofproto_, void *aux,
> s->n_dsts, s->src_vlans,
> bundle_lookup(ofproto, s->out_bundle),
> s->snaplen, s->out_vlan);
> +
> +if (!error) {
> +ofproto->backer->need_revalidate = REV_RECONFIGURE;
> +} else if (error == ECANCELED) {
> +/* The user requested a change that is identical to the current 
> state,
> + * the reconfiguration is canceled, but don't log an error message
> + * about that. */
> +error = 0;
> +}
> +
>  free(srcs);
>  free(dsts);
>  return error;
> -- 
> 2.43.5
>
> ___
> 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 v5 3/3] ovsdb: Optimize monitor update by directly serializing data into ds.

[Eelco Chaudron]

On 16 Sep 2024, at 18:33, Grigorii Nazarov wrote:

> `Hi Eelco,
>
> See my comments below.
>
> I'm new to this process, so I guess I'm to address all the issues together 
> when we agree on currently mentioned?

You are right Grigorii. After we wrap up the discussion on all patches in the 
series you can send out a new revision, which will go through another round of 
reviews.

If something is not clear, just let me know.

See my comments below…


> On 9/6/24 5:14 PM, Eelco Chaudron wrote:
>>
>>
>> On 1 Jul 2024, at 13:02, Grigorii Nazarov wrote:
>>
>> Hi Grigorii,
>>
>> This is not an in-depth review, but find some comments below.
>>
>> Cheers,
>>
>> Eelco
>>
>>> Currently serialization is performed by first converting
>>> the internal data representation into JSON objects, followed by
>>> serializing these objects by jsonrpc. This process results in lots of
>>> allocations for these intermediate objects. Consequently, this
>>> not only increases peak memory consumption, but also
>>> demands significantly more CPU work.
>>>
>>> By forming row-update JSONs directly in `ds`, which is then used
>>> to create 'serialized object' JSONs, the overall speed increased
>>> by a factor of 2.3.
>>>
>>> A local benchmark was run on a proprietary Southbound backup.
>>> Both versions, before and after applying the patch, were measured.
>>> For each version, there were two runs with 10 parallel clients,
>>> and two runs with 30 parallel clients. CPU time was recorded
>>> after startup (before clients started running) and after
>>> all clients received all updates. Clients were essentially running
>>> `ovsdb-client monitor-cond unix:pipe OVN_Southbound
>>> '[[\"actions\",\"!=\",\"sdfdfsd\"]]' Logical_Flow`.
>>> Similar results were obtained with other requests
>>> that required a significant amount of data transfer.
>>> The backup size is about 600 MB. Results are measured in seconds.
>>>
>>>  Before  After
>>> Baseline x10:   9.53108.54
>>> Baseline x10:   9.62108.67
>>> Baseline x30:   9.69307.04
>>> Baseline x30:   9.65303.32
>>> Patch x10:  9.6752.57
>>> Patch x10:  9.5753.12
>>> Patch x30:  9.53136.33
>>> Patch x30:  9.63135.88
>>
>> Are before and after times reversed? I assume a shorter time is better?
>> Also, how does this relate to the 2.3x speed improvement?
>>
>
> Honestly, it took me some time too, so probably I should reformat it somehow. 
> Meanwhile, 'before' and 'after' meaning is before and after payload, so the 
> time we improve is (After - Before).
>
> So, we have something close to
> Baseline x10 = 99
> Baseline x30 = 294
> Patch x10 = 43
> Patch x30 = 137
>
> Which is about 2.1-2.3 times greater for Baseline vs Patch.
> I had more thorough testing locally, including wall clock, but I think it's 
> lost by now.

Now I get it :) And yes you might want to reformat it.

>>> Signed-off-by: Grigorii Nazarov 
>>> ---
>>> v2: updated title
>>> v3: fixed bracing
>>> v4: changed patch number from 4/4 to 3/3
>>>
>>>   include/openvswitch/json.h |   1 +
>>>   lib/json.c |   8 ++
>>>   lib/ovsdb-data.c   | 105 +++
>>>   lib/ovsdb-data.h   |   3 +
>>>   ovsdb/monitor.c| 210 -
>>>   5 files changed, 254 insertions(+), 73 deletions(-)
>>>
>>> diff --git a/include/openvswitch/json.h b/include/openvswitch/json.h
>>> index 555440760..80b9479c7 100644
>>> --- a/include/openvswitch/json.h
>>> +++ b/include/openvswitch/json.h
>>> @@ -81,6 +81,7 @@ struct json *json_boolean_create(bool);
>>>   struct json *json_string_create(const char *);
>>>   struct json *json_string_create_nocopy(char *);
>>>   struct json *json_serialized_object_create(const struct json *);
>>> +struct json *json_serialized_object_create_from_string(const char *);
>>>   struct json *json_integer_create(long long int);
>>>   struct json *json_real_create(double);
>>>
>>> diff --git a/lib/json.c b/lib/json.c
>>> index d40e93857..66b1f571f 100644
>>> --- a/lib/json.c
>>> +++ b/lib/json.c
>>> @@ -199,6 +199,14 @@ json_serialized_object_create(const struct json *src)
>>>   return js

Re: [ovs-dev] [PATCH v6] bond: Always revalidate unbalanced bonds when active member changes.

[Eelco Chaudron]

On 16 Sep 2024, at 16:14, Mike Pattrick wrote:

> On Mon, Sep 16, 2024 at 6:18 AM Eelco Chaudron  wrote:
>>
>>
>>
>> On 13 Sep 2024, at 16:20, Mike Pattrick wrote:
>>
>>> Currently a bond will not always revalidate when an active member
>>> changes. This can result in counter-intuitive behaviors like the fact
>>> that using ovs-appctl bond/set-active-member will cause the bond to
>>> revalidate but changing other_config:bond-primary will not trigger a
>>> revalidate in the bond.
>>>
>>> When revalidation is not set but the active member changes in an
>>> unbalanced bond, OVS may send traffic out of previously active member
>>> instead of the new active member.
>>>
>>> This change will always mark unbalanced bonds for revalidation if the
>>> active member changes.
>>>
>>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2214979
>>> Signed-off-by: Mike Pattrick 
>>
>> Hi Mike,
>>
>> I think you missed Ilya’s comment on v4. Let me quote him here:
>>
>> “Slightly orthogonal question:  why this needs to be a system test
>> with pings and other stuff?  Simple 'unit' tests with dummy datapath
>> are more reliable, much faster and running more frequently in CI.”
>
> Hello,
>
> I tried to make a dummy test but it didn't reproduce the bug. When I
> set a dummy interface down, OVS revalidates immediately. I've also
> made changes to the system test to make it more reliable and faster to
> execute.

FYI you could pause the revalidator, but not sure if that solves the problem. 
I’ll wait for Ilya to respond, as it was his earlier question.

//Eelco

> Cheers,
> M
>
>>
>> Cheers,
>>
>> Eelco
>>
>>> ---
>>> v2: Added a test
>>> v3: Made the test more reliable
>>> v4: Made test much more reliable
>>> v5: Improved test performance
>>> v6: Improved system test by removing waits on ping.
>>> ---
>>>  ofproto/bond.c  |  8 +--
>>>  tests/system-traffic.at | 51 +
>>>  2 files changed, 57 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/ofproto/bond.c b/ofproto/bond.c
>>> index 0858de374..b9e2282f0 100644
>>> --- a/ofproto/bond.c
>>> +++ b/ofproto/bond.c
>>> @@ -193,6 +193,7 @@ static void bond_update_post_recirc_rules__(struct bond 
>>> *, bool force)
>>>  static bool bond_is_falling_back_to_ab(const struct bond *);
>>>  static void bond_add_lb_output_buckets(const struct bond *);
>>>  static void bond_del_lb_output_buckets(const struct bond *);
>>> +static bool bond_is_balanced(const struct bond *bond) 
>>> OVS_REQ_RDLOCK(rwlock);
>>>
>>>
>>>  /* Attempts to parse 's' as the name of a bond balancing mode.  If 
>>> successful,
>>> @@ -549,11 +550,15 @@ bond_find_member_by_mac(const struct bond *bond, 
>>> const struct eth_addr mac)
>>>
>>>  static void
>>>  bond_active_member_changed(struct bond *bond)
>>> +OVS_REQ_WRLOCK(rwlock)
>>>  {
>>>  if (bond->active_member) {
>>>  struct eth_addr mac;
>>>  netdev_get_etheraddr(bond->active_member->netdev, &mac);
>>>  bond->active_member_mac = mac;
>>> +if (!bond_is_balanced(bond)) {
>>> +bond->bond_revalidate = true;
>>> +}
>>>  } else {
>>>  bond->active_member_mac = eth_addr_zero;
>>>  }
>>> @@ -1121,7 +1126,7 @@ bond_get_recirc_id_and_hash_basis(struct bond *bond, 
>>> uint32_t *recirc_id,
>>>  /* Rebalancing. */
>>>
>>>  static bool
>>> -bond_is_balanced(const struct bond *bond) OVS_REQ_RDLOCK(rwlock)
>>> +bond_is_balanced(const struct bond *bond)
>>>  {
>>>  return bond->rebalance_interval
>>>  && (bond->balance == BM_SLB || bond->balance == BM_TCP)
>>> @@ -1725,7 +1730,6 @@ bond_unixctl_set_active_member(struct unixctl_conn 
>>> *conn,
>>>  }
>>>
>>>  if (bond->active_member != member) {
>>> -bond->bond_revalidate = true;
>>>  bond->active_member = member;
>>>  VLOG_INFO("bond %s: active member is now %s",
>>>bond->name, member->name);
>>> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
>>> index 724b25fa9..2b7e5530b 100644
>>&

Re: [ovs-dev] [PATCH v5 1/3] ovsdb: Simplify UUID formatting code.

[Eelco Chaudron]

On 16 Sep 2024, at 14:12, Grigorii Nazarov wrote:

> Hi Eelco,
>
> Thank you for your reply.
>
> On 9/6/24 1:09 PM, Eelco Chaudron wrote:
>>
>>
>> On 1 Jul 2024, at 13:02, Grigorii Nazarov wrote:
>>
>>> Signed-off-by: Grigorii Nazarov 
>>
>> Hi Grigorii,
>>
>> Thanks for the patch, see some comments below.
>>
>>> ---
>>> v2: fixed title
>>> v4: changed patch number from 2/4 to 1/3
>>>
>>>   lib/ovsdb-data.c | 8 +---
>>>   lib/uuid.h   | 1 +
>>>   2 files changed, 2 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/lib/ovsdb-data.c b/lib/ovsdb-data.c
>>> index abb923ad8..defb048d7 100644
>>> --- a/lib/ovsdb-data.c
>>> +++ b/lib/ovsdb-data.c
>>> @@ -2582,14 +2582,8 @@ char *
>>>   ovsdb_data_row_name(const struct uuid *uuid)
>>>   {
>>>   char *name;
>>> -char *p;
>>>
>>> -name = xasprintf("row"UUID_FMT, UUID_ARGS(uuid));
>>> -for (p = name; *p != '\0'; p++) {
>>> -if (*p == '-') {
>>> -*p = '_';
>>> -}
>>> -}
>>> +name = xasprintf(UUID_ROW_FMT, UUID_ARGS(uuid));
>>>
>>>   return name;
>>>   }
>>> diff --git a/lib/uuid.h b/lib/uuid.h
>>> index fa49354f6..6a8069f68 100644
>>> --- a/lib/uuid.h
>>> +++ b/lib/uuid.h
>>> @@ -34,6 +34,7 @@ extern "C" {
>>>*/
>>>   #define UUID_LEN 36
>>>   #define UUID_FMT "%08x-%04x-%04x-%04x-%04x%08x"
>>> +#define UUID_ROW_FMT "row%08x_%04x_%04x_%04x_%04x%08x"
>>
>> The “row” addition is rather dbase specific. What about calling it:
>>
>>#define UUID_UNDERSCORE_FMT "%08x_%04x_%04x_%04x_%04x%08x"
>>
>> and add the “row” part in the xasprintf() directly, so:
>>
>>name = xasprintf(“row”UUID_UNDERSCORE_FMT, UUID_ARGS(uuid));
>>
>>
> Agreed. I'm using it in the next patch, So probably the right decision would 
> be to move UUID_ROW_FMT to lib/ovsdb-data.c file instead. It's not otherwise 
> needed even partially in lib/uuid.h.

If you use my approach above, you could keep UUID_UNDERSCORE_FMT in uuid.h

>>>   #define UUID_ARGS(UUID) \
>>>   ((unsigned int) ((UUID)->parts[0])),\
>>>   ((unsigned int) ((UUID)->parts[1] >> 16)),  \
>>> -- 
>>> 2.45.2
>>>
>>> ___
>>> dev mailing list
>>> d...@openvswitch.org
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>
> -- 
> Best,
> Grigorii

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

[ovs-dev] [PATCH] ci: Run oss-fuzz build stage during CI.

[Eelco Chaudron]

The oss-fuzz project builds specific OVS fuzzing code located in the
tests/oss-fuzz/ directory of our repository. However, this code is
not currently built as part of our CI pipeline, creating a potential
risk that changes in the main OVS code could break the oss-fuzz
integration. This commit addresses that by ensuring the fuzzing code
is built during CI, preventing potential issues. The additional build
step takes approximately 4 minutes.

Signed-off-by: Eelco Chaudron 
---
 .github/workflows/build-and-test.yml | 26 ++
 1 file changed, 26 insertions(+)

diff --git a/.github/workflows/build-and-test.yml 
b/.github/workflows/build-and-test.yml
index 9d3a13ca1..5dd0cc2ad 100644
--- a/.github/workflows/build-and-test.yml
+++ b/.github/workflows/build-and-test.yml
@@ -388,6 +388,32 @@ jobs:
 - name: build
   run:  ./.ci/linux-build.sh
 
+  build-oss-fuzz:
+name: Build oss-fuzz fuzzers
+runs-on: ubuntu-22.04
+timeout-minutes: 30
+
+steps:
+- name: Checkout OVS
+  uses: actions/checkout@v4
+
+- name: Checkout oss-fuzz
+  uses: actions/checkout@v4
+  with:
+repository: google/oss-fuzz
+path: oss-fuzz
+
+- name: Build oss-fuzz image
+  run: |
+cd oss-fuzz
+python infra/helper.py build_image openvswitch --no-pull
+
+- name: Build oss-fuzz fuzzers
+  run: |
+cd oss-fuzz
+python infra/helper.py build_fuzzers --sanitizer address \
+  --engine afl --architecture x86_64 openvswitch $GITHUB_WORKSPACE
+
   build-osx:
 env:
   CC:clang
-- 
2.46.0

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

Re: [ovs-dev] [PATCH v6] bond: Always revalidate unbalanced bonds when active member changes.

[Eelco Chaudron]

On 13 Sep 2024, at 16:20, Mike Pattrick wrote:

> Currently a bond will not always revalidate when an active member
> changes. This can result in counter-intuitive behaviors like the fact
> that using ovs-appctl bond/set-active-member will cause the bond to
> revalidate but changing other_config:bond-primary will not trigger a
> revalidate in the bond.
>
> When revalidation is not set but the active member changes in an
> unbalanced bond, OVS may send traffic out of previously active member
> instead of the new active member.
>
> This change will always mark unbalanced bonds for revalidation if the
> active member changes.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2214979
> Signed-off-by: Mike Pattrick 

Hi Mike,

I think you missed Ilya’s comment on v4. Let me quote him here:

“Slightly orthogonal question:  why this needs to be a system test
with pings and other stuff?  Simple 'unit' tests with dummy datapath
are more reliable, much faster and running more frequently in CI.”

Cheers,

Eelco

> ---
> v2: Added a test
> v3: Made the test more reliable
> v4: Made test much more reliable
> v5: Improved test performance
> v6: Improved system test by removing waits on ping.
> ---
>  ofproto/bond.c  |  8 +--
>  tests/system-traffic.at | 51 +
>  2 files changed, 57 insertions(+), 2 deletions(-)
>
> diff --git a/ofproto/bond.c b/ofproto/bond.c
> index 0858de374..b9e2282f0 100644
> --- a/ofproto/bond.c
> +++ b/ofproto/bond.c
> @@ -193,6 +193,7 @@ static void bond_update_post_recirc_rules__(struct bond 
> *, bool force)
>  static bool bond_is_falling_back_to_ab(const struct bond *);
>  static void bond_add_lb_output_buckets(const struct bond *);
>  static void bond_del_lb_output_buckets(const struct bond *);
> +static bool bond_is_balanced(const struct bond *bond) OVS_REQ_RDLOCK(rwlock);
>
>
>  /* Attempts to parse 's' as the name of a bond balancing mode.  If 
> successful,
> @@ -549,11 +550,15 @@ bond_find_member_by_mac(const struct bond *bond, const 
> struct eth_addr mac)
>
>  static void
>  bond_active_member_changed(struct bond *bond)
> +OVS_REQ_WRLOCK(rwlock)
>  {
>  if (bond->active_member) {
>  struct eth_addr mac;
>  netdev_get_etheraddr(bond->active_member->netdev, &mac);
>  bond->active_member_mac = mac;
> +if (!bond_is_balanced(bond)) {
> +bond->bond_revalidate = true;
> +}
>  } else {
>  bond->active_member_mac = eth_addr_zero;
>  }
> @@ -1121,7 +1126,7 @@ bond_get_recirc_id_and_hash_basis(struct bond *bond, 
> uint32_t *recirc_id,
>  /* Rebalancing. */
>
>  static bool
> -bond_is_balanced(const struct bond *bond) OVS_REQ_RDLOCK(rwlock)
> +bond_is_balanced(const struct bond *bond)
>  {
>  return bond->rebalance_interval
>  && (bond->balance == BM_SLB || bond->balance == BM_TCP)
> @@ -1725,7 +1730,6 @@ bond_unixctl_set_active_member(struct unixctl_conn 
> *conn,
>  }
>
>  if (bond->active_member != member) {
> -bond->bond_revalidate = true;
>  bond->active_member = member;
>  VLOG_INFO("bond %s: active member is now %s",
>bond->name, member->name);
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index 724b25fa9..2b7e5530b 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -291,6 +291,57 @@ NS_CHECK_EXEC([at_ns0], [ping -s 3200 -q -c 3 -i 0.3 -W 
> 2 10.1.1.2 | FORMAT_PING
>  OVS_TRAFFIC_VSWITCHD_STOP
>  AT_CLEANUP
>
> +AT_SETUP([datapath - bond active-backup failover])
> +OVS_TRAFFIC_VSWITCHD_START([_ADD_BR([br1])])
> +
> +AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"])
> +AT_CHECK([ovs-ofctl add-flow br1 "actions=normal"])
> +
> +ADD_NAMESPACES(at_ns0, at_ns1)
> +
> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
> +ADD_VETH(p1, at_ns1, br1, "10.1.1.2/24")
> +on_exit 'ip link del link0a'
> +on_exit 'ip link del link0b'
> +AT_CHECK([ip link add link0a type veth peer name link1a])
> +AT_CHECK([ip link add link0b type veth peer name link1b])
> +
> +AT_CHECK([ip link set dev link0a up])
> +AT_CHECK([ip link set dev link1a up])
> +AT_CHECK([ip link set dev link0b up])
> +AT_CHECK([ip link set dev link1b up])
> +
> +AT_CHECK([ovs-vsctl add-bond br0 bond0 link0a link0b 
> bond_mode=active-backup])
> +AT_CHECK([ovs-vsctl add-bond br1 bond1 link1a link1b 
> bond_mode=active-backup])
> +
> +dnl Set primary bond member.
> +AT_CHECK([ovs-vsctl set port bond0 other_config:bond-primary=link0a -- \
> +set port bond1 other_config:bond-primary=link1a])
> +
> +OVS_WAIT_UNTIL([test -n "`ovs-appctl bond/show | grep 'active-backup 
> primary: link0a'`"])
> +
> +OVS_WAIT_UNTIL([ip netns exec at_ns0 ping -c 1 10.1.1.2])
> +
> +dnl Keep traffic active in the background.
> +NETNS_DAEMONIZE([at_ns0], [ping -q 10.1.1.2], [ping.pid])
> +
> +dnl Check correct port is used.
> +AT_CHECK([ovs-appctl dpctl/dump-flows -m | grep -Eq "actions:link[[01]]a"], 
> [0])
>

Re: [ovs-dev] [PATCH v1] netdev-dpdk: Disable outer udp checksum offload for txgbe driver.

[Eelco Chaudron]

On 16 Sep 2024, at 12:03, Eelco Chaudron wrote:

> On 14 Sep 2024, at 4:52, Jun Wang wrote:
>
>> Fixing the issue of incorrect outer UDP checksum in packets sent by
>> the wangxun network card (driver is txgbe), we disabled
>> RTE_ETH_TX_OFFLOAD_OUTER_UDP_CKSUM.
>>
>> Fixes: 084c8087292c ("userspace: Support VXLAN and GENEVE TSO.")
>> Reported-by: Jun Wang 
>>
>> Signed-off-by: Jun Wang 
>
> Thanks for the patch Jun,
>
> Like David mentioned, you do not need the extra new line, and if you are the 
> fixer and finder of the bug, the Signed-off-by is enough.
>
> I also do not have the specific hardware to test, so will leave the patch in 
> the queue till the end of the week, maybe someone who has the hardware can 
> double check…

Forgot to add my ACK :)

Acked-by: Eelco Chaudron 

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

Re: [ovs-dev] [PATCH v1] netdev-dpdk: Disable outer udp checksum offload for txgbe driver.

[Eelco Chaudron]

On 14 Sep 2024, at 4:52, Jun Wang wrote:

> Fixing the issue of incorrect outer UDP checksum in packets sent by
> the wangxun network card (driver is txgbe), we disabled
> RTE_ETH_TX_OFFLOAD_OUTER_UDP_CKSUM.
>
> Fixes: 084c8087292c ("userspace: Support VXLAN and GENEVE TSO.")
> Reported-by: Jun Wang 
>
> Signed-off-by: Jun Wang 

Thanks for the patch Jun,

Like David mentioned, you do not need the extra new line, and if you are the 
fixer and finder of the bug, the Signed-off-by is enough.

I also do not have the specific hardware to test, so will leave the patch in 
the queue till the end of the week, maybe someone who has the hardware can 
double check…

Cheers,

Eelco

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

Re: [ovs-dev] [PATCH v4] utilities: Add a GDB macro to dump ct conns.

[Eelco Chaudron]

On 13 Sep 2024, at 5:14, LIU Yulong wrote:

> Add a new GDB macro called ovs_dump_conntrack_conns, which can
> be used to dump all conn structure in a conntrack. For example
>
> (gdb) ovs_dump_conntrack_conns
> usage: ovs_dump_conntrack_conns  {short}
>
> (gdb) ovs_dump_conntrack_conns 0x5606339c25e0
> (struct conn *) 0x7f32c000a8c0: expiration = 26162886419, mark = 0, dl_type = 
> 8, zone = 3, nw_proto = 6 '\006'
> (struct conn *) 0x7f32c00489d0: expiration = 26162900867, mark = 0, dl_type = 
> 8, zone = 3, nw_proto = 1 '\001'
> (struct conn *) 0x7f32c0153bb0: expiration = 26249266420, mark = 0, dl_type = 
> 8, zone = 3, nw_proto = 6 '\006'
>
> (gdb) ovs_dump_conntrack_conns 0x5606339c25e0 short
> (struct conn *) 0x7f32c000a8c0
> (struct conn *) 0x7f32c00489d0
> (struct conn *) 0x7f32c0153bb0
>
> Signed-off-by: LIU Yulong 

LIU, thanks for the patch, and Mike thanks for the Review.

This patch was applied to main.

Cheers,

Eelco

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

Re: [ovs-dev] [PATCH v2] netdev-offload-dpdk: Change flow offload failure log level.

[Eelco Chaudron]

On 13 Sep 2024, at 14:25, Kevin Traynor wrote:

> Previously when a flow was attempted to be offloaded, if it
> could not be offloaded and did not return an actions error,
> a warning was logged.
>
> The reason there was an exception for an actions error was to allow
> for failure for full offload of a flow because it will fallback to
> partial offload. There are some issues with this approach to logging.
>
> Some NICs do not specify an actions error, because other config in
> the NIC may be checked first. e.g. In the case of Mellanox CX-5,
> there can be different types of offload configured, so an unspecified
> error may be returned.
>
> More generally, enabling hw-offload is best effort per datapath/NIC/flow
> as full and partial offload support in NICs is variable and there is
> always fallback to software.
>
> So there is likely to be repeated logging about offloading of flows
> failing. With this in mind, change the log level to debug.
>
> The status of the offload can still be seen with below command:
> $ ovs-appctl dpctl/dump-flows -m
> ... offloaded:partial ...
>
> Also, remove some duplicated rate limiting and tidy-up the succeed
> and failure logs.
>
> Signed-off-by: Kevin Traynor 

Kevin, thanks for the patch, and David thanks for the Review.

This patch was applied to main.

Cheers,

Eelco

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

Re: [ovs-dev] [PATCH v2] netdev-offload-dpdk: Change flow offload failure log level.

[Eelco Chaudron]

On 16 Sep 2024, at 10:01, David Marchand wrote:

> On Fri, Sep 13, 2024 at 2:25 PM Kevin Traynor  wrote:
>>
>> Previously when a flow was attempted to be offloaded, if it
>> could not be offloaded and did not return an actions error,
>> a warning was logged.
>>
>> The reason there was an exception for an actions error was to allow
>> for failure for full offload of a flow because it will fallback to
>> partial offload. There are some issues with this approach to logging.
>>
>> Some NICs do not specify an actions error, because other config in
>> the NIC may be checked first. e.g. In the case of Mellanox CX-5,
>> there can be different types of offload configured, so an unspecified
>> error may be returned.
>>
>> More generally, enabling hw-offload is best effort per datapath/NIC/flow
>> as full and partial offload support in NICs is variable and there is
>> always fallback to software.
>>
>> So there is likely to be repeated logging about offloading of flows
>> failing. With this in mind, change the log level to debug.
>>
>> The status of the offload can still be seen with below command:
>> $ ovs-appctl dpctl/dump-flows -m
>> ... offloaded:partial ...
>>
>> Also, remove some duplicated rate limiting and tidy-up the succeed
>> and failure logs.
>>
>> Signed-off-by: Kevin Traynor 
>>
>> ---
>> v2: combined logs on failure and added confirmation of result on
>> succeed.
>> ---
>>  lib/netdev-offload-dpdk.c | 23 +--
>>  1 file changed, 9 insertions(+), 14 deletions(-)
>>
>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
>> index 623005b1c..342292d23 100644
>> --- a/lib/netdev-offload-dpdk.c
>> +++ b/lib/netdev-offload-dpdk.c
>> @@ -920,22 +920,17 @@ netdev_offload_dpdk_flow_create(struct netdev *netdev,
>>  dump_flow(&s, &s_extra, attr, flow_patterns, flow_actions);
>>  extra_str = ds_cstr(&s_extra);
>> -VLOG_DBG_RL(&rl, "%s: rte_flow 0x%"PRIxPTR" %s  flow create %d 
>> %s",
>> -netdev_get_name(netdev), (intptr_t) flow, extra_str,
>> -netdev_dpdk_get_port_id(netdev), ds_cstr(&s));
>> +VLOG_DBG("%s: rte_flow creation succeeded: rte_flow 
>> 0x%"PRIxPTR" "
>> + "%s  flow create %d %s", netdev_get_name(netdev),
>> + (intptr_t) flow, extra_str,
>> + netdev_dpdk_get_port_id(netdev), ds_cstr(&s));
>>  }
>>  } else {
>> -enum vlog_level level = VLL_WARN;
>> -
>> -if (error->type == RTE_FLOW_ERROR_TYPE_ACTION) {
>> -level = VLL_DBG;
>> -}
>> -VLOG_RL(&rl, level, "%s: rte_flow creation failed: %d (%s).",
>> -netdev_get_name(netdev), error->type, error->message);
>> -if (!vlog_should_drop(&this_module, level, &rl)) {
>> +if (!VLOG_DROP_DBG(&rl)) {
>>  dump_flow(&s, &s_extra, attr, flow_patterns, flow_actions);
>>  extra_str = ds_cstr(&s_extra);
>> -VLOG_RL(&rl, level, "%s: Failed flow: %s  flow create %d %s",
>> -netdev_get_name(netdev), extra_str,
>> -netdev_dpdk_get_port_id(netdev), ds_cstr(&s));
>> +VLOG_DBG("%s: rte_flow creation failed [%d (%s)]: "
>> + "%s  flow create %d %s", netdev_get_name(netdev),
>> + error->type, error->message,extra_str,
>
> Nit: I think an extra space is missing before extra_str.

Thanks for the review David! I’ll fix it at commit time.

>
>
>> + netdev_dpdk_get_port_id(netdev), ds_cstr(&s));
>>  }
>>  }
>> --
>> 2.46.0
>>
>
> Reviewed-by: David Marchand 
>
>
> -- 
> David Marchand

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

Re: [ovs-dev] [PATCH v1] netdev-dpdk: Added xstat statistics interface.

[Eelco Chaudron]

On 14 Sep 2024, at 7:26, Jun Wang wrote:

> For many network cards, xstat statistics cannot be queried in the
> ovs interface. A new interface is added to retrieve all xstat
> information of the network card.
Hi Jun,

Isn’t this already handled with netdev_dpdk_get_custom_stats()? I think you can 
see them with ‘ovs-vsctl get Interface  statistics.

If this is not the case this is what we should enhance.

//Eelco

> Reported-by: Jun Wang 
>
> Signed-off-by: Jun Wang 
> ---
>  lib/netdev-dpdk.c | 100 
> ++
>  1 file changed, 100 insertions(+)
>
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index a3878d2..59292e3 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -4692,6 +4692,102 @@ out:
>  netdev_close(netdev);
>  }
>
> +static void
> +netdev_dpdk_show_port_xstats(struct unixctl_conn *conn,
> +   uint16_t port_id, struct ds * ports_xstats)
> +{
> +char *response = NULL;
> +struct rte_eth_xstat_name *xstats_names;
> +uint64_t *values;
> +int len, ret, j;
> +static const char *nic_stats_border = "";
> +
> +if (!rte_eth_dev_is_valid_port(port_id)) {
> +return;
> +}
> +
> +len = rte_eth_xstats_get_names_by_id(port_id, NULL, 0, NULL);
> +if (len < 0) {
> +response = xasprintf("Cannot get xstats count\n");
> +goto err;
> +}
> +values = xmalloc(sizeof(*values) * len);
> +if (values == NULL) {
> +response = xasprintf("Cannot allocate memory for xstats\n");
> +goto err;
> +}
> +
> +xstats_names = xmalloc(sizeof(struct rte_eth_xstat_name) * len);
> +if (xstats_names == NULL) {
> +response = xasprintf("Cannot allocate memory for xstat"
> + " names\n");
> +free(values);
> +goto err;
> +}
> +if (len != rte_eth_xstats_get_names_by_id(port_id, xstats_names,
> +  len, NULL)) {
> +response = xasprintf("Cannot get xstat names\n");
> +free(values);
> +free(xstats_names);
> +goto err;
> +}
> +
> +ds_put_format(ports_xstats, "## NIC extended statistics for"
> +  " port %-2d #\n", port_id);
> +ds_put_format(ports_xstats, "%s\n",
> +  nic_stats_border);
> +ret = rte_eth_xstats_get_by_id(port_id, NULL, values, len);
> +if (ret < 0 || ret > len) {
> +response = xasprintf("Cannot get xstats\n");
> +free(values);
> +free(xstats_names);
> +goto err;
> +}
> +
> +for (j = 0; j < len; j++) {
> +ds_put_format(ports_xstats, "%s: %"PRIu64"\n",
> +  xstats_names[j].name, values[j]);
> +}
> +
> +ds_put_format(ports_xstats, "%s\n",
> +  nic_stats_border);
> +
> +free(values);
> +free(xstats_names);
> +return;
> +err:
> +unixctl_command_reply_error(conn, response);
> +free(response);
> +}
> +
> +static void
> +netdev_dpdk_show_xstats(struct unixctl_conn *conn, int argc,
> +   const char *argv[], void *aux OVS_UNUSED)
> +{
> +int i;
> +struct ds ports_xstats = DS_EMPTY_INITIALIZER;
> +struct netdev *netdev = NULL;
> +
> +if (argc == 2) {
> +netdev = netdev_from_name(argv[1]);
> +if (!netdev || !is_dpdk_class(netdev->netdev_class)) {
> +unixctl_command_reply_error(conn, "Not a DPDK Interface");
> +goto xstats;
> +}
> +struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> +netdev_dpdk_show_port_xstats(conn, dev->port_id,
> + &ports_xstats);
> +} else {
> +for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
> +netdev_dpdk_show_port_xstats(conn, i, &ports_xstats);
> +}
> +}
> +unixctl_command_reply(conn, ds_cstr(&ports_xstats));
> +xstats:
> +ds_destroy(&ports_xstats);
> +netdev_close(netdev);
> +}
> +
>  /*
>   * Set virtqueue flags so that we do not receive interrupts.
>   */
> @@ -5164,6 +5260,10 @@ netdev_dpdk_class_init(void)
>   "[netdev]", 0, 1,
>   netdev_dpdk_get_mempool_info, NULL);
>
> +unixctl_command_register("netdev-dpdk/show-xstats",
> + "[netdev]", 0, 1,
> + netdev_dpdk_show_xstats, NULL);
> +
>  netdev_dpdk_reset_seq = seq_create();
>  netdev_dpdk_last_reset_seq = seq_read(netdev_dpdk_reset_seq);
>  ret = rte_eth_dev_callback_register(RTE_ETH_ALL,
> -- 
> 1.8.3.1
>
>
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.

Re: [ovs-dev] [PATCH v2] netdev-offload-dpdk: Change flow offload failure log level.

[Eelco Chaudron]

On 13 Sep 2024, at 14:25, Kevin Traynor wrote:

> Previously when a flow was attempted to be offloaded, if it
> could not be offloaded and did not return an actions error,
> a warning was logged.
>
> The reason there was an exception for an actions error was to allow
> for failure for full offload of a flow because it will fallback to
> partial offload. There are some issues with this approach to logging.
>
> Some NICs do not specify an actions error, because other config in
> the NIC may be checked first. e.g. In the case of Mellanox CX-5,
> there can be different types of offload configured, so an unspecified
> error may be returned.
>
> More generally, enabling hw-offload is best effort per datapath/NIC/flow
> as full and partial offload support in NICs is variable and there is
> always fallback to software.
>
> So there is likely to be repeated logging about offloading of flows
> failing. With this in mind, change the log level to debug.
>
> The status of the offload can still be seen with below command:
> $ ovs-appctl dpctl/dump-flows -m
> ... offloaded:partial ...
>
> Also, remove some duplicated rate limiting and tidy-up the succeed
> and failure logs.
>
> Signed-off-by: Kevin Traynor 
>

Thanks, Kevin, for following up and for the consistent debug messages for both 
failures and successes.

Acked-by: Eelco Chaudron 

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

Re: [ovs-dev] [PATCH v4] utilities: Add a GDB macro to dump ct conns.

[Eelco Chaudron]

On 13 Sep 2024, at 5:14, LIU Yulong wrote:

> Add a new GDB macro called ovs_dump_conntrack_conns, which can
> be used to dump all conn structure in a conntrack. For example
>
> (gdb) ovs_dump_conntrack_conns
> usage: ovs_dump_conntrack_conns  {short}
>
> (gdb) ovs_dump_conntrack_conns 0x5606339c25e0
> (struct conn *) 0x7f32c000a8c0: expiration = 26162886419, mark = 0, dl_type = 
> 8, zone = 3, nw_proto = 6 '\006'
> (struct conn *) 0x7f32c00489d0: expiration = 26162900867, mark = 0, dl_type = 
> 8, zone = 3, nw_proto = 1 '\001'
> (struct conn *) 0x7f32c0153bb0: expiration = 26249266420, mark = 0, dl_type = 
> 8, zone = 3, nw_proto = 6 '\006'
>
> (gdb) ovs_dump_conntrack_conns 0x5606339c25e0 short
> (struct conn *) 0x7f32c000a8c0
> (struct conn *) 0x7f32c00489d0
> (struct conn *) 0x7f32c0153bb0
>
> Signed-off-by: LIU Yulong 

Thanks LIU for the v4, it looks good to me with one small nit below, which I 
can apply on commit.

Acked-by: Eelco Chaudron 

> ---
> v1: initial version
> v2: address comments from reviewers
> print details of struct conn
> add short param
> v3: address pep8 check warnnings
> v4: align the new data structure of struct conn {}
> ---
>  utilities/gdb/ovs_gdb.py | 64 
>  1 file changed, 64 insertions(+)
>
> diff --git a/utilities/gdb/ovs_gdb.py b/utilities/gdb/ovs_gdb.py
> index 982395dd1..53acc6c4b 100644
> --- a/utilities/gdb/ovs_gdb.py
> +++ b/utilities/gdb/ovs_gdb.py
> @@ -37,6 +37,7 @@
>  #- ovs_dump_udpif_keys {|} {short}
>  #- ovs_show_fdb {[] {dbg} {hash}}
>  #- ovs_show_upcall {dbg}
> +#- ovs_dump_conntrack_conns  {short}
>  #
>  #  Example:
>  #$ gdb $(which ovs-vswitchd) $(pidof ovs-vswitchd)
> @@ -1550,6 +1551,68 @@ class CmdDumpPackets(gdb.Command):
>  return packet
>
>
> +#
> +# Implements the GDB "ovs_dump_conntrack_conns" command
> +#
> +class CmdDumpDpConntrackConn(gdb.Command):
> +"""Dump all connections in a conntrack set
> +Usage:
> +  ovs_dump_conntrack_conns  {short}
> +
> +   : Pointer to conntrack
> +  short: Only dump conn structure addresses,
> + no content details
> +
> +Example dumping all  connections:
> +
> +(gdb) ovs_dump_conntrack_conns 0x5606339c25e0
> +(struct conn *) 0x7f32c000a8c0: expiration = ... nw_proto = 1
> +(struct conn *) 0x7f32c00489d0: expiration = ... nw_proto = 6
> +(struct conn *) 0x7f32c0153bb0: expiration = ... nw_proto = 17
> +
> +(gdb) ovs_dump_conntrack_conns 0x5606339c25e0 short
> +(struct conn *) 0x7f32c000a8c0
> +(struct conn *) 0x7f32c00489d0
> +(struct conn *) 0x7f32c0153bb0
> +"""
> +def __init__(self):
> +super(CmdDumpDpConntrackConn, self).__init__(
> +"ovs_dump_conntrack_conns",
> +gdb.COMMAND_DATA)
> +
> +@staticmethod
> +def display_single_conn(conn, dir_, indent=0, short=False):
> +indent = " " * indent
> +if short:
> +print("{}(struct conn *) {}".format(indent, conn))
> +else:
> +print("{}(struct conn *) {}: expiration = {}, mark = {}, "
> +  "dl_type = {}, zone = {}, nw_proto = {}".format(
> +  indent, conn, conn['expiration'],
> +  conn['mark'], conn['key_node'][dir_]['key']['dl_type'],
> +  conn['key_node'][dir_]['key']['zone'],
> +  conn['key_node'][dir_]['key']['nw_proto']))
> +
> +def invoke(self, arg, from_tty):
> +arg_list = gdb.string_to_argv(arg)
> +if len(arg_list) == 0:

We do not check if the 2nd parameter is garbage (or more exists). So I would 
add the following:

 def invoke(self, arg, from_tty):
 arg_list = gdb.string_to_argv(arg)
-if len(arg_list) == 0:
+if len(arg_list) not in (1, 2) or \
+   (len(arg_list) == 2 and arg_list[1] != "short"):
 print("usage: ovs_dump_conntrack_conns  "

> +print("usage: ovs_dump_conntrack_conns  "
> +  "{short}")
> +return
> +
> +ct = gdb.parse_and_eval(arg_list[0]).cast(
> +gdb.lookup_type('struct conntrack').pointer())
> +
> +for key_node in ForEachCMAP(ct["conns"],
> +"struct conn_key_node", "cm_node"):
> +node = c

Re: [ovs-dev] [PATCH] oss-fuzz: Fix odp_flow_format() API in the fuzz tests.

[Eelco Chaudron]

On 12 Sep 2024, at 16:19, Ilya Maximets wrote:

> On 9/12/24 15:45, Eelco Chaudron wrote:
>> When the commit below introduced an API change, the oss-fuzz component
>> was not updated accordingly, leading to failures in the upstream
>> oss-fuzz tests.
>>
>> For more information on the failure, see:
>>  https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=71524
>>
>> Fixes: 252ee0f18211 ("dpif: Fix flow put debug message match content.")
>> Signed-off-by: Eelco Chaudron 
>> ---
>>  tests/oss-fuzz/odp_target.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/oss-fuzz/odp_target.c b/tests/oss-fuzz/odp_target.c
>> index ae61cdca3..0d615f8a2 100644
>> --- a/tests/oss-fuzz/odp_target.c
>> +++ b/tests/oss-fuzz/odp_target.c
>> @@ -81,7 +81,8 @@ parse_keys(bool wc_keys, const char *in)
>>  ds_init(&out);
>>  if (wc_keys) {
>>  odp_flow_format(odp_key.data, odp_key.size,
>> -odp_mask.data, odp_mask.size, NULL, &out, false);
>> +odp_mask.data, odp_mask.size, NULL, &out, false,
>> +false);
>>  } else {
>>  odp_flow_key_format(odp_key.data, odp_key.size, &out);
>>  }
>
> I didn't test the actual build, but looks correct.

I verified it offline with their tools. Thanks for the quick review, and the 
patch is applied to main.

> Acked-by: Ilya Maximets 

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

Re: [ovs-dev] [PATCH] dpif: Fix flow put debug message match content.

[Eelco Chaudron]

On 12 Sep 2024, at 16:07, Ilya Maximets wrote:

> On 9/12/24 15:47, Eelco Chaudron wrote:
>>
>>
>> On 12 Sep 2024, at 14:58, Ilya Maximets wrote:
>>
>>> On 9/3/24 11:37, Eelco Chaudron wrote:
>>>> The odp_flow_format() function applies a wildcard mask when a
>>>> mask for a given key was not present. However, in the context of
>>>> installing flows in the datapath, the absence of a mask actually
>>>> indicates that the key should be ignored, meaning it should not
>>>> be masked at all.
>>>>
>>>> To address this inconsistency, odp_flow_format() now includes an
>>>> option to skip formatting keys that are missing a mask.
>>>>
>>>> Signed-off-by: Eelco Chaudron 
>>>> ---
>>>>  lib/dpctl.c|  4 ++--
>>>>  lib/dpif-netdev.c  |  6 +++---
>>>>  lib/dpif.c |  4 ++--
>>>>  lib/odp-util.c | 31 ++-
>>>>  lib/odp-util.h |  2 +-
>>>>  lib/tnl-ports.c|  2 +-
>>>>  ofproto/ofproto-dpif.c |  2 +-
>>>>  tests/test-odp.c   |  6 --
>>>>  8 files changed, 32 insertions(+), 25 deletions(-)
>>>
>>>
>>> Hi, Eelco.
>>>
>>> I got a notification from oss-fuzz that this change broke the build
>>> of the fuzzing target:
>>> https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=71524
>>>
>>> Step #3 - "compile-afl-address-x86_64": mv -f $depbase.Tpo $depbase.Po
>>> Step #3 - "compile-afl-address-x86_64": tests/oss-fuzz/odp_target.c:84:72: 
>>> error: too few arguments to function call, expected 8, have 7
>>> Step #3 - "compile-afl-address-x86_64":83 | 
>>> odp_flow_format(odp_key.data, odp_key.size,
>>> Step #3 - "compile-afl-address-x86_64":   | ~~~
>>> Step #3 - "compile-afl-address-x86_64":84 | 
>>> odp_mask.data, odp_mask.size, NULL, &out, false);
>>> Step #3 - "compile-afl-address-x86_64":   | 
>>>^
>>> Step #3 - "compile-afl-address-x86_64": ./lib/odp-util.h:171:6: note: 
>>> 'odp_flow_format' declared here
>>> Step #3 - "compile-afl-address-x86_64":   171 | void odp_flow_format(const 
>>> struct nlattr *key, size_t key_len,
>>> Step #3 - "compile-afl-address-x86_64":   |  ^   
>>> ~
>>> Step #3 - "compile-afl-address-x86_64":   172 |  const 
>>> struct nlattr *mask, size_t mask_len,
>>> Step #3 - "compile-afl-address-x86_64":   |  
>>> ~~~
>>> Step #3 - "compile-afl-address-x86_64":   173 |  const 
>>> struct hmap *portno_names, struct ds *,
>>> Step #3 - "compile-afl-address-x86_64":   |  
>>> ~
>>> Step #3 - "compile-afl-address-x86_64":   174 |  bool 
>>> verbose, bool skip_no_mask);
>>> Step #3 - "compile-afl-address-x86_64":   |  
>>> ~~~
>>> Step #3 - "compile-afl-address-x86_64": 1 error generated.
>>> Step #3 - "compile-afl-address-x86_64": make: *** [Makefile:4718: 
>>> tests/oss-fuzz/odp_target.o] Error 1
>>> Step #3 - "compile-afl-address-x86_64": 
>>> 
>>> Step #3 - "compile-afl-address-x86_64": Failed to build.
>>> Step #3 - "compile-afl-address-x86_64": To reproduce, run:
>>> Step #3 - "compile-afl-address-x86_64": python infra/helper.py build_image 
>>> openvswitch
>>> Step #3 - "compile-afl-address-x86_64": python infra/helper.py 
>>> build_fuzzers --sanitizer address --engine afl --architecture x86_64 
>>> openvswitch
>>> Step #3 - "compile-afl-address-x86_64": 
>>> 
>>> Finished Step #3 - "compile-afl-address-x86_64"
>>> ERROR
>>> ERROR: build step 3 "gcr.io/cloud-builders/docker" failed: step exited with 
>>> non-zero status: 1
>>>
>>>
>>> Could you, please, send a fix?
>>
>> Sent a patch…
>
> Thanks!
>
>> I learned something new today, as I was not aware OVS was part of this 
>> project.
>> Should we do the build as part of the CI?
>
> I don't remember how long does it take to build, but I remember it pulls
> a pile of large container images that takes a while.  Not sure how that
> will impact our CI times.

Yes, it takes quite some time to pull the containers and build. Maybe we can 
just build it standalone, let me investigate, as some of the tools are 
available on Fedora, and they might also be there on Ubuntu.

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

Re: [ovs-dev] [PATCH] dpif: Fix flow put debug message match content.

[Eelco Chaudron]

On 12 Sep 2024, at 14:58, Ilya Maximets wrote:

> On 9/3/24 11:37, Eelco Chaudron wrote:
>> The odp_flow_format() function applies a wildcard mask when a
>> mask for a given key was not present. However, in the context of
>> installing flows in the datapath, the absence of a mask actually
>> indicates that the key should be ignored, meaning it should not
>> be masked at all.
>>
>> To address this inconsistency, odp_flow_format() now includes an
>> option to skip formatting keys that are missing a mask.
>>
>> Signed-off-by: Eelco Chaudron 
>> ---
>>  lib/dpctl.c|  4 ++--
>>  lib/dpif-netdev.c  |  6 +++---
>>  lib/dpif.c |  4 ++--
>>  lib/odp-util.c | 31 ++-
>>  lib/odp-util.h |  2 +-
>>  lib/tnl-ports.c|  2 +-
>>  ofproto/ofproto-dpif.c |  2 +-
>>  tests/test-odp.c   |  6 --
>>  8 files changed, 32 insertions(+), 25 deletions(-)
>
>
> Hi, Eelco.
>
> I got a notification from oss-fuzz that this change broke the build
> of the fuzzing target:
>  https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=71524
>
> Step #3 - "compile-afl-address-x86_64": mv -f $depbase.Tpo $depbase.Po
> Step #3 - "compile-afl-address-x86_64": tests/oss-fuzz/odp_target.c:84:72: 
> error: too few arguments to function call, expected 8, have 7
> Step #3 - "compile-afl-address-x86_64":83 | 
> odp_flow_format(odp_key.data, odp_key.size,
> Step #3 - "compile-afl-address-x86_64":   | ~~~
> Step #3 - "compile-afl-address-x86_64":84 | 
> odp_mask.data, odp_mask.size, NULL, &out, false);
> Step #3 - "compile-afl-address-x86_64":   |   
>  ^
> Step #3 - "compile-afl-address-x86_64": ./lib/odp-util.h:171:6: note: 
> 'odp_flow_format' declared here
> Step #3 - "compile-afl-address-x86_64":   171 | void odp_flow_format(const 
> struct nlattr *key, size_t key_len,
> Step #3 - "compile-afl-address-x86_64":   |  ^   
> ~
> Step #3 - "compile-afl-address-x86_64":   172 |  const 
> struct nlattr *mask, size_t mask_len,
> Step #3 - "compile-afl-address-x86_64":   |  
> ~~~
> Step #3 - "compile-afl-address-x86_64":   173 |  const 
> struct hmap *portno_names, struct ds *,
> Step #3 - "compile-afl-address-x86_64":   |  
> ~
> Step #3 - "compile-afl-address-x86_64":   174 |  bool 
> verbose, bool skip_no_mask);
> Step #3 - "compile-afl-address-x86_64":   |  
> ~~~
> Step #3 - "compile-afl-address-x86_64": 1 error generated.
> Step #3 - "compile-afl-address-x86_64": make: *** [Makefile:4718: 
> tests/oss-fuzz/odp_target.o] Error 1
> Step #3 - "compile-afl-address-x86_64": 
> 
> Step #3 - "compile-afl-address-x86_64": Failed to build.
> Step #3 - "compile-afl-address-x86_64": To reproduce, run:
> Step #3 - "compile-afl-address-x86_64": python infra/helper.py build_image 
> openvswitch
> Step #3 - "compile-afl-address-x86_64": python infra/helper.py build_fuzzers 
> --sanitizer address --engine afl --architecture x86_64 openvswitch
> Step #3 - "compile-afl-address-x86_64": 
> 
> Finished Step #3 - "compile-afl-address-x86_64"
> ERROR
> ERROR: build step 3 "gcr.io/cloud-builders/docker" failed: step exited with 
> non-zero status: 1
>
>
> Could you, please, send a fix?

Sent a patch… I learned something new today, as I was not aware OVS was part of 
this project. Should we do the build as part of the CI?

Cheers,

Eelco

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

[ovs-dev] [PATCH] oss-fuzz: Fix odp_flow_format() API in the fuzz tests.

[Eelco Chaudron]

When the commit below introduced an API change, the oss-fuzz component
was not updated accordingly, leading to failures in the upstream
oss-fuzz tests.

For more information on the failure, see:
  https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=71524

Fixes: 252ee0f18211 ("dpif: Fix flow put debug message match content.")
Signed-off-by: Eelco Chaudron 
---
 tests/oss-fuzz/odp_target.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/oss-fuzz/odp_target.c b/tests/oss-fuzz/odp_target.c
index ae61cdca3..0d615f8a2 100644
--- a/tests/oss-fuzz/odp_target.c
+++ b/tests/oss-fuzz/odp_target.c
@@ -81,7 +81,8 @@ parse_keys(bool wc_keys, const char *in)
 ds_init(&out);
 if (wc_keys) {
 odp_flow_format(odp_key.data, odp_key.size,
-odp_mask.data, odp_mask.size, NULL, &out, false);
+odp_mask.data, odp_mask.size, NULL, &out, false,
+false);
 } else {
 odp_flow_key_format(odp_key.data, odp_key.size, &out);
 }
-- 
2.46.0

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

Re: [ovs-dev] [ovs-vswitchd 3.1] Excessive time spent in clock_gettime()

[Eelco Chaudron]

On 11 Sep 2024, at 14:47, Peter Spreadborough via dev wrote:

> Hi all,
>
> I'm looking at the performance of OVS-DPDK when offloading up to 1M flows
> to hardware and I'm seeing an excessive amount of time being spent in the
> clock_gettime() call. For the samples I have taken using Intel's VTune
> profiler I'm seeing just under a quarter of the entire time in
> clock_gettime().
>
> Is this expected and/or are there any build or runtime options to alleviate
> this?

It’s been a while since I used perf to profile OVS-DPDK, but I can’t remember 
clock_gettime() popped up high on the list.

Is this with 3.1 only? Can you try the latest main or 3.4? Just to be sure it’s 
not something already fixed. In addition, do you see this also with HW offload 
disabled?

Also, I noticed that if no packets are received in a processing loop, we will 
call pmd_thread_ctx_time_update(). So it might be a side effect of the PMD 
thread not having anything to do due to hardware offload, and it’s calling this 
every PMD loop round. You might want to take a loop at the PMD stats, which 
might indicate the CPU is not busy at all, so it should not be a problem.

https://github.com/openvswitch/ovs/blob/234e626198a4d99b6839061ca7e123f5c31c5ebe/lib/dpif-netdev.c#L7317

> I'm testing on a Dell R760 server running Redhat Linux 9.1
> (5.14.0-162.6.1.el9_1.x86_64)
>
> Top Hotspots
> Function  Module   CPU Time % of CPU Time
> clock_gettime  libc.so.697.908s 22.2%
> dp_netdev_process_rxq_port ovs-vswitchd 78.844s 17.9%
> pmd_perf_end_iteration ovs-vswitchd 35.506s  8.0%
> bnxt_rep_rx_burst  ovs-vswitchd 35.330s  8.0%
> bnxt_recv_pkts ovs-vswitchd 33.970s  7.7%
> [Others]   N/A*159.643s 36.2%
>
>
> The call stack of the clock_gettime() calls:
> libc.so.6 ! clock_gettime
> ovs-vswitchd ! time_usec + 0x2f
> ovs-vswitchd ! pmd_thread_main + 0x57c
> ovs-vswitchd ! ovsthread_wrapper + 0x74
> libc.so.6 ! start_thread + 0x2d1
> libc.so.6 ! __clone3 + 0x2f
>
> Thanks,
>
> Pete

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

Re: [ovs-dev] [PATCH] ci: Use previous sarif-tools release due to issue in latest release.

[Eelco Chaudron]

On 11 Sep 2024, at 20:42, Eelco Chaudron wrote:

> The just released v3.0 of the sarif tools do not work as
> expected when comparing results. Temporarily force pip
> to install the 2.0 release until this is fixes.
>
> Signed-off-by: Eelco Chaudron 

Thanks Aaron and Simon for the review. Patch applied.

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

[ovs-dev] [PATCH] ci: Use previous sarif-tools release due to issue in latest release.

[Eelco Chaudron]

The just released v3.0 of the sarif tools do not work as
expected when comparing results. Temporarily force pip
to install the 2.0 release until this is fixes.

Signed-off-by: Eelco Chaudron 
---
 .ci/linux-prepare.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.ci/linux-prepare.sh b/.ci/linux-prepare.sh
index 2a191b57f..5f8a1db6a 100755
--- a/.ci/linux-prepare.sh
+++ b/.ci/linux-prepare.sh
@@ -23,7 +23,7 @@ cd ..
 # https://github.com/pypa/pip/issues/10655
 pip3 install --disable-pip-version-check --user wheel
 pip3 install --disable-pip-version-check --user \
-flake8 netaddr pyparsing sarif-tools sphinx setuptools
+flake8 netaddr pyparsing sarif-tools==2.0.0 sphinx setuptools
 
 # Install python test dependencies
 pip3 install -r python/test_requirements.txt
-- 
2.46.0

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

Re: [ovs-dev] [PATCH] ovsdb-idl: Fix IDL memory leak.

[Eelco Chaudron]

On 2 Sep 2024, at 11:28, Xavier Simonart wrote:

> In the following case, we could see multiple leaks detected for memory 
> allocated
> by ovsdb_idl_txn_add_map_op: insert a row in a table, set a key and delete 
> the row
> (all within the same transaction).
>
> For instance:
> Direct leak of 24 byte(s) in 1 object(s) allocated from:
> 0 0x4e60a7 in calloc (./tests/test-ovsdb+0x4e60a7)
> 1 0x5f9b32 in xcalloc__ ./lib/util.c:125:31
> 2 0x5f9b60 in xzalloc__ ./lib/util.c:135:12
> 3 0x5f9c25 in xzalloc ./ovs/lib/util.c:169:12
> 4 0x5d4899 in ovsdb_idl_txn_add_map_op ./lib/ovsdb-idl.c:4175:29
> 5 0x5d4758 in ovsdb_idl_txn_write_partial_map ./lib/ovsdb-idl.c:4332:5
> 6 0x53cbe8 in idltest_simple2_update_smap_setkey ./tests/idltest.c:4701:5
> 7 0x526fe2 in do_idl_partial_update_map_column ./tests/test-ovsdb.c:3027:5
> 8 0x59d99c in ovs_cmdl_run_command__ ./lib/command-line.c:247:17
> 9 0x59d79a in ovs_cmdl_run_command ./lib/command-line.c:278:5
> 10 0x51d458 in main ./tests/test-ovsdb.c:80:5
> 11 0x7f0a20a79b74 in __libc_start_main (/lib64/libc.so.6+0x27b74)
>
> Signed-off-by: Xavier Simonart 

Thanks for the patch Xavier, it’s been applied to main. I will apply it to 
other branches later this week.

//Eelco

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

Re: [ovs-dev] [PATCH v3 0/8] Address clang analyze warnings.

[Eelco Chaudron]

On 9 Sep 2024, at 6:54, Mike Pattrick wrote:

> This series addresses some warnings produced by clang analyze, and reduces the
> total number of warnings down from 51 to 30. Asside from the theme of reducing
> clang analyze warnings, the individual patches aren't directly related to each
> other.

Thanks Mike for the series. Applied to main, and will backport fixes to older 
branches later this week.

//Eelco

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

Re: [ovs-dev] [PATCH v7 1/2] Userspace: Software fallback for UDP encapsulated TCP segmentation.

[Eelco Chaudron]

On 9 Sep 2024, at 7:04, Mike Pattrick wrote:

> When sending packets that are flagged as requiring segmentation to an
> interface that does not support this feature, send the packet to the TSO
> software fallback instead of dropping it.
>
> Reviewed-by: David Marchand 
> Signed-off-by: Mike Pattrick 

Thanks Mike and all the reviewers. It’s applied to main.

//Eelco

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

Re: [ovs-dev] [PATCH v5] ofproto/bond: Preserve active bond member over restarts.

[Eelco Chaudron]

On 10 Sep 2024, at 19:03, Mike Pattrick wrote:

> On Mon, Sep 9, 2024 at 4:01 AM Jonathan Davies
>  wrote:
>>
>> The OVS DB has a bond_active_slave field. This gets read by
>> port_configure_bond() into struct bond_settings' active_member_mac
>> field. See commit 3e5aeeb5 ("bridge: Keep bond active slave selection
>> across OVS restart") for the original rationale for preserving the
>> active bond member.
>>
>> But since commit 30353934 ("ofproto/bond: Validate active-slave mac.")
>> the bond_settings' active_member_mac field is ignored by bond_create(),
>> which set bond->active_member_mac to eth_addr_zero.
>>
>> Instead, set it to the value of the bond_settings' active_member_mac so
>> that the selection is preserved across OVS restarts.
>>
>> Also add a test that checks this behaviour.
>>
>> Fixes: 303539348848 ("ofproto/bond: Validate active-slave mac.")
>> Signed-off-by: Jonathan Davies 
>
> For posterity: I investigated what happens when the underlying mac
> address changes between restarts. And in that case a new active member
> is selected.
>
> It's unclear to me why eth_addr_zero was set in the first place.
>
> Acked-by: Mike Pattrick 

Thanks for the review Mike. The patch was applied to main and will prepare 
backports.

//Eelco

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

Re: [ovs-dev] [PATCH v2] conntrack: Disambiguate the cleaned count log.

[Eelco Chaudron]

On 11 Sep 2024, at 15:12, Aaron Conole wrote:

> After 3d9c1b855a5f ("conntrack: Replace timeout based expiration lists with 
> rculists.")
> the conntrack cleanup log reports the number of connections it checked
> rather than the number of connections it cleaned.  This patch includes the
> count of connections cleaned during expiration sweeping.
>
> Reported-by: Cheng Li 
> Suggested-by: Cheng Li 
> Fixes: 3d9c1b855a5f ("conntrack: Replace timeout based expiration lists with 
> rculists.")
> Acked-by: Simon Horman 
> Signed-off-by: Aaron Conole 

Thanks Aaron and Simon, the patch was applied to main.

//EElco

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

[ovs-dev] Failing robot/CI...

[Eelco Chaudron]

Hi All,

The Robot/CI seems to fail on the clang-analyze tests. I will dig into this, 
there seems to be a problem with the new released version (it was released 
yesterday).

Cheers,

Eelco


On 10 Sep 2024, at 20:20, 0-day Robot wrote:

> Bleep bloop.  Greetings Aaron Conole, I am a robot and I have tried out your 
> patch.
> Thanks for your contribution.
>
> I encountered some error that I wasn't expecting.  See the details below.
>
>
> checkpatch:
> WARNING: Unexpected sign-offs from developers who are not authors or 
> co-authors or committers: Aaron Conole 
> Lines checked: 158, Warnings: 1, Errors: 0
>
>
> Please check this out.  If you feel there has been an error, please email 
> acon...@redhat.com
>
> Thanks,
> 0-day Robot
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

Re: [ovs-dev] [PATCH v2] conntrack: Disambiguate the cleaned count log.

[Eelco Chaudron]

On 11 Sep 2024, at 15:12, Aaron Conole wrote:

> After 3d9c1b855a5f ("conntrack: Replace timeout based expiration lists with 
> rculists.")
> the conntrack cleanup log reports the number of connections it checked
> rather than the number of connections it cleaned.  This patch includes the
> count of connections cleaned during expiration sweeping.
>
> Reported-by: Cheng Li 
> Suggested-by: Cheng Li 
> Fixes: 3d9c1b855a5f ("conntrack: Replace timeout based expiration lists with 
> rculists.")
> Acked-by: Simon Horman 
> Signed-off-by: Aaron Conole 

Thanks Aaron for cleaning up the message.

Acked-by: Eelco Chaudron 

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

[ovs-dev] [PATCH branch-2.17] ofproto-dpif-upcall: Avoid stale ukeys leaks.

[Eelco Chaudron]

It is observed in some environments that there are much more ukeys than
actual DP flows. For example:

$ ovs-appctl upcall/show
system@ovs-system:
flows : (current 7) (avg 6) (max 117) (limit 2125)
offloaded flows : 525
dump duration : 1063ms
ufid enabled : true

23: (keys 3612)
24: (keys 3625)
25: (keys 3485)

The revalidator threads are busy revalidating the stale ukeys leading to
high CPU and long dump duration.

This patch tracks the number of consecutive missed dumps. If four dumps
are missed in a row, it is assumed that the datapath flow no longer
exists, and the ukey can be deleted.

Reported-by: Roi Dayan 
Co-authored-by: Han Zhou 
Co-authored-by: Roi Dayan 
Signed-off-by: Han Zhou 
Signed-off-by: Roi Dayan 
Signed-off-by: Eelco Chaudron 

NOTE: Backported a portion of
600125b2c380 ("ofproto: Add ofproto/detrace command to map UFIDs to OpenFlow.")
to provide `strip_dp_hash` macro.
---
 ofproto/ofproto-dpif-upcall.c | 16 +
 tests/ofproto-dpif.at | 45 +++
 tests/ofproto-macros.at   |  5 
 3 files changed, 66 insertions(+)

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 047f684e1..5c06beb16 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -55,6 +55,7 @@ COVERAGE_DEFINE(dumped_duplicate_flow);
 COVERAGE_DEFINE(dumped_new_flow);
 COVERAGE_DEFINE(handler_duplicate_upcall);
 COVERAGE_DEFINE(revalidate_missed_dp_flow);
+COVERAGE_DEFINE(revalidate_missing_dp_flow);
 COVERAGE_DEFINE(ukey_dp_change);
 COVERAGE_DEFINE(ukey_invalid_stat_reset);
 COVERAGE_DEFINE(ukey_replace_contention);
@@ -296,6 +297,7 @@ struct udpif_key {
 uint64_t dump_seq OVS_GUARDED;/* Tracks udpif->dump_seq. */
 uint64_t reval_seq OVS_GUARDED;   /* Tracks udpif->reval_seq. */
 enum ukey_state state OVS_GUARDED;/* Tracks ukey lifetime. */
+uint32_t missed_dumps OVS_GUARDED;/* Missed consecutive dumps. */
 
 /* 'state' debug information. */
 unsigned int state_thread OVS_GUARDED;/* Thread that transitions. */
@@ -2924,6 +2926,20 @@ revalidator_sweep__(struct revalidator *revalidator, 
bool purge)
 result = revalidate_ukey(udpif, ukey, &stats, &odp_actions,
  reval_seq, &recircs);
 }
+
+if (ukey->dump_seq != dump_seq) {
+ukey->missed_dumps++;
+if (ukey->missed_dumps >= 4) {
+/* If the flow was not dumped for 4 revalidator rounds,
+ * we can assume the datapath flow no longer exists
+ * and the ukey should be deleted. */
+COVERAGE_INC(revalidate_missing_dp_flow);
+result = UKEY_DELETE;
+}
+} else {
+ukey->missed_dumps = 0;
+}
+
 if (result != UKEY_KEEP) {
 /* Clears 'recircs' if filled by revalidate_ukey(). */
 reval_op_init(&ops[n_ops++], result, udpif, ukey, &recircs,
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 292e6e9b0..6423458d4 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -11933,3 +11933,48 @@ AT_CHECK([test 1 = `ovs-ofctl parse-pcap p2-tx.pcap | 
wc -l`])
 
 OVS_VSWITCHD_STOP
 AT_CLEANUP
+
+AT_SETUP([ofproto-dpif - Cleanup missing datapath flows])
+
+OVS_VSWITCHD_START
+add_of_ports br0 1 2
+
+m4_define([ICMP_PKT], [m4_join([,],
+[eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800)],
+[ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no)],
+[icmp(type=8,code=0)])])
+
+AT_CHECK([ovs-ofctl del-flows br0])
+AT_CHECK([ovs-ofctl add-flow br0 'actions=normal' ])
+
+AT_CHECK([ovs-appctl netdev-dummy/receive p1 'ICMP_PKT'])
+
+AT_CHECK([ovs-appctl dpctl/dump-flows --names | strip_used | strip_stats | dnl
+  strip_duration | strip_dp_hash | sort], [0], [dnl
+flow-dump from the main thread:
+recirc_id(0),in_port(p1),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(frag=no),
 packets:0, bytes:0, used:never, actions:br0,p2
+])
+
+dnl Make sure the ukey exists.
+AT_CHECK([ovs-appctl upcall/show | grep '(keys' | awk '{print $3}' | \
+grep -q '1)'], [0])
+
+dnl Delete all datapath flows, and make sure they are gone.
+AT_CHECK([ovs-appctl dpctl/del-flows])
+AT_CHECK([ovs-appctl dpctl/dump-flows --names ], [0], [])
+
+dnl Move forward in time and make sure we have at least 4 * 500ms.
+AT_CHECK([ovs-appctl time/warp 3000 300], [0], [ignore])
+
+dnl Make sure no more ukeys exists.
+AT_CHECK([ovs-appctl upcall/show | grep '(keys' | awk '{print $3}' | \
+grep -qv '0)'], [1])
+
+dnl Ve

Re: [ovs-dev] [PATCH branch-3.4] ofproto-dpif-upcall: Avoid stale ukeys leaks.

[Eelco Chaudron]

On 10 Sep 2024, at 17:13, Aaron Conole wrote:

> From: Eelco Chaudron 
>
> It is observed in some environments that there are much more ukeys than
> actual DP flows. For example:
>
> $ ovs-appctl upcall/show
> system@ovs-system:
> flows : (current 7) (avg 6) (max 117) (limit 2125)
> offloaded flows : 525
> dump duration : 1063ms
> ufid enabled : true
>
> 23: (keys 3612)
> 24: (keys 3625)
> 25: (keys 3485)
>
> The revalidator threads are busy revalidating the stale ukeys leading to
> high CPU and long dump duration.
>
> This patch tracks the number of consecutive missed dumps. If four dumps
> are missed in a row, it is assumed that the datapath flow no longer
> exists, and the ukey can be deleted.
>
> Reported-by: Roi Dayan 
> Co-authored-by: Han Zhou 
> Co-authored-by: Roi Dayan 
> Signed-off-by: Han Zhou 
> Signed-off-by: Roi Dayan 
> Signed-off-by: Eelco Chaudron 
> Signed-off-by: Aaron Conole 
> (cherry picked from commit 180ab2fd635e03ffab5381bb1c22dda3f13aea7c)

Thanks Aaron for doing all the backport patches on my behalf.

Acked-by: Eelco Chaudron 

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

Re: [ovs-dev] [PATCH branch-3.3] ofproto-dpif-upcall: Avoid stale ukeys leaks.

[Eelco Chaudron]

On 10 Sep 2024, at 17:13, Aaron Conole wrote:

> From: Eelco Chaudron 
>
> It is observed in some environments that there are much more ukeys than
> actual DP flows. For example:
>
> $ ovs-appctl upcall/show
> system@ovs-system:
> flows : (current 7) (avg 6) (max 117) (limit 2125)
> offloaded flows : 525
> dump duration : 1063ms
> ufid enabled : true
>
> 23: (keys 3612)
> 24: (keys 3625)
> 25: (keys 3485)
>
> The revalidator threads are busy revalidating the stale ukeys leading to
> high CPU and long dump duration.
>
> This patch tracks the number of consecutive missed dumps. If four dumps
> are missed in a row, it is assumed that the datapath flow no longer
> exists, and the ukey can be deleted.
>
> Reported-by: Roi Dayan 
> Co-authored-by: Han Zhou 
> Co-authored-by: Roi Dayan 
> Signed-off-by: Han Zhou 
> Signed-off-by: Roi Dayan 
> Signed-off-by: Eelco Chaudron 
> Signed-off-by: Aaron Conole 
>
> NOTE: Backported a portion of
> 600125b2c380 ("ofproto: Add ofproto/detrace command to map UFIDs to 
> OpenFlow.")
> to provide `strip_dp_hash` macro.

Thanks Aaron for doing all the backport patches on my behalf.

Acked-by: Eelco Chaudron 

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

Re: [ovs-dev] [PATCH branch-3.2] ofproto-dpif-upcall: Avoid stale ukeys leaks.

[Eelco Chaudron]

On 10 Sep 2024, at 17:13, Aaron Conole wrote:

> From: Eelco Chaudron 
>
> It is observed in some environments that there are much more ukeys than
> actual DP flows. For example:
>
> $ ovs-appctl upcall/show
> system@ovs-system:
> flows : (current 7) (avg 6) (max 117) (limit 2125)
> offloaded flows : 525
> dump duration : 1063ms
> ufid enabled : true
>
> 23: (keys 3612)
> 24: (keys 3625)
> 25: (keys 3485)
>
> The revalidator threads are busy revalidating the stale ukeys leading to
> high CPU and long dump duration.
>
> This patch tracks the number of consecutive missed dumps. If four dumps
> are missed in a row, it is assumed that the datapath flow no longer
> exists, and the ukey can be deleted.
>
> Reported-by: Roi Dayan 
> Co-authored-by: Han Zhou 
> Co-authored-by: Roi Dayan 
> Signed-off-by: Han Zhou 
> Signed-off-by: Roi Dayan 
> Signed-off-by: Eelco Chaudron 
> Signed-off-by: Aaron Conole 
>
> NOTE: Backported a portion of
> 600125b2c380 ("ofproto: Add ofproto/detrace command to map UFIDs to 
> OpenFlow.")
> to provide `strip_dp_hash` macro.

Thanks Aaron for doing all the backport patches on my behalf.

Acked-by: Eelco Chaudron 

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

Re: [ovs-dev] [PATCH branch-3.1] ofproto-dpif-upcall: Avoid stale ukeys leaks.

[Eelco Chaudron]

On 10 Sep 2024, at 17:13, Aaron Conole wrote:

> From: Eelco Chaudron 
>
> It is observed in some environments that there are much more ukeys than
> actual DP flows. For example:
>
> $ ovs-appctl upcall/show
> system@ovs-system:
> flows : (current 7) (avg 6) (max 117) (limit 2125)
> offloaded flows : 525
> dump duration : 1063ms
> ufid enabled : true
>
> 23: (keys 3612)
> 24: (keys 3625)
> 25: (keys 3485)
>
> The revalidator threads are busy revalidating the stale ukeys leading to
> high CPU and long dump duration.
>
> This patch tracks the number of consecutive missed dumps. If four dumps
> are missed in a row, it is assumed that the datapath flow no longer
> exists, and the ukey can be deleted.
>
> Reported-by: Roi Dayan 
> Co-authored-by: Han Zhou 
> Co-authored-by: Roi Dayan 
> Signed-off-by: Han Zhou 
> Signed-off-by: Roi Dayan 
> Signed-off-by: Eelco Chaudron 
> Signed-off-by: Aaron Conole 
>
> NOTE: Backported a portion of
> 600125b2c380 ("ofproto: Add ofproto/detrace command to map UFIDs to 
> OpenFlow.")
> to provide `strip_dp_hash` macro.

Thanks Aaron for doing all the backport patches on my behalf.

Acked-by: Eelco Chaudron 

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

Re: [ovs-dev] [PATCH branch-3.0] ofproto-dpif-upcall: Avoid stale ukeys leaks.

[Eelco Chaudron]

On 10 Sep 2024, at 17:13, Aaron Conole wrote:

> From: Eelco Chaudron 
>
> It is observed in some environments that there are much more ukeys than
> actual DP flows. For example:
>
> $ ovs-appctl upcall/show
> system@ovs-system:
> flows : (current 7) (avg 6) (max 117) (limit 2125)
> offloaded flows : 525
> dump duration : 1063ms
> ufid enabled : true
>
> 23: (keys 3612)
> 24: (keys 3625)
> 25: (keys 3485)
>
> The revalidator threads are busy revalidating the stale ukeys leading to
> high CPU and long dump duration.
>
> This patch tracks the number of consecutive missed dumps. If four dumps
> are missed in a row, it is assumed that the datapath flow no longer
> exists, and the ukey can be deleted.
>
> Reported-by: Roi Dayan 
> Co-authored-by: Han Zhou 
> Co-authored-by: Roi Dayan 
> Signed-off-by: Han Zhou 
> Signed-off-by: Roi Dayan 
> Signed-off-by: Eelco Chaudron 
> Signed-off-by: Aaron Conole 
>
> NOTE: Backported a portion of
> 600125b2c380 ("ofproto: Add ofproto/detrace command to map UFIDs to 
> OpenFlow.")
> to provide `strip_dp_hash` macro.

Thanks Aaron for doing all the backport patches on my behalf.

I think we should also do this for 2.17 so all supported releases get the 
backport. I will prepare a patch later once the electricity is restored here :(

Cheers,

Eelco

Acked-by: Eelco Chaudron 

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

Re: [ovs-dev] [PATCH] netdev-offload-dpdk: Change log level on flow offload failure.

[Eelco Chaudron]

On 10 Sep 2024, at 17:11, Kevin Traynor wrote:

> Previously when a flow was attempted to be offloaded, if it
> could not be offloaded and did not return an actions error,
> a warning was logged.
>
> The reason there was an exception for an actions error was to allow
> for failure for full offload of a flow because it will fallback to
> partial offload. There are some issues with this approach to logging.
>
> Some NICs do not specify an actions error, because other config in
> the NIC may be checked first. e.g. In the case of Mellanox CX-5,
> there can be different types of offload configured, so an unspecified
> error may be returned.
>
> More generally, enabling hw-offload is best effort per datapath/NIC/flow
> as full and partial offload support in NICs is variable and there is
> always fallback to software.
>
> So there is likely to be repeated logging about offloading of flows
> failing. With this in mind, change the log level to debug.
>
> The status of the offload can still be seen with below command:
> $ ovs-appctl dpctl/dump-flows -m
> ... offloaded:partial ...
>
> Also, remove some duplicated rate limiting.

Thanks for fixing this, and sending the patch. One comment below on the log 
messages.

> Signed-off-by: Kevin Traynor 
> ---
>  lib/netdev-offload-dpdk.c | 23 +--
>  1 file changed, 9 insertions(+), 14 deletions(-)
>
> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
> index 623005b1c..378bee161 100644
> --- a/lib/netdev-offload-dpdk.c
> +++ b/lib/netdev-offload-dpdk.c
> @@ -920,22 +920,17 @@ netdev_offload_dpdk_flow_create(struct netdev *netdev,
>  dump_flow(&s, &s_extra, attr, flow_patterns, flow_actions);
>  extra_str = ds_cstr(&s_extra);
> -VLOG_DBG_RL(&rl, "%s: rte_flow 0x%"PRIxPTR" %s  flow create %d 
> %s",
> -netdev_get_name(netdev), (intptr_t) flow, extra_str,
> -netdev_dpdk_get_port_id(netdev), ds_cstr(&s));
> +VLOG_DBG("%s: rte_flow 0x%"PRIxPTR" %s  flow create %d %s",
> + netdev_get_name(netdev), (intptr_t) flow, extra_str,
> + netdev_dpdk_get_port_id(netdev), ds_cstr(&s));
>  }
>  } else {
> -enum vlog_level level = VLL_WARN;
> -
> -if (error->type == RTE_FLOW_ERROR_TYPE_ACTION) {
> -level = VLL_DBG;
> -}
> -VLOG_RL(&rl, level, "%s: rte_flow creation failed: %d (%s).",
> -netdev_get_name(netdev), error->type, error->message);
> -if (!vlog_should_drop(&this_module, level, &rl)) {
> +if (!VLOG_DROP_DBG(&rl)) {
> +VLOG_DBG("%s: rte_flow creation failed: %d (%s).",
> + netdev_get_name(netdev), error->type, error->message);
>  dump_flow(&s, &s_extra, attr, flow_patterns, flow_actions);
>  extra_str = ds_cstr(&s_extra);
> -VLOG_RL(&rl, level, "%s: Failed flow: %s  flow create %d %s",
> -netdev_get_name(netdev), extra_str,
> -netdev_dpdk_get_port_id(netdev), ds_cstr(&s));
> +VLOG_DBG("%s: Failed flow: %s  flow create %d %s",
> + netdev_get_name(netdev), extra_str,
> + netdev_dpdk_get_port_id(netdev), ds_cstr(&s));

Would it make sense to combine the two debug log messages?
Something in the lines of:

%s: rte_flow creation failed [%d (%s)]: %s  flow create %d %s”


>  }
>  }
> -- 
> 2.46.0

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

Re: [ovs-dev] [PATCH v3 4/8] netlink-socket: Initialize socket family.

[Eelco Chaudron]

On 9 Sep 2024, at 6:55, Mike Pattrick wrote:

> The Clang analyzer will alert on the use of uninitialized variable local
> despite the fact that this should be set by a syscall.
>
> To suppress the warning, this variable is now initialized.
>
> Signed-off-by: Mike Pattrick 

Thanks for sending out the v3, the changes look good to me.

Cheers,

Eelco

Acked-by: Eelco Chaudron 

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

Re: [ovs-dev] [PATCH v3 2/8] dpif-netdev: Remove undefined integer division.

[Eelco Chaudron]

On 9 Sep 2024, at 6:54, Mike Pattrick wrote:

> Clang analyzer will complain about floating point operations conducted
> with integer types as rounding is undefined. In pmd_info_show_rxq() a
> percentage was calculated inside uint64 integers instead of a floating
> pointer variable for a user visible message. This issue can be resolved
> simply by casting to double while dividing.
>
> Signed-off-by: Mike Pattrick 

Thanks for sending out the v3, the changes look good to me.

Cheers,

Eelco

Acked-by: Eelco Chaudron 

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

Re: [ovs-dev] [PATCH v3 6/8] vconn: Always properly free flow stats reply.

[Eelco Chaudron]

On 9 Sep 2024, at 6:55, Mike Pattrick wrote:

> Currently the error conditions in vconn_dump_flows() don't handle
> freeing memory in a consistent fashion. This can make it possible to
> reference memory after it's freed.
>
> This patch attempts to handle errors consistently. Error conditions will
> always cause memory to be freed and then that memory will never be
> referenced.
>
> Fixes: d444a914fdbd ("ovn-trace: New --ovs option to also print OpenFlow 
> flows.")
> Signed-off-by: Mike Pattrick 

The changes look good to me.

Acked-by: Eelco Chaudron 

//Eelco

Had to spend some time figuring out why ‘*replyp = NULL;’ had to be moved out 
of the !more part…

> ---
> v3: Simplified logic per review.
> ---
>  lib/vconn.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/lib/vconn.c b/lib/vconn.c
> index e9603432d..4b1c262ea 100644
> --- a/lib/vconn.c
> +++ b/lib/vconn.c
> @@ -1017,6 +1017,8 @@ recv_flow_stats_reply(struct vconn *vconn, ovs_be32 
> send_xid,
>  VLOG_WARN_RL(&rl, "received bad reply: %s",
>   ofp_to_string(reply->data, reply->size,
> NULL, NULL, 1));
> +ofpbuf_delete(reply);
> +*replyp = NULL;
>  return EPROTO;
>  }
>  }
> @@ -1031,9 +1033,9 @@ recv_flow_stats_reply(struct vconn *vconn, ovs_be32 
> send_xid,
>  case EOF:
>  more = ofpmp_more(reply->header);
>  ofpbuf_delete(reply);
> +*replyp = NULL;
>  reply = NULL;
>  if (!more) {
> -*replyp = NULL;
>  return EOF;
>  }
>  break;
> @@ -1041,6 +1043,8 @@ recv_flow_stats_reply(struct vconn *vconn, ovs_be32 
> send_xid,
>  default:
>  VLOG_WARN_RL(&rl, "parse error in reply (%s)",
>   ofperr_to_string(retval));
> +ofpbuf_delete(reply);
> +*replyp = NULL;
>  return EPROTO;
>  }
>  }
> -- 
> 2.43.5
>
> ___
> 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 v3 1/8] ovsdb-error: Annotate non-null functions.

[Eelco Chaudron]

On 9 Sep 2024, at 6:54, Mike Pattrick wrote:

> The Clang analyzer has trouble detecting that functions can never return
> null in certain conditions, this results in several false "Dereference of
> null pointer" detections.
>
> This patch annotates functions that call ovsdb_error_valist()
> unconditionally as non-null, as this function will either return a valid
> pointer or call abort().
>
> Signed-off-by: Mike Pattrick 

Thanks for sending out the v3, the changes look good to me.

Cheers,

Eelco

Acked-by: Eelco Chaudron 

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

Re: [ovs-dev] [PATCH v3 8/8] mcast-snooping: Don't access ovs_list members directly.

[Eelco Chaudron]

On 9 Sep 2024, at 6:55, Mike Pattrick wrote:

> The Clang analyzer has trouble tracking the pointer usage in
> mrouter_get_lru and will report a use after free incorrectly. This patch
> migrates to using standard ovs_list functions instead of directly
> accessing the next member, which suppresses clang's warning.
>
> Acked-by: Eelco Chaudron 
> Signed-off-by: Mike Pattrick 

Thanks for sending out the v3, the changes look good to me.

Cheers,

Eelco

Acked-by: Eelco Chaudron 

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

Re: [ovs-dev] [PATCH v3 7/8] mcast-snooping: Properly check group_get_lru return code.

[Eelco Chaudron]

On 9 Sep 2024, at 6:55, Mike Pattrick wrote:

> Previously error codes from group_get_lru() were ignored, possibly
> causing NULL pointer dereferencing. This patch appropriately checks for
> errors.
>
> Fixes: 4a95091d1f66 ("lib: Add IGMP snooping library bits")
> Acked-by: Eelco Chaudron 
> Signed-off-by: Mike Pattrick 

Thanks for sending out the v3, the changes look good to me.

Cheers,

Eelco

Acked-by: Eelco Chaudron 

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

Re: [ovs-dev] [PATCH v3 5/8] classifier: Store n_indices between usage.

[Eelco Chaudron]

On 9 Sep 2024, at 6:55, Mike Pattrick wrote:

> Currently the Clang analyzer will complain about usage of an
> uninitialized variable in the classifier. This is a false positive, but
> not for a reason that could easily be detectable by clang.
>
> The classifier is not safe for multiple writer threads to use
> simultaneously so all callers protect these functions from simultaneous
> writes. However, this is not so clear from the code's static analysis
> alone. To help Clang out here, the n_indicies count is saved onto the
> stack instead of accessed from the subtables struct repeatedly.
>
> Acked-by: Eelco Chaudron 
> Signed-off-by: Mike Pattrick 

Thanks for sending out the v3, the changes look good to me.

Cheers,

Eelco

Acked-by: Eelco Chaudron 

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

Re: [ovs-dev] [PATCH v3 3/8] jsonrpc: Don't access ovs_list members directly.

[Eelco Chaudron]

On 9 Sep 2024, at 6:55, Mike Pattrick wrote:

> The Clang analyzer has trouble tracking the pointer usage in jsonrpc_run
> and will report a use after free incorrectly. This patch migrates to
> using standard ovs_list functions instead of directly accessing the next
> member, which suppresses clang's warning.
>
> Acked-by: Eelco Chaudron 
> Signed-off-by: Mike Pattrick 

Thanks for sending out the v3, the changes look good to me.

Cheers,

Eelco

Acked-by: Eelco Chaudron 

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

Re: [ovs-dev] [PATCH v7 2/2] netdev-dpdk: Re-enable VXLAN/Geneve offload for Intel cards.

[Eelco Chaudron]

On 9 Sep 2024, at 7:04, Mike Pattrick wrote:

> Previously support for UDP tunneled traffic TCP traffic with UDP
> checksum offloading did not work well in cases where the sending network
> card didn't also support these features.
>
> Some of the code had been written to assume that if a card supported
> VXLAN/Geneve offloading, then it also supported outer UDP checksum
> offloading. However, this was not the case for some Intel network cards.
>
> A previous change disabled the VXLAN/Geneve offload flags for these
> cards as a temporary fix. However, with "Userspace: Software fallback
> for UDP encapsulated TCP segmentation.", the logic related to software
> fallback for checksum offloading now anticipates this configuration.
>
> The modification to the outer UDP offload flag is still required. This
> feature does not work as expected in the current DPDK release.
>
> Suggested-by: David Marchand 
> Reviewed-by: David Marchand 
> Signed-off-by: Mike Pattrick 

Change looks good to me in combination with the first patch of this series.

Acked-by: Eelco Chaudron 


Note: this is technically v8, but this is removed at commit time anyway.

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

Re: [ovs-dev] [PATCH v7 1/2] Userspace: Software fallback for UDP encapsulated TCP segmentation.

[Eelco Chaudron]

On 9 Sep 2024, at 7:04, Mike Pattrick wrote:

> When sending packets that are flagged as requiring segmentation to an
> interface that does not support this feature, send the packet to the TSO
> software fallback instead of dropping it.
>
> Reviewed-by: David Marchand 
> Signed-off-by: Mike Pattrick 

Thanks for making the additional style changes.

Acked-by: Eelco Chaudron 

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

Re: [ovs-dev] [PATCH] netdev-dpdk: Fixed RCU deadlock when deleting vhostuser port

[Eelco Chaudron]

On 6 Sep 2024, at 16:03, Kevin Traynor wrote:

> On 06/09/2024 07:38, Eelco Chaudron wrote:
>>
>>
>> On 5 Sep 2024, at 18:35, Kevin Traynor wrote:
>>
>>> On 08/08/2024 10:57, Xinxin Zhao wrote:
>>>> When the ovs control thread del vhost-user port and
>>>> the vhost-event thread process the vhost-user port down concurrently,
>>>> the main thread may fall into a deadlock.
>>>>
>>>> E.g., vhostuser port is created as client.
>>>> The ovs control thread executes the following process:
>>>> rte_vhost_driver_unregister->fdset_try_del.
>>>> At the same time, the vhost-event thread executes the following process:
>>>> fdset_event_dispatch->vhost_user_read_cb->destroy_device.
>>>> At this time, vhost-event will wait for rcu scheduling,
>>>> and the ovs control thread is waiting for pfdentry->busy to be 0.
>>>> The two threads are waiting for each other and fall into a deadlock.
>>>>
>>>
>>> Hi Xinxin,
>>>
>>> Thanks for the patch. I managed to reproduced this with a little bit of
>>> hacking. Indeed, a deadlock can occur with some unlucky timing.
>>>
>>> Acked-by: Kevin Traynor 
>>
>> Kevin or Xinxin, can you add some more explanation on where the deadlock is 
>> occurring?
>>
>
> vhost-event thread is blocking on the synchronize, waiting for main
> thread to queisce, i.e.
>
> #3  0x03690c09 in ovsrcu_synchronize () at lib/ovs-rcu.c:237
> #4  0x037162fa in destroy_device (vid=0) at lib/netdev-dpdk.c:4919
> #5  0x03238a12 in vhost_destroy_device_notify (dev=0x14ba693c0)
> at ../lib/vhost/vhost.c:756
>
> While main thread is looping in dpdk unregister, waiting for vhost-event
> callback to finish, i.e.:
>
> #1  0x032336e8 in rte_vhost_driver_unregister (path=0xd217a30
> "/tmp/vhost0") at ../lib/vhost/socket.c:1075
> #2  0x0370d5f3 in dpdk_vhost_driver_unregister (dev=0x17d6e8b40,
> vhost_id=0xd217a30 "/tmp/vhost0") at lib/netdev-dpdk.c:1811
> #3  0x0370d709 in netdev_dpdk_vhost_destruct
> (netdev=0x17d6e8bc0) at lib/netdev-dpdk.c:1843
>
>> Also, how do we guarantee that it’s safe to go to quiesce state and that no 
>> others in the call chain hold/use any RCU-protected data?
>>
>
> It should be fine for anything rcu freed in the main thread (possible
> mirror bridge struct) as other threads would need to quiesce too, but
> you have a point about anything used in main thread. We are deep into
> bridge_run(), so I'm not sure how we could test for every scenario.
>
> If we can't guarantee it, then maybe another approach is needed, perhaps
> we could hijack the ovs_vhost thread mechanism to call the unregister ?
> but i'm not sure if there's other implications doing it asynchronously.


This callback is called by netdev_unref(), which is called for example by 
netdev_close(). netdev_close() is called all over the place which makes it 
unsafe to just go through quiesce state. I guess we need another way to fix 
this.

>> Thanks,
>>
>> Eelco
>>
>>>> Fixes: afee281 ("netdev-dpdk: Fix dpdk_watchdog failure to quiesce.")
>>>>
>>>> Signed-off-by: Xinxin Zhao <15957197...@163.com>
>>>> ---
>>>>  lib/netdev-dpdk.c | 11 ++-
>>>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>>>> index 02cef6e45..0c02357f5 100644
>>>> --- a/lib/netdev-dpdk.c
>>>> +++ b/lib/netdev-dpdk.c
>>>> @@ -1808,7 +1808,16 @@ dpdk_vhost_driver_unregister(struct netdev_dpdk 
>>>> *dev OVS_UNUSED,
>>>>  OVS_EXCLUDED(dpdk_mutex)
>>>>  OVS_EXCLUDED(dev->mutex)
>>>>  {
>>>> -return rte_vhost_driver_unregister(vhost_id);
>>>> +int ret;
>>>> +/* Due to the rcu wait of the vhost-event thread,
>>>> + * rte_vhost_driver_unregister() may loop endlessly.
>>>> + * So the unregister action needs to be removed from the rcu_list.
>>>> + */
>>>> +ovsrcu_quiesce_start();
>>>> +ret = rte_vhost_driver_unregister(vhost_id);
>>>> +ovsrcu_quiesce_end();
>>>> +
>>>> +return ret;
>>>>  }
>>>>
>>>>  static void
>>>
>>> ___
>>> 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] ovsdb-idl: Fix IDL memory leak.

[Eelco Chaudron]

On 2 Sep 2024, at 11:28, Xavier Simonart wrote:

> In the following case, we could see multiple leaks detected for memory 
> allocated
> by ovsdb_idl_txn_add_map_op: insert a row in a table, set a key and delete 
> the row
> (all within the same transaction).
>
> For instance:
> Direct leak of 24 byte(s) in 1 object(s) allocated from:
> 0 0x4e60a7 in calloc (./tests/test-ovsdb+0x4e60a7)
> 1 0x5f9b32 in xcalloc__ ./lib/util.c:125:31
> 2 0x5f9b60 in xzalloc__ ./lib/util.c:135:12
> 3 0x5f9c25 in xzalloc ./ovs/lib/util.c:169:12
> 4 0x5d4899 in ovsdb_idl_txn_add_map_op ./lib/ovsdb-idl.c:4175:29
> 5 0x5d4758 in ovsdb_idl_txn_write_partial_map ./lib/ovsdb-idl.c:4332:5
> 6 0x53cbe8 in idltest_simple2_update_smap_setkey ./tests/idltest.c:4701:5
> 7 0x526fe2 in do_idl_partial_update_map_column ./tests/test-ovsdb.c:3027:5
> 8 0x59d99c in ovs_cmdl_run_command__ ./lib/command-line.c:247:17
> 9 0x59d79a in ovs_cmdl_run_command ./lib/command-line.c:278:5
> 10 0x51d458 in main ./tests/test-ovsdb.c:80:5
> 11 0x7f0a20a79b74 in __libc_start_main (/lib64/libc.so.6+0x27b74)
>
> Signed-off-by: Xavier Simonart 
> ---

I looked at the patch and with my minimal ovsdb knowledge this 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] dpif: Fix flow put debug message match content.

[Eelco Chaudron]

On 4 Sep 2024, at 15:54, Simon Horman wrote:

> On Wed, Sep 04, 2024 at 01:40:52PM +0200, Eelco Chaudron wrote:
>>
>>
>> On 4 Sep 2024, at 12:38, Simon Horman wrote:
>>
>>> On Tue, Sep 03, 2024 at 11:37:16AM +0200, Eelco Chaudron wrote:
>>>> The odp_flow_format() function applies a wildcard mask when a
>>>> mask for a given key was not present. However, in the context of
>>>> installing flows in the datapath, the absence of a mask actually
>>>> indicates that the key should be ignored, meaning it should not
>>>> be masked at all.
>>>>
>>>> To address this inconsistency, odp_flow_format() now includes an
>>>> option to skip formatting keys that are missing a mask.
>>>
>>> Hi Eelco,
>>>
>>> Would you be able to provide some example output,
>>> say as part of the commit message.
>>
>> I can add the following example to the commit message:
>>
>> This was found during a debug session of the ‘datapath - ping between two
>> ports on cvlan’ test case. The log message was showing the following:
>>
>>   put[create] ufid:XX recirc_id(0),dp_hash(0/0),skb_priority(0/0),in_port(3),
>> skb_mark(0/0),ct_state(0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),
>> eth(src=12:f6:8b:52:f9:75,dst=6e:48:c8:77:d3:8c),eth_type(0x88a8),
>> vlan(vid=4094,pcp=0/0x0),encap(eth_type(0x8100),
>> vlan(vid=100/0x0,pcp=0/0x0),encap(eth_type(0x0800),
>> ipv4(src=10.2.2.2,dst=10.2.2.1,proto=1,tos=0,ttl=64,frag=no),
>> icmp(type=0,code=0))), actions:2
>>
>> Where it should have shown the below:
>>
>>   put[create] ufid:XX recirc_id(0),dp_hash(0/0),skb_priority(0/0),in_port(3),
>> skb_mark(0/0),ct_state(0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),
>> eth(src=12:f6:8b:52:f9:75,dst=6e:48:c8:77:d3:8c),eth_type(0x88a8),
>> vlan(vid=4094,pcp=0/0x0),encap(eth_type(0x8100)), actions:2
>
> Thanks,
>
> Could you include that when applying the patch, or post a v2,
> whichever you prefer?
>
>>
>>> And, does this warrant a test?
>>
>> Good question, as this was a debug message I thought it would not
>> matter much. But if you feel it’s needed, I can try adding a
>> dp independent unit test.
>
> I think we can pass on the test if it is just a debugging message.
>
>>
>>>>
>>>> Signed-off-by: Eelco Chaudron 
>>>
>>> ...
>
> Acked-by: Simon Horman 

Thanks Simon, applied to main.

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

Re: [ovs-dev] [PATCH v5 3/3] ovsdb: Optimize monitor update by directly serializing data into ds.

[Eelco Chaudron]

On 1 Jul 2024, at 13:02, Grigorii Nazarov wrote:

Hi Grigorii,

This is not an in-depth review, but find some comments below.

Cheers,

Eelco

> Currently serialization is performed by first converting
> the internal data representation into JSON objects, followed by
> serializing these objects by jsonrpc. This process results in lots of
> allocations for these intermediate objects. Consequently, this
> not only increases peak memory consumption, but also
> demands significantly more CPU work.
>
> By forming row-update JSONs directly in `ds`, which is then used
> to create 'serialized object' JSONs, the overall speed increased
> by a factor of 2.3.
>
> A local benchmark was run on a proprietary Southbound backup.
> Both versions, before and after applying the patch, were measured.
> For each version, there were two runs with 10 parallel clients,
> and two runs with 30 parallel clients. CPU time was recorded
> after startup (before clients started running) and after
> all clients received all updates. Clients were essentially running
> `ovsdb-client monitor-cond unix:pipe OVN_Southbound
> '[[\"actions\",\"!=\",\"sdfdfsd\"]]' Logical_Flow`.
> Similar results were obtained with other requests
> that required a significant amount of data transfer.
> The backup size is about 600 MB. Results are measured in seconds.
>
> Before  After
> Baseline x10:   9.53108.54
> Baseline x10:   9.62108.67
> Baseline x30:   9.69307.04
> Baseline x30:   9.65303.32
> Patch x10:  9.6752.57
> Patch x10:  9.5753.12
> Patch x30:  9.53136.33
> Patch x30:  9.63135.88

Are before and after times reversed? I assume a shorter time is better?
Also, how does this relate to the 2.3x speed improvement?

> Signed-off-by: Grigorii Nazarov 
> ---
> v2: updated title
> v3: fixed bracing
> v4: changed patch number from 4/4 to 3/3
>
>  include/openvswitch/json.h |   1 +
>  lib/json.c |   8 ++
>  lib/ovsdb-data.c   | 105 +++
>  lib/ovsdb-data.h   |   3 +
>  ovsdb/monitor.c| 210 -
>  5 files changed, 254 insertions(+), 73 deletions(-)
>
> diff --git a/include/openvswitch/json.h b/include/openvswitch/json.h
> index 555440760..80b9479c7 100644
> --- a/include/openvswitch/json.h
> +++ b/include/openvswitch/json.h
> @@ -81,6 +81,7 @@ struct json *json_boolean_create(bool);
>  struct json *json_string_create(const char *);
>  struct json *json_string_create_nocopy(char *);
>  struct json *json_serialized_object_create(const struct json *);
> +struct json *json_serialized_object_create_from_string(const char *);
>  struct json *json_integer_create(long long int);
>  struct json *json_real_create(double);
>
> diff --git a/lib/json.c b/lib/json.c
> index d40e93857..66b1f571f 100644
> --- a/lib/json.c
> +++ b/lib/json.c
> @@ -199,6 +199,14 @@ json_serialized_object_create(const struct json *src)
>  return json;
>  }
>
> +struct json *
> +json_serialized_object_create_from_string(const char *s)
> +{
> +struct json *json = json_create(JSON_SERIALIZED_OBJECT);
> +json->string = xstrdup(s);
> +return json;
> +}
> +
>  struct json *
>  json_serialized_object_create_with_yield(const struct json *src)
>  {
> diff --git a/lib/ovsdb-data.c b/lib/ovsdb-data.c
> index defb048d7..f32b7975a 100644
> --- a/lib/ovsdb-data.c
> +++ b/lib/ovsdb-data.c
> @@ -492,6 +492,45 @@ ovsdb_atom_to_json__(const union ovsdb_atom *atom, enum 
> ovsdb_atomic_type type,
>  }
>  }
>
> +/* Serializes 'atom', of the specified 'type', into 'ds' in JSON format.
> + *
> + * Refer to RFC 7047 for the format of the JSON that this function produces. 
> */
> +static void
> +ovsdb_atom_to_json_ds(const union ovsdb_atom *atom,
> +  enum ovsdb_atomic_type type, struct ds *ds)

Isn’t this a duplicate of ovsdb_atom_to_string()?

> +{
> +switch (type) {
> +case OVSDB_TYPE_VOID:
> +OVS_NOT_REACHED();
> +
> +case OVSDB_TYPE_INTEGER:
> +ds_put_format(ds, "%lld", (long long) atom->integer);

Rather than typecast this one, just use:

 ds_put_format(ds, "%"PRId64, atom->integer);

> +return;
> +
> +case OVSDB_TYPE_REAL:
> +ds_put_format(ds, "%.*g", DBL_DIG, atom->real);
> +return;
> +
> +case OVSDB_TYPE_BOOLEAN:
> +ds_put_cstr(ds, atom->boolean ? "true" : "false");
> +return;
> +
> +case OVSDB_TYPE_STRING:
> +json_to_ds(atom->s, JSSF_SORT, ds);
> +return;
> +
> +case OVSDB_TYPE_UUID:
> +ds_put_cstr(ds, "[\"uuid\",\"");
> +ds_put_format(ds, UUID_FMT, UUID_ARGS(&atom->uuid));
> +ds_put_cstr(ds, "\"]");
> +return;
> +
> +case OVSDB_N_TYPES:
> +default:
> +OVS_NOT_REACHED();
> +}
> +}
> +
>  struct json *
>  ovsdb_atom_to_json(const union ovsdb_atom *atom, enum ovsdb_atomic_type type)
>  {
> @@ -1445,6 +1484,23 @@ ovsdb_base_to_json(const union ovsdb_atom *atom,
>  

Re: [ovs-dev] [PATCH v5 2/3] lib/json: Simplify string serialization code.

[Eelco Chaudron]

On 6 Sep 2024, at 12:24, Eelco Chaudron wrote:

> On 1 Jul 2024, at 13:02, Grigorii Nazarov wrote:
>
>> Signed-off-by: Grigorii Nazarov 
>> ---
>> There's an open question on whether this function should exist, or being
>> placed in header etc. However, no decision was made yet.
>
> I looked at the previous discussion, I’m fine with just keeping this API.
>
> If we want to inline it, we need to expose json_serialize_string which then 
> it might be better to just use/expose that API instead.
>
> Anyway;
>
> Acked-by: Eelco Chaudron 

Forgot to add a small nit, see below.

>> v2: fixed title
>> v4: changed patch number from 3/4 to 2/3
>>
>>  lib/json.c | 11 ---
>>  1 file changed, 4 insertions(+), 7 deletions(-)
>>
>> diff --git a/lib/json.c b/lib/json.c
>> index 001f6e6ab..d40e93857 100644
>> --- a/lib/json.c
>> +++ b/lib/json.c
>> @@ -127,7 +127,9 @@ static void json_parser_input(struct json_parser *, 
>> struct json_token *);
>>
>>  static void json_error(struct json_parser *p, const char *format, ...)
>>  OVS_PRINTF_FORMAT(2, 3);
>> -
>> +
>> +static void json_serialize_string(const char *, struct ds *);
>> +
>>  const char *
>>  json_type_to_string(enum json_type type)
>>  {
>> @@ -987,11 +989,7 @@ exit:
>>  void
>>  json_string_escape(const char *in, struct ds *out)
>>  {
>> -struct json json = {
>> -.type = JSON_STRING,
>> -.string = CONST_CAST(char *, in),
>> -};
>> -json_to_ds(&json, 0, out);
>> +json_serialize_string(in, out);
>>  }
>>
>>  static void
>> @@ -1572,7 +1570,6 @@ static void json_serialize_object(const struct shash 
>> *object,
>>struct json_serializer *);
>>  static void json_serialize_array(const struct json_array *,
>>   struct json_serializer *);
>> -static void json_serialize_string(const char *, struct ds *);

If we move the static definition, I would move all of them, so they are at one 
location.

>>  /* Converts 'json' to a string in JSON format, encoded in UTF-8, and returns
>>   * that string.  The caller is responsible for freeing the returned string,
>> -- 
>> 2.45.2
>>
>> ___
>> 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 v5 2/3] lib/json: Simplify string serialization code.

[Eelco Chaudron]

On 1 Jul 2024, at 13:02, Grigorii Nazarov wrote:

> Signed-off-by: Grigorii Nazarov 
> ---
> There's an open question on whether this function should exist, or being
> placed in header etc. However, no decision was made yet.

I looked at the previous discussion, I’m fine with just keeping this API.

If we want to inline it, we need to expose json_serialize_string which then it 
might be better to just use/expose that API instead.

Anyway;

Acked-by: Eelco Chaudron 

> v2: fixed title
> v4: changed patch number from 3/4 to 2/3
>
>  lib/json.c | 11 ---
>  1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/lib/json.c b/lib/json.c
> index 001f6e6ab..d40e93857 100644
> --- a/lib/json.c
> +++ b/lib/json.c
> @@ -127,7 +127,9 @@ static void json_parser_input(struct json_parser *, 
> struct json_token *);
>
>  static void json_error(struct json_parser *p, const char *format, ...)
>  OVS_PRINTF_FORMAT(2, 3);
> -
> +
> +static void json_serialize_string(const char *, struct ds *);
> +
>  const char *
>  json_type_to_string(enum json_type type)
>  {
> @@ -987,11 +989,7 @@ exit:
>  void
>  json_string_escape(const char *in, struct ds *out)
>  {
> -struct json json = {
> -.type = JSON_STRING,
> -.string = CONST_CAST(char *, in),
> -};
> -json_to_ds(&json, 0, out);
> +json_serialize_string(in, out);
>  }
>
>  static void
> @@ -1572,7 +1570,6 @@ static void json_serialize_object(const struct shash 
> *object,
>struct json_serializer *);
>  static void json_serialize_array(const struct json_array *,
>   struct json_serializer *);
> -static void json_serialize_string(const char *, struct ds *);
>
>  /* Converts 'json' to a string in JSON format, encoded in UTF-8, and returns
>   * that string.  The caller is responsible for freeing the returned string,
> -- 
> 2.45.2
>
> ___
> 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 v5 1/3] ovsdb: Simplify UUID formatting code.

[Eelco Chaudron]

On 1 Jul 2024, at 13:02, Grigorii Nazarov wrote:

> Signed-off-by: Grigorii Nazarov 

Hi Grigorii,

Thanks for the patch, see some comments below.

> ---
> v2: fixed title
> v4: changed patch number from 2/4 to 1/3
>
>  lib/ovsdb-data.c | 8 +---
>  lib/uuid.h   | 1 +
>  2 files changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/lib/ovsdb-data.c b/lib/ovsdb-data.c
> index abb923ad8..defb048d7 100644
> --- a/lib/ovsdb-data.c
> +++ b/lib/ovsdb-data.c
> @@ -2582,14 +2582,8 @@ char *
>  ovsdb_data_row_name(const struct uuid *uuid)
>  {
>  char *name;
> -char *p;
>
> -name = xasprintf("row"UUID_FMT, UUID_ARGS(uuid));
> -for (p = name; *p != '\0'; p++) {
> -if (*p == '-') {
> -*p = '_';
> -}
> -}
> +name = xasprintf(UUID_ROW_FMT, UUID_ARGS(uuid));
>
>  return name;
>  }
> diff --git a/lib/uuid.h b/lib/uuid.h
> index fa49354f6..6a8069f68 100644
> --- a/lib/uuid.h
> +++ b/lib/uuid.h
> @@ -34,6 +34,7 @@ extern "C" {
>   */
>  #define UUID_LEN 36
>  #define UUID_FMT "%08x-%04x-%04x-%04x-%04x%08x"
> +#define UUID_ROW_FMT "row%08x_%04x_%04x_%04x_%04x%08x"

The “row” addition is rather dbase specific. What about calling it:

  #define UUID_UNDERSCORE_FMT "%08x_%04x_%04x_%04x_%04x%08x"

and add the “row” part in the xasprintf() directly, so:

  name = xasprintf(“row”UUID_UNDERSCORE_FMT, UUID_ARGS(uuid));


>  #define UUID_ARGS(UUID) \
>  ((unsigned int) ((UUID)->parts[0])),\
>  ((unsigned int) ((UUID)->parts[1] >> 16)),  \
> -- 
> 2.45.2
>
> ___
> 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] conntrack: Disambiguate the cleaned count log.

[Eelco Chaudron]

On 5 Sep 2024, at 14:32, Aaron Conole wrote:

> After 3d9c1b855a5f ("conntrack: Replace timeout based expiration lists with 
> rculists.")
> the conntrack cleanup log reports the number of connections it checked
> rather than the number of connections it cleaned.  This patch includes the
> count of connections cleaned during expiration sweeping.

The patch looks good to me, however one comments on the log message itself.

//Eelco


> Reported-by: Cheng Li 
> Suggested-by: Cheng Li 
> Fixes: 3d9c1b855a5f ("conntrack: Replace timeout based expiration lists with 
> rculists.")
> Signed-off-by: Aaron Conole 
> ---
>  lib/conntrack.c | 18 +++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index db44f82374..e90c2ac19f 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -1443,20 +1443,27 @@ conntrack_get_sweep_interval(struct conntrack *ct)
>  }
>
>  static size_t
> -ct_sweep(struct conntrack *ct, struct rculist *list, long long now)
> +ct_sweep(struct conntrack *ct, struct rculist *list, long long now,
> + size_t *cleaned_count)
>  OVS_NO_THREAD_SAFETY_ANALYSIS
>  {
>  struct conn *conn;
> +size_t cleaned = 0;
>  size_t count = 0;
>
>  RCULIST_FOR_EACH (conn, node, list) {
>  if (conn_expired(conn, now)) {
>  conn_clean(ct, conn);
> +cleaned++;
>  }
>
>  count++;
>  }
>
> +if (cleaned_count) {
> +*cleaned_count = cleaned;
> +}
> +
>  return count;
>  }
>
> @@ -1469,22 +1476,27 @@ conntrack_clean(struct conntrack *ct, long long now)
>  long long next_wakeup = now + conntrack_get_sweep_interval(ct);
>  unsigned int n_conn_limit, i;
>  size_t clean_end, count = 0;
> +size_t total_cleaned = 0;
>
>  atomic_read_relaxed(&ct->n_conn_limit, &n_conn_limit);
>  clean_end = n_conn_limit / 64;
>
>  for (i = ct->next_sweep; i < N_EXP_LISTS; i++) {
> +size_t cleaned;
> +
>  if (count > clean_end) {
>  next_wakeup = 0;
>  break;
>  }
>
> -count += ct_sweep(ct, &ct->exp_lists[i], now);
> +count += ct_sweep(ct, &ct->exp_lists[i], now, &cleaned);
> +total_cleaned += cleaned;
>  }
>
>  ct->next_sweep = (i < N_EXP_LISTS) ? i : 0;
>
> -VLOG_DBG("conntrack cleanup %"PRIuSIZE" entries in %lld msec", count,
> +VLOG_DBG("conntrack cleaned %"PRIuSIZE" / %"PRIuSIZE
> + " entries in %lld msec", total_cleaned, count,
>   time_msec() - now);

Can we make the log message a bit more clear? Maybe something like:

“conntrack cleaned %"PRIuSIZE" entries out of %"PRIuSIZE" in %lld msec”

>
>  return next_wakeup;
> -- 
> 2.45.2
>
> ___
> 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