Re: [ovs-dev] [PATCH 3/3] netdev-dpdk: Fix tunnel type check during Tx offload preparation.

[Ilya Maximets]

On 3/14/24 15:17, Mike Pattrick wrote:
> On Wed, Mar 13, 2024 at 1:29 PM Ilya Maximets  wrote:
>>
>> Tunnel types are not flags, but 4-bit fields, so checking them with
>> a simple binary 'and' is incorrect and may produce false-positive
>> matches.
>>
>> While the current implementation is unlikely to cause any issues today,
>> since both RTE_MBUF_F_TX_TUNNEL_VXLAN and RTE_MBUF_F_TX_TUNNEL_GENEVE
>> only have 1 bit set, it is risky to have this code and it may lead
>> to problems if we add support for other tunnel types in the future.
>>
>> Use proper field checks instead.  Also adding a warning for unexpected
>> tunnel types in case something goes wrong.
>>
>> Fixes: 084c8087292c ("userspace: Support VXLAN and GENEVE TSO.")
>> Signed-off-by: Ilya Maximets 
> 
> Acked-by: Mike Pattrick 
> 

Thanks, Mike!  I applied the set and backported to 3.3.

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

Re: [ovs-dev] [PATCH v2] ovs-tcpdump: Fix cleanup mirror failed with twice fatal signals.

[Ilya Maximets]

On 2/23/24 04:37, Daniel Ding wrote:
> After running ovs-tcpdump and inputs multiple CTRL+C, the program will
> raise the following exception.
> 
> Error in atexit._run_exitfuncs:
> Traceback (most recent call last):
>   File "/usr/bin/ovs-tcpdump", line 421, in cleanup_mirror
> ovsdb = OVSDB(db_sock)
>   File "/usr/bin/ovs-tcpdump", line 168, in __init__
> OVSDB.wait_for_db_change(self._idl_conn)  # Initial Sync with DB
>   File "/usr/bin/ovs-tcpdump", line 155, in wait_for_db_change
> while idl.change_seqno == seq and not idl.run():
> 
> The default handler of SIGINT is default_int_handler, so it not
> register the signal handler successfully. When received CTRL+C again,
> the program be break, and calling hook can't execute completely.
> 
> Signed-off-by: Daniel Ding 
> ---
>  python/ovs/fatal_signal.py | 24 +---
>  utilities/ovs-tcpdump.in   | 38 +-
>  2 files changed, 30 insertions(+), 32 deletions(-)

Sorry for the late response.  Thanks for v2!

I did some testing and it seem to fix the problem, though in case the ovsdb
connection is no longer available for some reason, we would block forever in
the handler waiting for the server to reply while the signals are blocked.
So, it's getting harder to kill the ovs-tcpdump in this situation.  But I
don't really have a solution for this.  We will have either this problem or
lingering ports and mirrors as long as we're trying to communicate with the
external process in the signal handler.

I think, it's an OK trade-off to make.  Double Ctrl+C seems like a more
frequent event than paused or stuck ovsdb-server, so I think it's OK to have
this patch.

See a few minor comments below.

Best regards, Ilya Maximets.

> 
> diff --git a/python/ovs/fatal_signal.py b/python/ovs/fatal_signal.py
> index cb2e99e87..077f50dd5 100644
> --- a/python/ovs/fatal_signal.py
> +++ b/python/ovs/fatal_signal.py
> @@ -16,6 +16,7 @@ import atexit
>  import os
>  import signal
>  import sys
> +import threading
>  
>  import ovs.vlog
>  
> @@ -112,29 +113,29 @@ def _unlink(file_):
>  def _signal_handler(signr, _):
>  _call_hooks(signr)
>  
> -# Re-raise the signal with the default handling so that the program
> -# termination status reflects that we were killed by this signal.
> -signal.signal(signr, signal.SIG_DFL)
> -os.kill(os.getpid(), signr)
> -
>  
>  def _atexit_handler():
>  _call_hooks(0)
>  
>  
> -recurse = False
> +mutex = threading.Lock()
>  
>  
>  def _call_hooks(signr):
> -global recurse
> -if recurse:
> +global mutex
> +if not mutex.acquire(blocking=False):
>  return
> -recurse = True
>  
>  for hook, cancel, run_at_exit in _hooks:
>  if signr != 0 or run_at_exit:
>  hook()
>  
> +if signr != 0:
> +   # Re-raise the signal with the default handling so that the program
> +   # termination status reflects that we were killed by this signal.
> +   signal.signal(signr, signal.SIG_DFL)
> +   os.kill(os.getpid(), signr)

The indentation here is incorrect:

python/ovs/fatal_signal.py:134:8: E114 indentation is not a multiple of 4 
(comment)
python/ovs/fatal_signal.py:135:8: E114 indentation is not a multiple of 4 
(comment)
python/ovs/fatal_signal.py:136:8: E111 indentation is not a multiple of 4
python/ovs/fatal_signal.py:137:8: E111 indentation is not a multiple of 4

> +
>  
>  _inited = False
>  
> @@ -150,7 +151,9 @@ def _init():
> signal.SIGALRM]
>  
>  for signr in signals:
> -if signal.getsignal(signr) == signal.SIG_DFL:
> +handler = signal.getsignal(signr)
> +if (handler == signal.SIG_DFL or
> +handler == signal.default_int_handler):
>  signal.signal(signr, _signal_handler)
>  atexit.register(_atexit_handler)
>  
> @@ -165,7 +168,6 @@ def signal_alarm(timeout):
>  
>  if sys.platform == "win32":
>  import time
> -import threading
>  
>  class Alarm (threading.Thread):
>  def __init__(self, timeout):
> diff --git a/utilities/ovs-tcpdump.in b/utilities/ovs-tcpdump.in
> index 4cbd9a5d3..0731b4ac8 100755
> --- a/utilities/ovs-tcpdump.in
> +++ b/utilities/ovs-tcpdump.in
> @@ -534,29 +534,25 @@ def main():
>  ovsdb.close_idl()
>  
>  pipes = _doexec(*([dump_cmd, '-i', mirror_interface] + tcpdargs))
> +while pipes.poll() is None:
> +data = pipes.stdout.readline().strip(b'\n')
> +if len(data) == 0:
> +break
> +print(data.decode('utf-8'))
> +
> +# If there is a pipe behind ovs-tcpdump (such as ovs-tcpdump
> +# -i eth0 | grep "192.168.1.1"), the pipe is no longer available
> +# after received Ctrl+C.
> +# If we write data to an unavailable pipe, a pipe error will be
> +# reported, so we turn off stdout to avoid subsequent flushing
> +# of data into the pipe.

I'm not sure if we need this comment.
The 

Re: [ovs-dev] [PATCH] ofproto-dpif-upcall: Don't mirror packets that aren't modified.

[Ilya Maximets]

On 3/12/24 18:37, Mike Pattrick wrote:
> Previously OVS reset the mirror contents when a packet is modified in
> such a way that the packets contents changes. However, this change
> incorrectly reset that mirror context when only metadata changes as
> well.
> 
> Now we check for all metadata fields, instead of just tunnel metadata,
> before resetting the mirror context.
> 
> Fixes: feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are modified.")
> Reported-by: Zhangweiwei 
> Signed-off-by: Mike Pattrick 
> ---
>  include/openvswitch/meta-flow.h |   1 +
>  lib/meta-flow.c | 109 
>  ofproto/ofproto-dpif-xlate.c|   2 +-
>  tests/ofproto-dpif.at   |   5 +-
>  4 files changed, 114 insertions(+), 3 deletions(-)

Thanks, Mike!  The change makes sense to me.
See some comments inline.

Best regards, Ilya Maixmets.

> 
> diff --git a/include/openvswitch/meta-flow.h b/include/openvswitch/meta-flow.h
> index 3b0220aaa..96aad3933 100644
> --- a/include/openvswitch/meta-flow.h
> +++ b/include/openvswitch/meta-flow.h
> @@ -2305,6 +2305,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 *);

Nit: maybe call it mf_is_any_metadata ?

>  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 aa7cf1fcb..7ecec334e 100644
> --- a/lib/meta-flow.c
> +++ b/lib/meta-flow.c
> @@ -1788,6 +1788,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_TUN_METADATA:
> +case MFF_METADATA:
> +case MFF_IN_PORT:
> +case MFF_IN_PORT_OXM:
> +CASE_MFF_REGS:
> +CASE_MFF_XREGS:
> +CASE_MFF_XXREGS:
> +case MFF_PACKET_TYPE:
> +case MFF_DP_HASH:
> +case MFF_RECIRC_ID:
> +case MFF_CONJ_ID:
> +case MFF_ACTSET_OUTPUT:
> +case MFF_SKB_PRIORITY:
> +case MFF_PKT_MARK:
> +case MFF_CT_STATE:
> +case MFF_CT_ZONE:
> +case MFF_CT_MARK:
> +case MFF_CT_LABEL:
> +case MFF_CT_NW_PROTO:
> +case MFF_CT_NW_SRC:
> +case MFF_CT_NW_DST:
> +case MFF_CT_IPV6_SRC:
> +case MFF_CT_IPV6_DST:
> +case MFF_CT_TP_SRC:
> +case MFF_CT_TP_DST:
> +case MFF_N_IDS:

The N_IDS should not be here.  It probably goes to the 'default' case.

> +return true;
> +
> +case MFF_TUN_ID:
> +case MFF_TUN_SRC:
> +case MFF_TUN_DST:
> +case MFF_TUN_IPV6_SRC:
> +case MFF_TUN_IPV6_DST:
> +case MFF_TUN_FLAGS:
> +case MFF_TUN_GBP_ID:
> +case MFF_TUN_GBP_FLAGS:
> +case MFF_TUN_ERSPAN_VER:
> +case MFF_TUN_ERSPAN_IDX:
> +case MFF_TUN_ERSPAN_DIR:
> +case MFF_TUN_ERSPAN_HWID:
> +case MFF_TUN_GTPU_FLAGS:
> +case MFF_TUN_GTPU_MSGTYPE:
> +case MFF_TUN_TTL:
> +case MFF_TUN_TOS:
> +case MFF_ETH_SRC:
> +case MFF_ETH_DST:
> +case MFF_ETH_TYPE:
> +case MFF_VLAN_TCI:
> +case MFF_DL_VLAN:
> +case MFF_VLAN_VID:
> +case MFF_DL_VLAN_PCP:
> +case MFF_VLAN_PCP:
> +case MFF_MPLS_LABEL:
> +case MFF_MPLS_TC:
> +case MFF_MPLS_BOS:
> +case MFF_MPLS_TTL:
> +case MFF_IPV4_SRC:
> +case MFF_IPV4_DST:
> +case MFF_IPV6_SRC:
> +case MFF_IPV6_DST:
> +case MFF_IPV6_LABEL:
> +case MFF_IP_PROTO:
> +case MFF_IP_DSCP:
> +case MFF_IP_DSCP_SHIFTED:
> +case MFF_IP_ECN:
> +case MFF_IP_TTL:
> +case MFF_IP_FRAG:
> +case MFF_ARP_OP:
> +case MFF_ARP_SPA:
> +case MFF_ARP_TPA:
> +case MFF_ARP_SHA:
> +case MFF_ARP_THA:
> +case MFF_TCP_SRC:
> +case MFF_TCP_DST:
> +case MFF_TCP_FLAGS:
> +case MFF_UDP_SRC:
> +case MFF_UDP_DST:
> +case MFF_SCTP_SRC:
> +case MFF_SCTP_DST:
> +case MFF_ICMPV4_TYPE:
> +case MFF_ICMPV4_CODE:
> +case MFF_ICMPV6_TYPE:
> +case MFF_ICMPV6_CODE:
> +case MFF_ND_TARGET:
> +case MFF_ND_SLL:
> +case MFF_ND_TLL:
> +case MFF_ND_RESERVED:
> +case MFF_ND_OPTIONS_TYPE:
> +case MFF_NSH_FLAGS:
> +case MFF_NSH_TTL:
> +case MFF_NSH_MDTYPE:
> +case MFF_NSH_NP:
> +case MFF_NSH_SPI:
> +case MFF_NSH_SI:
> +case MFF_NSH_C1:
> +case MFF_NSH_C2:
> +case MFF_NSH_C3:
> +case MFF_NSH_C4:
> +return false;

In general, functions like this are trying to maintain a specific order
of the cases.  It's either alphabetical or the order in which they are
defined in the enum.  I think, most of the mf functions are trying to
follwo the enum order.  Order in this fucntion doesn't seem to match any
other function.  Could you re-order the cases in the enum order, 

Re: [ovs-dev] [PATCH] ofproto: Fix mod flow table name not to take effect.

[Ilya Maximets]

On 3/16/24 01:44, Ilya Maximets wrote:
> On 3/8/24 09:32, Yuhao zhou via dev wrote:
>> From: "zhouyuhao.philozhou" 
>>
>> When mod a flow table's name with table's prefix name, there
>> will be no change. Because when check whether the new and old
>> name are the same, only compare the length of the new name.
>>
>> Case:
>>   table 10: "good"
>>   There will be no change if mod the table's name with "g" "go" "goo".
>>
>> Signed-off-by: zhouyuhao.philozhou 
>> ---

Also, please, add a version number to the patch subject while sending
new versions, e.g. [PATCH v3] ofproto: ...

And add a small description on what changed between versions here, after
the '---', but before the diff.

Best regards, Ilya Maximets.

>>  ofproto/ofproto.c |  4 +++-
>>  tests/ofproto.at  | 12 
>>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> Good catch!
> 
>>
>> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
>> index 122a06f30..bf7ed91b1 100644
>> --- a/ofproto/ofproto.c
>> +++ b/ofproto/ofproto.c
>> @@ -9293,7 +9293,9 @@ oftable_set_name(struct oftable *table, const char 
>> *name, int level)
>>  if (level >= table->name_level) {
>>  if (name) {
>>  if (name[0]) {
>> -if (!table->name || strncmp(name, table->name, len)) {
>> +if (!table->name
>> +|| strncmp(name, table->name, len)
>> +|| len != strlen(table->name)) {
> 
> Maybe we can just use OFP_MAX_TABLE_NAME_LEN in the strncmp call?
> Will it produce the same result?
> 
> Also, there is an oftable_may_set_name() function just below, it
> will need the same change.
> 
>>  free(table->name);
>>  table->name = xmemdup0(name, len);
>>  }
>> diff --git a/tests/ofproto.at b/tests/ofproto.at
>> index 2889f81fb..09c57b292 100644
>> --- a/tests/ofproto.at
>> +++ b/tests/ofproto.at
>> @@ -2523,6 +2523,18 @@ AT_CHECK([ovs-ofctl -O OpenFlow15 dump-table-features 
>> br0 |grep '^  table'],
>>table 253:
>>  ])
>>  
>> +# Make sure that the new name is old table's name prefix can also take 
>> effect.
> 
> s/is old table's name/equal to the old name's/
> 
> Best regards, Ilya Maximets.

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

Re: [ovs-dev] [PATCH] ofproto: Fix mod flow table name not to take effect.

[Ilya Maximets]

On 3/8/24 09:32, Yuhao zhou via dev wrote:
> From: "zhouyuhao.philozhou" 
> 
> When mod a flow table's name with table's prefix name, there
> will be no change. Because when check whether the new and old
> name are the same, only compare the length of the new name.
> 
> Case:
>   table 10: "good"
>   There will be no change if mod the table's name with "g" "go" "goo".
> 
> Signed-off-by: zhouyuhao.philozhou 
> ---
>  ofproto/ofproto.c |  4 +++-
>  tests/ofproto.at  | 12 
>  2 files changed, 15 insertions(+), 1 deletion(-)

Good catch!

> 
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 122a06f30..bf7ed91b1 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -9293,7 +9293,9 @@ oftable_set_name(struct oftable *table, const char 
> *name, int level)
>  if (level >= table->name_level) {
>  if (name) {
>  if (name[0]) {
> -if (!table->name || strncmp(name, table->name, len)) {
> +if (!table->name
> +|| strncmp(name, table->name, len)
> +|| len != strlen(table->name)) {

Maybe we can just use OFP_MAX_TABLE_NAME_LEN in the strncmp call?
Will it produce the same result?

Also, there is an oftable_may_set_name() function just below, it
will need the same change.

>  free(table->name);
>  table->name = xmemdup0(name, len);
>  }
> diff --git a/tests/ofproto.at b/tests/ofproto.at
> index 2889f81fb..09c57b292 100644
> --- a/tests/ofproto.at
> +++ b/tests/ofproto.at
> @@ -2523,6 +2523,18 @@ AT_CHECK([ovs-ofctl -O OpenFlow15 dump-table-features 
> br0 |grep '^  table'],
>table 253:
>  ])
>  
> +# Make sure that the new name is old table's name prefix can also take 
> effect.

s/is old table's name/equal to the old name's/

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

Re: [ovs-dev] [PATCH] ofproto-dpif-xlate: Fix continuations with associated metering.

[Ilya Maximets]

On 3/7/24 18:25, Aaron Conole wrote:
> Open vSwitch supports the ability to invoke a controller action by way
> of a sample action with a specified meter.  In the normal case, this
> sample action is transparently generated during xlate processing.  However,
> when executing via a continuation, the logic to generate the sample
> action when finishing the context freeze was missing.  The result is that
> the behavior when action is 'controller(pause,meter_id=1)' does not match
> the behavior when action is 'controller(meter_id=1)'.
> 
> OVN and other controller solutions may rely on this metering to protect
> the control path, so it is critical to preserve metering, whether we are
> doing a plain old send to controller, or a continuation.
> 
> Fixes: 77ab5fd2a95b ("Implement serializing the state of packet traversal in 
> "continuations".")
> Reported-at: https://issues.redhat.com/browse/FDP-455
> Tested-by: Alex Musil 
> Signed-off-by: Aaron Conole 
> ---
>  ofproto/ofproto-dpif-xlate.c | 66 +++-
>  tests/ofproto-dpif.at| 50 +++
>  2 files changed, 84 insertions(+), 32 deletions(-)

Thanks, Aaron!  The change seems corrct to me.
See a couple of comments inline.

Best regrads, Ilya Maximets.

> 
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 89f183182e..4f1e57e6d1 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -5080,10 +5080,37 @@ put_controller_user_action(struct xlate_ctx *ctx,
> bool dont_send, bool continuation,
> uint32_t recirc_id, int len,
> enum ofp_packet_in_reason reason,
> +   uint32_t provider_meter_id,
> uint16_t controller_id)
>  {
>  struct user_action_cookie cookie;
>  
> +/* If the controller action didn't request a meter (indicated by a
> + * 'meter_id' argument other than NX_CTLR_NO_METER), see if one was
> + * configured through the "controller" virtual meter.
> + *
> + * Internally, ovs-vswitchd uses UINT32_MAX to indicate no meter is
> + * configured. */
> +uint32_t meter_id;
> +if (provider_meter_id == UINT32_MAX) {
> +meter_id = ctx->xbridge->ofproto->up.controller_meter_id;
> +} else {
> +meter_id = provider_meter_id;
> +}
> +
> +size_t offset;
> +size_t ac_offset;
> +if (meter_id != UINT32_MAX) {
> +/* If controller meter is configured, generate clone(meter,
> + * userspace) action. */

Might be better to keep the comment the way it were originally.
I find it harder to read the action split in two lines.

> +offset = nl_msg_start_nested(ctx->odp_actions, 
> OVS_ACTION_ATTR_SAMPLE);
> +nl_msg_put_u32(ctx->odp_actions, OVS_SAMPLE_ATTR_PROBABILITY,
> +   UINT32_MAX);
> +ac_offset = nl_msg_start_nested(ctx->odp_actions,
> +OVS_SAMPLE_ATTR_ACTIONS);
> +nl_msg_put_u32(ctx->odp_actions, OVS_ACTION_ATTR_METER, meter_id);
> +}
> +
>  memset(&cookie, 0, sizeof cookie);
>  cookie.type = USER_ACTION_COOKIE_CONTROLLER;
>  cookie.ofp_in_port = OFPP_NONE,
> @@ -5101,6 +5128,11 @@ put_controller_user_action(struct xlate_ctx *ctx,
>  uint32_t pid = dpif_port_get_pid(ctx->xbridge->dpif, odp_port);
>  odp_put_userspace_action(pid, &cookie, sizeof cookie, ODPP_NONE,
>   false, ctx->odp_actions, NULL);
> +
> +if (meter_id != UINT32_MAX) {
> +nl_msg_end_nested(ctx->odp_actions, ac_offset);
> +nl_msg_end_nested(ctx->odp_actions, offset);
> +}
>  }
>  
>  static void
> @@ -5145,32 +5177,6 @@ xlate_controller_action(struct xlate_ctx *ctx, int len,
>  }
>  recirc_refs_add(&ctx->xout->recircs, recirc_id);
>  
> -/* If the controller action didn't request a meter (indicated by a
> - * 'meter_id' argument other than NX_CTLR_NO_METER), see if one was
> - * configured through the "controller" virtual meter.
> - *
> - * Internally, ovs-vswitchd uses UINT32_MAX to indicate no meter is
> - * configured. */
> -uint32_t meter_id;
> -if (provider_meter_id == UINT32_MAX) {
> -meter_id = ctx->xbridge->ofproto->up.controller_meter_id;
> -} else {
> -meter_id = provider_meter_id;
> -}
> -
> -size_t offset;
> -size_t ac_offset;
> -if (meter_id != UINT32_MAX) {
> -/* If controller meter is configured, generate clone(meter, 
> userspace)
> - * action. */
> -offset = nl_msg_start_nested(ctx->odp_actions, 
> OVS_ACTION_ATTR_SAMPLE);
> -nl_msg_put_u32(ctx->odp_actions, OVS_SAMPLE_ATTR_PROBABILITY,
> -   UINT32_MAX);
> -ac_offset = nl_msg_start_nested(ctx->odp_actions,
> -OVS_SAMPLE_ATTR_ACTIONS);
> -nl_msg_put_u32(ctx->odp_act

[ovs-dev] [PATCH 4/5] ovsdb: raft: Fix assertion when 1-node cluster looses leadership.

[Ilya Maximets]

Some of the failure tests can make a single-node cluster to
loose leadership.  In this case the next raft_run() will
trigger election with a pre-vore enabled.  This is causing
an assertion when this server attempts to vote for itself.

Fix that by not using pre-voting if the is only one server.

A new failure test introduced in later commit triggers this
assertion every time.

Fixes: 85634fd58004 ("ovsdb: raft: Support pre-vote mechanism to deal with 
disruptive server.")
Signed-off-by: Ilya Maximets 
---
 ovsdb/raft.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ovsdb/raft.c b/ovsdb/raft.c
index 237d7ebf5..c41419052 100644
--- a/ovsdb/raft.c
+++ b/ovsdb/raft.c
@@ -2083,7 +2083,7 @@ raft_run(struct raft *raft)
 raft_start_election(raft, true, false);
 }
 } else {
-raft_start_election(raft, true, false);
+raft_start_election(raft, hmap_count(&raft->servers) > 1, false);
 }
 
 }
-- 
2.43.0

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

[ovs-dev] [PATCH 5/5] ovsdb: raft: Fix inability to join after leadership change round trip.

[Ilya Maximets]

Consider the following sequence of events:

 1. Cluster with 2 nodes - A and B.  A is a leader.
 2. C connects to A and sends a join request.
 3. A sends an append request to C.  C is in CATCHUP phase for A.
 4. A looses leadership to B.  Sends join failure notification to C.
 5. C sends append reply to A.
 6. A discards append reply (not leader).
 7. B looses leadership back to A.
 8. C sends a new join request to A.
 9. A replies with failure (already in progress).
 10. GoTo step 8.

At this point A is waiting for an append reply that it already
discarded at step 6 and fails all the new attempts of C to join with
'already in progress' verdict.  C stays forever in a joining state
and in a CATCHUP phase from A's perspective.

This is a similar case to a sudden disconnect from a leader fixed in
commit 999ba294fb4f ("ovsdb: raft: Fix inability to join the cluster
after interrupted attempt."), but since we're not disconnecting, the
servers are not getting destroyed.

Fix that by destroying all the servers that are not yet part of the
configuration after leadership is lost.  This way, server C will be
able to simply re-start the joining process from scratch.

New failure test command is added in order to simulate leadership
change before we receive the append reply, so it gets discarded.
New cluster test is added to exercise this scenario.

Fixes: 1b1d2e6daa56 ("ovsdb: Introduce experimental support for clustered 
databases.")
Reported-at: https://github.com/ovn-org/ovn/issues/235
Signed-off-by: Ilya Maximets 
---
 ovsdb/raft.c   | 16 -
 tests/ovsdb-cluster.at | 53 ++
 2 files changed, 68 insertions(+), 1 deletion(-)

diff --git a/ovsdb/raft.c b/ovsdb/raft.c
index c41419052..f9e760a08 100644
--- a/ovsdb/raft.c
+++ b/ovsdb/raft.c
@@ -81,6 +81,7 @@ enum raft_failure_test {
 FT_STOP_RAFT_RPC,
 FT_TRANSFER_LEADERSHIP,
 FT_TRANSFER_LEADERSHIP_AFTER_SEND_APPEND_REQ,
+FT_TRANSFER_LEADERSHIP_AFTER_STARTING_TO_ADD,
 };
 static enum raft_failure_test failure_test;
 
@@ -2702,15 +2703,22 @@ raft_become_follower(struct raft *raft)
  * new configuration.  Our AppendEntries processing will properly update
  * the server configuration later, if necessary.
  *
+ * However, since we're sending replies about a failure to add, those new
+ * servers has to be cleaned up.  Otherwise, they will stuck in a 'CATCHUP'
+ * phase in case this server regains leadership before they join through
+ * the current new leader.  They are not yet in 'raft->servers', so not
+ * part of the shared configuration.
+ *
  * Also we do not complete commands here, as they can still be completed
  * if their log entries have already been replicated to other servers.
  * If the entries were actually committed according to the new leader, our
  * AppendEntries processing will complete the corresponding commands.
  */
 struct raft_server *s;
-HMAP_FOR_EACH (s, hmap_node, &raft->add_servers) {
+HMAP_FOR_EACH_POP (s, hmap_node, &raft->add_servers) {
 raft_send_add_server_reply__(raft, &s->sid, s->address, false,
  RAFT_SERVER_LOST_LEADERSHIP);
+raft_server_destroy(s);
 }
 if (raft->remove_server) {
 raft_send_remove_server_reply__(raft, &raft->remove_server->sid,
@@ -3985,6 +3993,10 @@ raft_handle_add_server_request(struct raft *raft,
  "to cluster "CID_FMT, s->nickname, SID_ARGS(&s->sid),
  rq->address, CID_ARGS(&raft->cid));
 raft_send_append_request(raft, s, 0, "initialize new server");
+
+if (failure_test == FT_TRANSFER_LEADERSHIP_AFTER_STARTING_TO_ADD) {
+failure_test = FT_TRANSFER_LEADERSHIP;
+}
 }
 
 static void
@@ -5110,6 +5122,8 @@ raft_unixctl_failure_test(struct unixctl_conn *conn 
OVS_UNUSED,
 } else if (!strcmp(test,
"transfer-leadership-after-sending-append-request")) {
 failure_test = FT_TRANSFER_LEADERSHIP_AFTER_SEND_APPEND_REQ;
+} else if (!strcmp(test, "transfer-leadership-after-starting-to-add")) {
+failure_test = FT_TRANSFER_LEADERSHIP_AFTER_STARTING_TO_ADD;
 } else if (!strcmp(test, "transfer-leadership")) {
 failure_test = FT_TRANSFER_LEADERSHIP;
 } else if (!strcmp(test, "clear")) {
diff --git a/tests/ovsdb-cluster.at b/tests/ovsdb-cluster.at
index 482e4e02d..9d8b4d06a 100644
--- a/tests/ovsdb-cluster.at
+++ b/tests/ovsdb-cluster.at
@@ -525,6 +525,59 @@ for i in $(seq $n); do
 OVS_APP_EXIT_AND_WAIT_BY_TARGET([$(pwd)/s$i], [s$i.pid])
 done
 
+AT_CLEANUP
+
+AT_SETUP([OVSDB cluster - leadership change before replication while joining])
+AT_KEYWORDS([ovsdb server negative unix cluster join])
+
+n=5
+AT_CHECK([ovsdb-tool '-vPATTERN:console:%c|%p|%m' create-cluster s1.db dnl
+  $abs_srcdir/idltest.ovsschema unix:s1.raft], [0], [], [stderr])
+cid=$(ovsdb-tool db-cid s1.db)
+schema_name=$(ovsdb-tool schema-

[ovs-dev] [PATCH 3/5] ovsdb: raft: Fix permanent joining state on a cluster member.

[Ilya Maximets]

Consider the following chain of events:

 1. Have a cluster with 2 members - A and B.  A is a leader.
 2. C connects to A, sends a request to join the cluster.
 3. A catches up C, creates an update for the 'servers' list and sends
it to B and C to apply.  This entry is not committed yet.
 4. Before B or C can reply, A looses leadership for some reason.
 5. A sends a joining failure message to C, C remains in joining state.
 5. Both B and C have the new version of 'servers', so they recognize
each other as valid cluster members.
 6. B initiates a vote, C (or A) replies and B becomes a new leader.
 7. B has a new list of servers.  B commits it.  C becomes a committed
cluster member.
 8. A and C receive heartbeats with a new commit index and C is now
a committed cluster member for all A, B and C.

However, at the end of this process, C is still in joining state
as it never received a successful reply for a join request, and C is
still in a COMMITTING phase for A.  So, C skips some parts of the RAFT
life cycle and A will refuse to transfer leadership to C if something
happens in the future.

More interestingly, B can actually transfer leadership to C and vote
for it.  A will vote for it just fine as well.  After that, C becomes
a new cluster leader while still in joining state.  In this state C
will not commit any changes.  So, we have seemingly stable cluster
that doesn't commit any changes!  E.g.:

  s3
  Address: unix:s3.raft
  Status: joining cluster
  Remotes for joining: unix:s3.raft unix:s2.raft unix:s1.raft
  Role: leader
  Term: 4
  Leader: self
  Vote: self

  Last Election started 30095 ms ago, reason: leadership_transfer
  Last Election won: 30093 ms ago
  Election timer: 1000
  Log: [2, 7]
  Entries not yet committed: 2
  Entries not yet applied: 6
  Connections: ->s1 ->s2 <-s1 <-s2
  Disconnections: 0
  Servers:
s3 (60cf at unix:s3.raft) (self) next_index=7 match_index=6
s2 (46aa at unix:s2.raft) next_index=7 match_index=6 last msg 58 ms ago
s1 (28f7 at unix:s1.raft) next_index=7 match_index=6 last msg 59 ms ago

Fix the first scenario by examining server changes in committed log
entries.  This way server A can transition C to a STABLE phase and
server C can find itself in the committed list of servers and move out
from a joining state.  This is similar to completing commands without
receiving an explicit reply or after the role change from leader to
follower.

The second scenario with a leader in a joining state can be fixed
when the joining server becomes leader.  New leader's log is getting
committed automatically and all servers transition into STABLE phase
for it, but it should also move on from a joining state, since it
leads the cluster now.
It is also possible that B transfers leadership to C before the list
of servers is marked as committed on other servers.  In this case
C will commit it's own addition to the cluster configuration.

The added test usually triggers both scenarios, but it will trigger at
least one of them.

Fixes: 1b1d2e6daa56 ("ovsdb: Introduce experimental support for clustered 
databases.")
Signed-off-by: Ilya Maximets 
---
 ovsdb/raft.c   | 44 ++-
 tests/ovsdb-cluster.at | 53 ++
 2 files changed, 96 insertions(+), 1 deletion(-)

diff --git a/ovsdb/raft.c b/ovsdb/raft.c
index 57e27bf73..237d7ebf5 100644
--- a/ovsdb/raft.c
+++ b/ovsdb/raft.c
@@ -385,6 +385,7 @@ static void raft_get_servers_from_log(struct raft *, enum 
vlog_level);
 static void raft_get_election_timer_from_log(struct raft *);
 
 static bool raft_handle_write_error(struct raft *, struct ovsdb_error *);
+static bool raft_has_uncommitted_configuration(const struct raft *);
 
 static void raft_run_reconfigure(struct raft *);
 
@@ -2810,6 +2811,18 @@ raft_become_leader(struct raft *raft)
 raft_reset_election_timer(raft);
 raft_reset_ping_timer(raft);
 
+if (raft->joining) {
+/* It is possible that the server committing this one to the list of
+ * servers lost leadership before the entry is committed but after
+ * it was already replicated to majority of servers.  In this case
+ * other servers will recognize this one as a valid cluster member
+ * and may transfer leadership to it and vote for it.  This way
+ * we're becoming a cluster leader without receiving reply for a
+ * join request and will commit addition of this server ourselves. */
+VLOG_INFO_RL(&rl, "elected as leader while joining");
+raft->joining = false;
+}
+
 struct raft_server *s;
 HMAP_FOR_EACH (s, hmap_node, &raft->servers) {
 raft_server_init_leader(raft, s);
@@ -2968,12 +2981,12 @@ raft_update_commit_index(struct raft *raft, uint64_t 
new_commit_index)
 }
 
 while (raft->commit_index < new_commit_index) {
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
 uint64_t index = ++raft->commit_index;

[ovs-dev] [PATCH 0/5] ovsdb: raft: Fixes for cluster joining state.

[Ilya Maximets]

Issues discovered while working on:
  https://github.com/ovn-org/ovn/issues/235

Ilya Maximets (5):
  ovsdb: raft: Randomize leadership transfer.
  ovsdb: raft: Fix time intervals for multitasking while joining.
  ovsdb: raft: Fix permanent joining state on a cluster member.
  ovsdb: raft: Fix assertion when 1-node cluster looses leadership.
  ovsdb: raft: Fix inability to join after leadership change round trip.

 ovsdb/raft.c   |  73 +---
 tests/ovsdb-cluster.at | 106 +
 2 files changed, 173 insertions(+), 6 deletions(-)

-- 
2.43.0

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

[ovs-dev] [PATCH 2/5] ovsdb: raft: Fix time intervals for multitasking while joining.

[Ilya Maximets]

While joining, ovsdb-server may not wake up for a duration of a join
timer, which is 1 second and is by default 3x larger than a heartbeat
timer.  This is causing unnecessary warnings from the cooperative
multitasking module that thinks that we missed the heartbeat time by
a lot.

Use join timer (1000) instead while joining.

Fixes: d4a15647b917 ("ovsdb: raft: Enable cooperative multitasking.")
Signed-off-by: Ilya Maximets 
---

CC: Frode Nordahl 

 ovsdb/raft.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/ovsdb/raft.c b/ovsdb/raft.c
index 25f462431..57e27bf73 100644
--- a/ovsdb/raft.c
+++ b/ovsdb/raft.c
@@ -2126,10 +2126,11 @@ raft_run(struct raft *raft)
 raft_reset_ping_timer(raft);
 }
 
+uint64_t interval = raft->joining
+? 1000 : RAFT_TIMER_THRESHOLD(raft->election_timer);
 cooperative_multitasking_set(
 &raft_run_cb, (void *) raft, time_msec(),
-RAFT_TIMER_THRESHOLD(raft->election_timer)
-+ RAFT_TIMER_THRESHOLD(raft->election_timer) / 10, "raft_run");
+interval + interval / 10, "raft_run");
 
 /* Do this only at the end; if we did it as soon as we set raft->left or
  * raft->failed in handling the RemoveServerReply, then it could easily
-- 
2.43.0

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

[ovs-dev] [PATCH 1/5] ovsdb: raft: Randomize leadership transfer.

[Ilya Maximets]

Each cluster member typically always transfers leadership to the same
other member, which is the first in their list of servers.  This may
result in two servers in a 3-node cluster to transfer leadership to
each other and never to the third one.

Randomizing the selection to make the load more evenly distributed.

This also makes cluster failure tests cover more scenarios as servers
will transfer leadership to servers they didn't before.  This is
important especially for cluster joining tests.

Ideally, we would transfer to a random server with a highest apply
index, but not trying to implement this for now.

Signed-off-by: Ilya Maximets 
---
 ovsdb/raft.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/ovsdb/raft.c b/ovsdb/raft.c
index f463afcb3..25f462431 100644
--- a/ovsdb/raft.c
+++ b/ovsdb/raft.c
@@ -1261,8 +1261,12 @@ raft_transfer_leadership(struct raft *raft, const char 
*reason)
 return;
 }
 
+size_t n = hmap_count(&raft->servers) * 3;
 struct raft_server *s;
-HMAP_FOR_EACH (s, hmap_node, &raft->servers) {
+
+while (n--) {
+s = CONTAINER_OF(hmap_random_node(&raft->servers),
+ struct raft_server, hmap_node);
 if (!uuid_equals(&raft->sid, &s->sid)
 && s->phase == RAFT_PHASE_STABLE) {
 struct raft_conn *conn = raft_find_conn_by_sid(raft, &s->sid);
-- 
2.43.0

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

[ovs-dev] [PATCH v12 3/6] dpif: Support atomic_bool field type.

[Eric Garver]

The next commit will convert a dp feature from bool to atomic_bool. As
such we have to add support to the macros and functions. We must pass by
reference instead of pass by value because all the atomic operations
require a reference.

Acked-by: Eelco Chaudron 
Signed-off-by: Eric Garver 
---
 ofproto/ofproto-dpif.c | 54 --
 1 file changed, 47 insertions(+), 7 deletions(-)

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index d732198de5ea..2ec1cd1854ab 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -717,6 +717,8 @@ close_dpif_backer(struct dpif_backer *backer, bool del)
 }
 
 static void check_support(struct dpif_backer *backer);
+static void copy_support(struct dpif_backer_support *dst,
+ struct dpif_backer_support *src);
 
 static int
 open_dpif_backer(const char *type, struct dpif_backer **backerp)
@@ -837,7 +839,7 @@ open_dpif_backer(const char *type, struct dpif_backer 
**backerp)
  * 'boottime_support' can be checked to prevent 'support' to be changed
  * beyond the datapath capabilities. In case 'support' is changed by
  * the user, 'boottime_support' can be used to restore it.  */
-backer->bt_support = backer->rt_support;
+copy_support(&backer->bt_support, &backer->rt_support);
 
 return error;
 }
@@ -1611,6 +1613,24 @@ CHECK_FEATURE__(ct_orig_tuple6, ct_orig_tuple6, 
ct_nw_proto, 1, ETH_TYPE_IPV6)
 #undef CHECK_FEATURE
 #undef CHECK_FEATURE__
 
+static void
+copy_support(struct dpif_backer_support *dst, struct dpif_backer_support *src)
+{
+#define DPIF_SUPPORT_FIELD(TYPE, NAME, TITLE) \
+if (!strcmp(#TYPE, "atomic_bool")) { \
+bool value; \
+atomic_read_relaxed((atomic_bool *) &src->NAME, &value); \
+atomic_store_relaxed((atomic_bool *) &dst->NAME, value); \
+} else { \
+dst->NAME = src->NAME; \
+}
+
+DPIF_SUPPORT_FIELDS
+#undef DPIF_SUPPORT_FIELD
+
+dst->odp = src->odp;
+}
+
 static void
 check_support(struct dpif_backer *backer)
 {
@@ -6248,20 +6268,30 @@ ofproto_unixctl_dpif_dump_dps(struct unixctl_conn 
*conn, int argc OVS_UNUSED,
 }
 
 static void
-show_dp_feature_bool(struct ds *ds, const char *feature, bool b)
+show_dp_feature_bool(struct ds *ds, const char *feature, const bool *b)
 {
-ds_put_format(ds, "%s: %s\n", feature, b ? "Yes" : "No");
+ds_put_format(ds, "%s: %s\n", feature, *b ? "Yes" : "No");
+}
+
+static void OVS_UNUSED
+show_dp_feature_atomic_bool(struct ds *ds, const char *feature,
+const atomic_bool *b)
+{
+bool value;
+atomic_read_relaxed((atomic_bool *) b, &value);
+ds_put_format(ds, "%s: %s\n", feature, value ? "Yes" : "No");
 }
 
 static void
-show_dp_feature_size_t(struct ds *ds, const char *feature, size_t s)
+show_dp_feature_size_t(struct ds *ds, const char *feature, const size_t *s)
 {
-ds_put_format(ds, "%s: %"PRIuSIZE"\n", feature, s);
+ds_put_format(ds, "%s: %"PRIuSIZE"\n", feature, *s);
 }
 
 enum dpif_support_field_type {
 DPIF_SUPPORT_FIELD_bool,
 DPIF_SUPPORT_FIELD_size_t,
+DPIF_SUPPORT_FIELD_atomic_bool,
 };
 
 struct dpif_support_field {
@@ -6278,12 +6308,12 @@ static void
 dpif_show_support(const struct dpif_backer_support *support, struct ds *ds)
 {
 #define DPIF_SUPPORT_FIELD(TYPE, NAME, TITLE) \
-show_dp_feature_##TYPE (ds, TITLE, support->NAME);
+show_dp_feature_##TYPE (ds, TITLE, &support->NAME);
 DPIF_SUPPORT_FIELDS
 #undef DPIF_SUPPORT_FIELD
 
 #define ODP_SUPPORT_FIELD(TYPE, NAME, TITLE) \
-show_dp_feature_##TYPE (ds, TITLE, support->odp.NAME );
+show_dp_feature_##TYPE (ds, TITLE, &support->odp.NAME );
 ODP_SUPPORT_FIELDS
 #undef ODP_SUPPORT_FIELD
 }
@@ -6302,6 +6332,16 @@ display_support_field(const char *name,
   b ? "true" : "false");
 break;
 }
+case DPIF_SUPPORT_FIELD_atomic_bool: {
+bool b, v;
+
+atomic_read_relaxed((atomic_bool *) field->rt_ptr, &v);
+atomic_read_relaxed((atomic_bool *) field->bt_ptr, &b);
+ds_put_format(ds, "%s (%s) : [run time]:%s, [boot time]:%s\n", name,
+  field->title, v ? "true" : "false",
+  b ? "true" : "false");
+break;
+}
 case DPIF_SUPPORT_FIELD_size_t:
 ds_put_format(ds, "%s (%s) : [run time]:%"PRIuSIZE
   ", [boot time]:%"PRIuSIZE"\n", name,
-- 
2.43.0

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

[ovs-dev] [PATCH v12 5/6] tests: system-traffic: Add coverage for drop action.

[Eric Garver]

Exercise the drop action in the datapath. This specific tests triggers
an xlate_error.

For the kernel datapath skb drop reasons can then be seen while this
test runs.

 # perf trace -e skb:kfree_skb --filter="reason >= 0x3"
   0.000 ping/1275884 skb:kfree_skb(skbaddr: 0x8acd76546000, \
  location: 0xc0ee3634, protocol: 2048, reason: 196611)

Acked-by: Eelco Chaudron 
Signed-off-by: Eric Garver 
---
 tests/system-common-macros.at |  4 
 tests/system-traffic.at   | 31 +++
 2 files changed, 35 insertions(+)

diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at
index 01ebe364ee7c..2a68cd664e5c 100644
--- a/tests/system-common-macros.at
+++ b/tests/system-common-macros.at
@@ -374,3 +374,7 @@ m4_define([OVS_CHECK_CT_CLEAR],
 # OVS_CHECK_GITHUB_ACTION
 m4_define([OVS_CHECK_GITHUB_ACTION],
 [AT_SKIP_IF([test "$GITHUB_ACTIONS" = "true"])])
+
+# OVS_CHECK_DROP_ACTION()
+m4_define([OVS_CHECK_DROP_ACTION],
+[AT_SKIP_IF([! grep -q "Datapath supports explicit drop action" 
ovs-vswitchd.log])])
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 2d12d558ec2f..2ddb4921c60e 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -2231,6 +2231,37 @@ masks-cache:size:256
 OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP
 
+AT_SETUP([datapath - drop action])
+OVS_TRAFFIC_VSWITCHD_START()
+OVS_CHECK_DROP_ACTION()
+AT_KEYWORDS(drop_action)
+
+ADD_NAMESPACES(at_ns0, at_ns1)
+
+ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
+ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
+
+dnl Exceed the max number of resubmits.
+(echo "dl_type=0x806, actions=normal"
+for i in `seq 1 64`; do
+ j=`expr $i + 1`
+ echo "in_port=$i, actions=resubmit:$j, resubmit:$j, local"
+ done
+ echo "in_port=65, actions=local"
+) > flows.txt
+AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
+
+dnl Generate some traffic.
+NS_CHECK_EXEC([at_ns0], [ping -q -c 10 -i 0.1 -w 2 10.1.1.2], [1], [ignore])
+
+OVS_WAIT_UNTIL_EQUAL([ovs-appctl dpctl/dump-flows | grep "eth_type(0x0800)" | 
dnl
+  sed 's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/;s/,eth(),/,/;' | dnl
+  strip_recirc | strip_stats | strip_used | sort], [dnl
+recirc_id(),in_port(2),eth_type(0x0800),ipv4(frag=no), packets:0, 
bytes:0, used:0.0s, actions:drop])
+
+OVS_TRAFFIC_VSWITCHD_STOP(["/|WARN|/d"])
+AT_CLEANUP
+
 AT_SETUP([datapath - simulated flow action update])
 OVS_TRAFFIC_VSWITCHD_START()
 
-- 
2.43.0

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

[ovs-dev] [PATCH v12 6/6] tests: system-offload-traffic: Verify re-probe of drop action.

[Eric Garver]

Verify that the explicit drop action is re-probed if the hw-offload flag
is changed.

Signed-off-by: Eric Garver 
---
 tests/system-offloads-traffic.at | 12 
 1 file changed, 12 insertions(+)

diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at
index 6bd49a3eef30..899a8e9bca2f 100644
--- a/tests/system-offloads-traffic.at
+++ b/tests/system-offloads-traffic.at
@@ -921,3 +921,15 @@ AT_CHECK([tc -d filter show dev ovs-p0 ingress | grep -q 
"csum (iph)"], [0])
 
 OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP
+
+AT_SETUP([offloads - re-probe drop action])
+OVS_TRAFFIC_VSWITCHD_START()
+OVS_CHECK_DROP_ACTION()
+AT_KEYWORDS(drop_action)
+
+dnl trigger a re-probe of the explicit drop action
+AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=true])
+OVS_WAIT_UNTIL([grep -q "Datapath does not support explicit drop action" 
ovs-vswitchd.log])
+
+OVS_TRAFFIC_VSWITCHD_STOP
+AT_CLEANUP
-- 
2.43.0

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

[ovs-dev] [PATCH v12 0/6] dpif: probe support for OVS_ACTION_ATTR_DROP

[Eric Garver]

v12:
  - new patch to verify re-probe
  - don't mention hw offload in the probe
  - changed log message to not mention HW offload
v11:
  - move netdev and flow API check to lib
  - tweaked log message
  - use atomic store instead of cmpx
  - copy odp in copy_support()
v10:
  - fix a sparse error in show_dp_feature_atomic_bool()
v9:
  - new patch make get_datapath_cap() access support by pointer
  - fix a clang warning in copy_support()
v8:
  - new patch to support atomic_bool dpif field types
  - re-add re-probe of support
  - use atomic_bool type for explicit_drop_action
v7:
  - remove re-probe of support as Ilya is working on a generic solution
v6:
  - improve log if hw-offload enabled
  - re-probe support if hw-offload set true at runtime
v5:
  - combine patches 3 and 4
  - add description to combined patch 3
v4:
  - avoid passing action to datapath if TC offload in use
v3:
  - alter test such that it's reliable, different xlate_error
  - better commit message in patch 2
  - reordered _DEC_TTL value in switch statements
  - add format_dec_ttl_action()
v2:
  - new patch (1) to fix build (switch cases)
  - fixed check-system-userspace

Eric Garver (6):
  dpif: Stub out unimplemented action OVS_ACTION_ATTR_DEC_TTL.
  dpif: Make get_datapath_cap() access support by pointer.
  dpif: Support atomic_bool field type.
  dpif: Probe support for OVS_ACTION_ATTR_DROP.
  tests: system-traffic: Add coverage for drop action.
  tests: system-offload-traffic: Verify re-probe of drop action.

 include/linux/openvswitch.h  |   3 +-
 lib/dpif-netdev.c|   1 +
 lib/dpif.c   |   7 +-
 lib/dpif.h   |   2 +-
 lib/odp-execute.c|   2 +
 lib/odp-util.c   |  23 
 ofproto/ofproto-dpif-ipfix.c |   1 +
 ofproto/ofproto-dpif-sflow.c |   1 +
 ofproto/ofproto-dpif.c   | 187 ---
 ofproto/ofproto-dpif.h   |   4 +-
 tests/system-common-macros.at|   4 +
 tests/system-offloads-traffic.at |  12 ++
 tests/system-traffic.at  |  31 +
 13 files changed, 233 insertions(+), 45 deletions(-)

-- 
2.43.0

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

[ovs-dev] [PATCH v12 4/6] dpif: Probe support for OVS_ACTION_ATTR_DROP.

[Eric Garver]

Kernel support has been added for this action. As such, we need to probe
the datapath for support.

Signed-off-by: Eric Garver 
---
 include/linux/openvswitch.h |  2 +-
 lib/dpif.c  |  6 ++-
 lib/dpif.h  |  2 +-
 ofproto/ofproto-dpif.c  | 78 ++---
 ofproto/ofproto-dpif.h  |  4 +-
 5 files changed, 82 insertions(+), 10 deletions(-)

diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
index a265e05ad253..fce6456f47c8 100644
--- a/include/linux/openvswitch.h
+++ b/include/linux/openvswitch.h
@@ -1086,11 +1086,11 @@ enum ovs_action_attr {
OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */
OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */
OVS_ACTION_ATTR_DEC_TTL,  /* Nested OVS_DEC_TTL_ATTR_*. */
+   OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */
 
 #ifndef __KERNEL__
OVS_ACTION_ATTR_TUNNEL_PUSH,   /* struct ovs_action_push_tnl*/
OVS_ACTION_ATTR_TUNNEL_POP,/* u32 port number. */
-   OVS_ACTION_ATTR_DROP,  /* u32 xlate_error. */
OVS_ACTION_ATTR_LB_OUTPUT, /* u32 bond-id. */
 #endif
__OVS_ACTION_ATTR_MAX,/* Nothing past this will be accepted
diff --git a/lib/dpif.c b/lib/dpif.c
index 0f480bec48d0..23eb18495a66 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -28,6 +28,7 @@
 #include "dpctl.h"
 #include "dpif-netdev.h"
 #include "flow.h"
+#include "netdev-offload.h"
 #include "netdev-provider.h"
 #include "netdev.h"
 #include "netlink.h"
@@ -1935,9 +1936,10 @@ dpif_supports_tnl_push_pop(const struct dpif *dpif)
 }
 
 bool
-dpif_supports_explicit_drop_action(const struct dpif *dpif)
+dpif_may_support_explicit_drop_action(const struct dpif *dpif)
 {
-return dpif_is_netdev(dpif);
+/* TC does not support offloading this action. */
+return dpif_is_netdev(dpif) || !netdev_is_flow_api_enabled();
 }
 
 bool
diff --git a/lib/dpif.h b/lib/dpif.h
index 0f2dc2ef3c55..a764e8a592bd 100644
--- a/lib/dpif.h
+++ b/lib/dpif.h
@@ -940,7 +940,7 @@ int dpif_get_pmds_for_port(const struct dpif * dpif, 
odp_port_t port_no,
 
 char *dpif_get_dp_version(const struct dpif *);
 bool dpif_supports_tnl_push_pop(const struct dpif *);
-bool dpif_supports_explicit_drop_action(const struct dpif *);
+bool dpif_may_support_explicit_drop_action(const struct dpif *);
 bool dpif_synced_dp_layers(struct dpif *);
 
 /* Log functions. */
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 2ec1cd1854ab..dcef9ade5c2e 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -221,6 +221,7 @@ static void ct_zone_config_init(struct dpif_backer *backer);
 static void ct_zone_config_uninit(struct dpif_backer *backer);
 static void ct_zone_timeout_policy_sweep(struct dpif_backer *backer);
 static void ct_zone_limits_commit(struct dpif_backer *backer);
+static bool recheck_support_explicit_drop_action(struct dpif_backer *backer);
 
 static inline struct ofproto_dpif *
 ofproto_dpif_cast(const struct ofproto *ofproto)
@@ -391,6 +392,10 @@ type_run(const char *type)
 udpif_set_threads(backer->udpif, n_handlers, n_revalidators);
 }
 
+if (recheck_support_explicit_drop_action(backer)) {
+backer->need_revalidate = REV_RECONFIGURE;
+}
+
 if (backer->need_revalidate) {
 struct ofproto_dpif *ofproto;
 struct simap_node *node;
@@ -807,6 +812,7 @@ open_dpif_backer(const char *type, struct dpif_backer 
**backerp)
 
 shash_add(&all_dpif_backers, type, backer);
 
+atomic_init(&backer->rt_support.explicit_drop_action, false);
 check_support(backer);
 atomic_count_init(&backer->tnl_count, 0);
 
@@ -855,7 +861,12 @@ ovs_native_tunneling_is_on(struct ofproto_dpif *ofproto)
 bool
 ovs_explicit_drop_action_supported(struct ofproto_dpif *ofproto)
 {
-return ofproto->backer->rt_support.explicit_drop_action;
+bool value;
+
+atomic_read_relaxed(&ofproto->backer->rt_support.explicit_drop_action,
+&value);
+
+return value;
 }
 
 bool
@@ -1379,6 +1390,39 @@ check_ct_timeout_policy(struct dpif_backer *backer)
 return !error;
 }
 
+/* Tests whether backer's datapath supports the OVS_ACTION_ATTR_DROP action. */
+static bool
+check_drop_action(struct dpif_backer *backer)
+{
+struct odputil_keybuf keybuf;
+uint8_t actbuf[NL_A_U32_SIZE];
+struct ofpbuf actions;
+struct ofpbuf key;
+struct flow flow;
+bool supported;
+
+struct odp_flow_key_parms odp_parms = {
+.flow = &flow,
+.probe = true,
+};
+
+memset(&flow, 0, sizeof flow);
+ofpbuf_use_stack(&key, &keybuf, sizeof keybuf);
+odp_flow_key_from_flow(&odp_parms, &key);
+
+ofpbuf_use_stack(&actions, &actbuf, sizeof actbuf);
+nl_msg_put_u32(&actions, OVS_ACTION_ATTR_DROP, XLATE_OK);
+
+supported = dpif_may_support_explicit_drop_action(backer->dpif) &&
+dpif_probe_feature(backer->dpif, "drop", &key, &actions

[ovs-dev] [PATCH v12 2/6] dpif: Make get_datapath_cap() access support by pointer.

[Eric Garver]

This avoids copying the support struct onto the stack.

Acked-by: Eelco Chaudron 
Signed-off-by: Eric Garver 
---
 ofproto/ofproto-dpif.c | 59 +-
 1 file changed, 29 insertions(+), 30 deletions(-)

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index f59d69c4d1e8..d732198de5ea 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -5688,47 +5688,46 @@ ct_zone_limit_protection_update(const char 
*datapath_type, bool protected)
 static void
 get_datapath_cap(const char *datapath_type, struct smap *cap)
 {
-struct odp_support odp;
-struct dpif_backer_support s;
+struct dpif_backer_support *s;
 struct dpif_backer *backer = shash_find_data(&all_dpif_backers,
  datapath_type);
 if (!backer) {
 return;
 }
-s = backer->rt_support;
-odp = s.odp;
+s = &backer->rt_support;
 
 /* ODP_SUPPORT_FIELDS */
 smap_add_format(cap, "max_vlan_headers", "%"PRIuSIZE,
-odp.max_vlan_headers);
-smap_add_format(cap, "max_mpls_depth", "%"PRIuSIZE, odp.max_mpls_depth);
-smap_add(cap, "recirc", odp.recirc ? "true" : "false");
-smap_add(cap, "ct_state", odp.ct_state ? "true" : "false");
-smap_add(cap, "ct_zone", odp.ct_zone ? "true" : "false");
-smap_add(cap, "ct_mark", odp.ct_mark ? "true" : "false");
-smap_add(cap, "ct_label", odp.ct_label ? "true" : "false");
-smap_add(cap, "ct_state_nat", odp.ct_state_nat ? "true" : "false");
-smap_add(cap, "ct_orig_tuple", odp.ct_orig_tuple ? "true" : "false");
-smap_add(cap, "ct_orig_tuple6", odp.ct_orig_tuple6 ? "true" : "false");
-smap_add(cap, "nd_ext", odp.nd_ext ? "true" : "false");
+s->odp.max_vlan_headers);
+smap_add_format(cap, "max_mpls_depth", "%"PRIuSIZE, s->odp.max_mpls_depth);
+smap_add(cap, "recirc", s->odp.recirc ? "true" : "false");
+smap_add(cap, "ct_state", s->odp.ct_state ? "true" : "false");
+smap_add(cap, "ct_zone", s->odp.ct_zone ? "true" : "false");
+smap_add(cap, "ct_mark", s->odp.ct_mark ? "true" : "false");
+smap_add(cap, "ct_label", s->odp.ct_label ? "true" : "false");
+smap_add(cap, "ct_state_nat", s->odp.ct_state_nat ? "true" : "false");
+smap_add(cap, "ct_orig_tuple", s->odp.ct_orig_tuple ? "true" : "false");
+smap_add(cap, "ct_orig_tuple6", s->odp.ct_orig_tuple6 ? "true" : "false");
+smap_add(cap, "nd_ext", s->odp.nd_ext ? "true" : "false");
 
 /* DPIF_SUPPORT_FIELDS */
-smap_add(cap, "masked_set_action", s.masked_set_action ? "true" : "false");
-smap_add(cap, "tnl_push_pop", s.tnl_push_pop ? "true" : "false");
-smap_add(cap, "ufid", s.ufid ? "true" : "false");
-smap_add(cap, "trunc", s.trunc ? "true" : "false");
-smap_add(cap, "clone", s.clone ? "true" : "false");
-smap_add(cap, "sample_nesting", s.sample_nesting ? "true" : "false");
-smap_add(cap, "ct_eventmask", s.ct_eventmask ? "true" : "false");
-smap_add(cap, "ct_clear", s.ct_clear ? "true" : "false");
-smap_add_format(cap, "max_hash_alg", "%"PRIuSIZE, s.max_hash_alg);
-smap_add(cap, "check_pkt_len", s.check_pkt_len ? "true" : "false");
-smap_add(cap, "ct_timeout", s.ct_timeout ? "true" : "false");
+smap_add(cap, "masked_set_action",
+ s->masked_set_action ? "true" : "false");
+smap_add(cap, "tnl_push_pop", s->tnl_push_pop ? "true" : "false");
+smap_add(cap, "ufid", s->ufid ? "true" : "false");
+smap_add(cap, "trunc", s->trunc ? "true" : "false");
+smap_add(cap, "clone", s->clone ? "true" : "false");
+smap_add(cap, "sample_nesting", s->sample_nesting ? "true" : "false");
+smap_add(cap, "ct_eventmask", s->ct_eventmask ? "true" : "false");
+smap_add(cap, "ct_clear", s->ct_clear ? "true" : "false");
+smap_add_format(cap, "max_hash_alg", "%"PRIuSIZE, s->max_hash_alg);
+smap_add(cap, "check_pkt_len", s->check_pkt_len ? "true" : "false");
+smap_add(cap, "ct_timeout", s->ct_timeout ? "true" : "false");
 smap_add(cap, "explicit_drop_action",
- s.explicit_drop_action ? "true" :"false");
-smap_add(cap, "lb_output_action", s.lb_output_action ? "true" : "false");
-smap_add(cap, "ct_zero_snat", s.ct_zero_snat ? "true" : "false");
-smap_add(cap, "add_mpls", s.add_mpls ? "true" : "false");
+ s->explicit_drop_action ? "true" :"false");
+smap_add(cap, "lb_output_action", s->lb_output_action ? "true" : "false");
+smap_add(cap, "ct_zero_snat", s->ct_zero_snat ? "true" : "false");
+smap_add(cap, "add_mpls", s->add_mpls ? "true" : "false");
 
 /* The ct_tuple_flush is implemented on dpif level, so it is supported
  * for all backers. */
-- 
2.43.0

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

[ovs-dev] [PATCH v12 1/6] dpif: Stub out unimplemented action OVS_ACTION_ATTR_DEC_TTL.

[Eric Garver]

This is prep for adding a different OVS_ACTION_ATTR_ enum value. This
action, OVS_ACTION_ATTR_DEC_TTL, is not actually implemented. However,
to make -Werror happy we must add a case to all existing switches.

Acked-by: Eelco Chaudron 
Signed-off-by: Eric Garver 
---
 include/linux/openvswitch.h  |  1 +
 lib/dpif-netdev.c|  1 +
 lib/dpif.c   |  1 +
 lib/odp-execute.c|  2 ++
 lib/odp-util.c   | 23 +++
 ofproto/ofproto-dpif-ipfix.c |  1 +
 ofproto/ofproto-dpif-sflow.c |  1 +
 7 files changed, 30 insertions(+)

diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
index e305c331516b..a265e05ad253 100644
--- a/include/linux/openvswitch.h
+++ b/include/linux/openvswitch.h
@@ -1085,6 +1085,7 @@ enum ovs_action_attr {
OVS_ACTION_ATTR_CLONE,/* Nested OVS_CLONE_ATTR_*.  */
OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */
OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */
+   OVS_ACTION_ATTR_DEC_TTL,  /* Nested OVS_DEC_TTL_ATTR_*. */
 
 #ifndef __KERNEL__
OVS_ACTION_ATTR_TUNNEL_PUSH,   /* struct ovs_action_push_tnl*/
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index e6c53937d8b9..89b0d1d6b4aa 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -9489,6 +9489,7 @@ dp_execute_cb(void *aux_, struct dp_packet_batch 
*packets_,
 case OVS_ACTION_ATTR_CHECK_PKT_LEN:
 case OVS_ACTION_ATTR_DROP:
 case OVS_ACTION_ATTR_ADD_MPLS:
+case OVS_ACTION_ATTR_DEC_TTL:
 case __OVS_ACTION_ATTR_MAX:
 OVS_NOT_REACHED();
 }
diff --git a/lib/dpif.c b/lib/dpif.c
index d07241f1e7cd..0f480bec48d0 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -1289,6 +1289,7 @@ dpif_execute_helper_cb(void *aux_, struct dp_packet_batch 
*packets_,
 case OVS_ACTION_ATTR_CHECK_PKT_LEN:
 case OVS_ACTION_ATTR_DROP:
 case OVS_ACTION_ATTR_ADD_MPLS:
+case OVS_ACTION_ATTR_DEC_TTL:
 case __OVS_ACTION_ATTR_MAX:
 OVS_NOT_REACHED();
 }
diff --git a/lib/odp-execute.c b/lib/odp-execute.c
index eb03b57c42ec..081e4d43268a 100644
--- a/lib/odp-execute.c
+++ b/lib/odp-execute.c
@@ -837,6 +837,7 @@ requires_datapath_assistance(const struct nlattr *a)
 case OVS_ACTION_ATTR_CT_CLEAR:
 case OVS_ACTION_ATTR_CHECK_PKT_LEN:
 case OVS_ACTION_ATTR_ADD_MPLS:
+case OVS_ACTION_ATTR_DEC_TTL:
 case OVS_ACTION_ATTR_DROP:
 return false;
 
@@ -1227,6 +1228,7 @@ odp_execute_actions(void *dp, struct dp_packet_batch 
*batch, bool steal,
 case OVS_ACTION_ATTR_RECIRC:
 case OVS_ACTION_ATTR_CT:
 case OVS_ACTION_ATTR_UNSPEC:
+case OVS_ACTION_ATTR_DEC_TTL:
 case __OVS_ACTION_ATTR_MAX:
 /* The following actions are handled by the scalar implementation. */
 case OVS_ACTION_ATTR_POP_VLAN:
diff --git a/lib/odp-util.c b/lib/odp-util.c
index 9306c9b4d47a..f4c492f2ae6f 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -143,6 +143,7 @@ odp_action_len(uint16_t type)
 case OVS_ACTION_ATTR_POP_NSH: return 0;
 case OVS_ACTION_ATTR_CHECK_PKT_LEN: return ATTR_LEN_VARIABLE;
 case OVS_ACTION_ATTR_ADD_MPLS: return sizeof(struct ovs_action_add_mpls);
+case OVS_ACTION_ATTR_DEC_TTL: return ATTR_LEN_VARIABLE;
 case OVS_ACTION_ATTR_DROP: return sizeof(uint32_t);
 
 case OVS_ACTION_ATTR_UNSPEC:
@@ -1130,6 +1131,25 @@ format_odp_check_pkt_len_action(struct ds *ds, const 
struct nlattr *attr,
 ds_put_cstr(ds, "))");
 }
 
+static void
+format_dec_ttl_action(struct ds *ds, const struct nlattr *attr,
+  const struct hmap *portno_names)
+{
+const struct nlattr *a;
+unsigned int left;
+
+ds_put_cstr(ds,"dec_ttl(le_1(");
+NL_ATTR_FOR_EACH (a, left,
+  nl_attr_get(attr), nl_attr_get_size(attr)) {
+if (nl_attr_type(a) == OVS_ACTION_ATTR_DEC_TTL) {
+   format_odp_actions(ds, nl_attr_get(a),
+  nl_attr_get_size(a), portno_names);
+   break;
+}
+}
+ds_put_format(ds, "))");
+}
+
 static void
 format_odp_action(struct ds *ds, const struct nlattr *a,
   const struct hmap *portno_names)
@@ -1283,6 +1303,9 @@ format_odp_action(struct ds *ds, const struct nlattr *a,
   ntohs(mpls->mpls_ethertype));
 break;
 }
+case OVS_ACTION_ATTR_DEC_TTL:
+format_dec_ttl_action(ds, a, portno_names);
+break;
 case OVS_ACTION_ATTR_DROP:
 ds_put_cstr(ds, "drop");
 break;
diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
index e6c2968f7e90..cd65dae7e18a 100644
--- a/ofproto/ofproto-dpif-ipfix.c
+++ b/ofproto/ofproto-dpif-ipfix.c
@@ -3135,6 +3135,7 @@ dpif_ipfix_read_actions(const struct flow *flow,
 case OVS_ACTION_ATTR_UNSPEC:
 case OVS_ACTION_ATTR_DROP:
 case OVS_ACTION_ATTR_ADD_MPLS:
+case OVS_ACTION_ATTR_DEC_TTL:
 case __OVS_ACTION_ATTR_MAX:

Re: [ovs-dev] [PATCH ovn] northd: Fix NAT configuration with --add-route option for gw-router.

[Mark Michelson]

Thanks for this Lorenzo, looks good to me.

Acked-by: Mark Michelson 

On 2/16/24 09:23, Lorenzo Bianconi wrote:

Enable automatic static route configuration when NAT is created with
--ad-route option for gw routers similar to what is currently supported
for distributed routers with gw_router_ports.

Reported-at: https://issues.redhat.com/browse/FDP-244
Signed-off-by: Lorenzo Bianconi 
---
  northd/northd.c |  2 +-
  tests/ovn-northd.at | 12 ++
  tests/system-ovn.at | 90 +
  3 files changed, 103 insertions(+), 1 deletion(-)

diff --git a/northd/northd.c b/northd/northd.c
index 7c731db6d..4d31b7e22 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -15340,7 +15340,7 @@ build_routable_flows_for_router_port(
  }
  
  if (lrp->nbrp->ha_chassis_group ||

-lrp->nbrp->n_gateway_chassis) {
+lrp->nbrp->n_gateway_chassis || lrp->od->is_gw_router) {
  for (size_t j = 0; j < ra.n_addrs; j++) {
  struct lport_addresses *laddrs = &ra.laddrs[j];
  for (size_t k = 0; k < laddrs->n_ipv4_addrs; k++) {
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 591ad5aad..098bec0bb 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -5040,6 +5040,18 @@ check ovn-nbctl --wait=sb lrp-del-gateway-chassis ro2-sw 
hv2
  
  check_lflows 0
  
+AS_BOX([Checking that NAT flows are installed for gw routers])

+
+check ovn-nbctl set logical_router ro1 options:chassis=hv1
+check ovn-nbctl --wait=sb set logical_router ro2 options:chassis=hv2
+
+check_lflows 1
+
+check ovn-nbctl clear logical_router ro1 options
+check ovn-nbctl --wait=sb clear logical_router ro2 options
+
+check_lflows 0
+
  AS_BOX([Checking that NAT flows are installed for routers with 
HA_Chassis_Group])
  
  check ovn-nbctl set logical_router_port ro1-sw ha_chassis_group="$grp1_uuid"

diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index c22c7882f..b1fd1a937 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -12184,3 +12184,93 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port 
patch-.*/d
  /connection dropped.*/d"])
  AT_CLEANUP
  ])
+
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([IP NAT add-route])
+AT_KEYWORDS([ip-nat-add-route])
+
+ovn_start
+OVS_TRAFFIC_VSWITCHD_START()
+ADD_BR([br-int])
+
+check ovs-vsctl \
+-- set Open_vSwitch . external-ids:system-id=hv1 \
+-- set Open_vSwitch . 
external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \
+-- set Open_vSwitch . external-ids:ovn-encap-type=geneve \
+-- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \
+-- set bridge br-int fail-mode=secure other-config:disable-in-band=true
+
+start_daemon ovn-controller
+
+check ovn-nbctl lr-add lr0
+check ovn-nbctl set logical_router lr0 options:chassis=hv1
+
+check ovn-nbctl lr-add lr1
+check ovn-nbctl set logical_router lr1 options:chassis=hv1
+
+check ovn-nbctl ls-add sw0
+check ovn-nbctl ls-add sw1
+check ovn-nbctl ls-add join
+
+check ovn-nbctl lrp-add lr0 lr-sw0 00:00:01:01:02:03 192.168.0.1/24
+check ovn-nbctl lsp-add sw0 rp-sw0 -- set Logical_Switch_Port rp-sw0 \
+type=router options:router-port=lr-sw0 \
+-- lsp-set-addresses rp-sw0 router
+
+check ovn-nbctl lrp-add lr0 lr0-join 04:00:01:01:02:03 172.16.1.1/24
+check ovn-nbctl lsp-add join rp0-join -- set Logical_Switch_Port rp0-join \
+type=router options:router-port=lr0-join \
+-- lsp-set-addresses rp0-join router
+
+check ovn-nbctl lrp-add lr1 lr-sw1 00:00:02:01:02:03 192.168.1.1/24
+check ovn-nbctl lsp-add sw1 rp-sw1 -- set Logical_Switch_Port rp-sw1 \
+type=router options:router-port=lr-sw1 \
+-- lsp-set-addresses rp-sw1 router
+
+check ovn-nbctl lrp-add lr1 lr1-join 04:00:02:01:02:03 172.16.2.2/24
+check ovn-nbctl lsp-add join rp1-join -- set Logical_Switch_Port rp1-join \
+type=router options:router-port=lr1-join \
+-- lsp-set-addresses rp1-join router
+
+ADD_NAMESPACES(sw0-p0)
+ADD_VETH(sw0-p0, sw0-p0, br-int, "192.168.0.2/24", "f0:00:00:01:02:03", 
"192.168.0.1")
+check ovn-nbctl lsp-add sw0 sw0-p0 \
+-- lsp-set-addresses sw0-p0 "f0:00:00:01:02:03 192.168.0.2"
+
+ADD_NAMESPACES(sw1-p0)
+ADD_VETH(sw1-p0, sw1-p0, br-int, "192.168.1.2/24", "f0:00:00:11:02:03", 
"192.168.1.1")
+check ovn-nbctl lsp-add sw1 sw1-p0 \
+-- lsp-set-addresses sw1-p0 "f0:00:00:11:02:03 192.168.1.2"
+
+check ovn-nbctl --add-route lr-nat-add lr0 dnat_and_snat 172.16.1.100 
192.168.0.2 sw0-p0 00:00:00:00:03:01
+check ovn-nbctl --add-route lr-nat-add lr1 dnat_and_snat 172.16.1.101 
192.168.1.2 sw1-p0 00:00:00:00:04:01
+
+wait_for_ports_up
+check ovn-nbctl --wait=hv sync
+
+NS_CHECK_EXEC([sw0-p0], [ping -q -c 3 -i 0.3 -w 2 172.16.1.101 | FORMAT_PING], 
\
+[0], [dnl
+3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+
+NS_CHECK_EXEC([sw1-p0], [ping -q -c 3 -i 0.3 -w 2 172.16.1.100 | FORMAT_PING], 
\
+[0], [dnl
+3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+
+OVS_A

Re: [ovs-dev] [PATCH ovn] northd: Fix pmtud for non routed traffic.

[Mark Michelson]

Hi Lorenzo,

Thanks for the fix.

Acked-by: Mark Michelson 

When this is merged, the following should also be folded in:

---
diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
index 17b414144..0cf1c2bb5 100644
--- a/northd/ovn-northd.8.xml
+++ b/northd/ovn-northd.8.xml
@@ -338,7 +338,7 @@
 

 
-  This table adds a priority-110 flow that matches 'recirculated' 
icmp{4,6}
+  This table adds a priority-105 flow that matches 'recirculated' 
icmp{4,6}

   error 'packet too big' to drop the packet.
 

---

This accounts for the change of priority introduced in this patch.

I also noticed a small spelling mistake that should be corrected. I 
marked it below.


On 2/13/24 08:32, Lorenzo Bianconi wrote:

Similar to what is already implemented for routed e/w traffic,
introduce pmtud support for e/w traffic between two logical switch ports
connected to the same logical switch, but running on two different
hypervisors.

Reported-at: https://issues.redhat.com/browse/FDP-362
Signed-off-by: Lorenzo Bianconi 
---
  controller/lflow.h  |   1 +
  controller/physical.c   |  31 -
  northd/northd.c |  35 +++---
  northd/ovn-northd.8.xml |  14 +++-
  tests/multinode.at  | 151 
  tests/ovn-northd.at |  22 --
  6 files changed, 236 insertions(+), 18 deletions(-)

diff --git a/controller/lflow.h b/controller/lflow.h
index 9b7ffa19c..906a26280 100644
--- a/controller/lflow.h
+++ b/controller/lflow.h
@@ -94,6 +94,7 @@ struct uuid;
  #define OFTABLE_ECMP_NH  77
  #define OFTABLE_CHK_LB_AFFINITY  78
  #define OFTABLE_MAC_CACHE_USE79
+#define OFTABLE_CT_ZONE_LOOKUP   80
  
  struct lflow_ctx_in {

  struct ovsdb_idl_index *sbrec_multicast_group_by_name_datapath;
diff --git a/controller/physical.c b/controller/physical.c
index c32642d2c..6a9327b8d 100644
--- a/controller/physical.c
+++ b/controller/physical.c
@@ -2451,8 +2451,37 @@ physical_run(struct physical_ctx *p_ctx,
p_ctx->n_encap_ips,
p_ctx->encap_ips,
flow_table, &ofpacts);
+
+if (!local_binding_get_primary_pb(p_ctx->local_bindings,
+  binding->logical_port)) {
+continue;
+}
+
+/* Table 80, priority 100.
+ * ===
+ *
+ * Process ICMP{4,6} error packets too big locally generalted from the


s/generalted/generated/


+ * kernel in order to lookup proper ct_zone. */
+struct match match = MATCH_CATCHALL_INITIALIZER;
+match_set_metadata(&match, htonll(binding->datapath->tunnel_key));
+match_set_reg(&match, MFF_LOG_INPORT - MFF_REG0, binding->tunnel_key);
+
+ofpbuf_clear(&ofpacts);
+struct zone_ids zone_ids = get_zone_ids(binding, p_ctx->ct_zones);
+put_zones_ofpacts(&zone_ids, &ofpacts);
+put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, &ofpacts);
+ofctrl_add_flow(flow_table, OFTABLE_CT_ZONE_LOOKUP, 100, 0,
+&match, &ofpacts, hc_uuid);
  }
  
+/* Default flow for CT_ZONE_LOOKUP Table. */

+struct match ct_look_def_match;
+match_init_catchall(&ct_look_def_match);
+ofpbuf_clear(&ofpacts);
+put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, &ofpacts);
+ofctrl_add_flow(flow_table, OFTABLE_CT_ZONE_LOOKUP, 0, 0,
+&ct_look_def_match, &ofpacts, hc_uuid);
+
  /* Handle output to multicast groups, in tables 40 and 41. */
  const struct sbrec_multicast_group *mc;
  SBREC_MULTICAST_GROUP_TABLE_FOR_EACH (mc, p_ctx->mc_group_table) {
@@ -2511,7 +2540,7 @@ physical_run(struct physical_ctx *p_ctx,
  /* Add specif flows for E/W ICMPv{4,6} packets if tunnelled packets
   * do not fit path MTU.
   */
-put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, &ofpacts);
+put_resubmit(OFTABLE_CT_ZONE_LOOKUP, &ofpacts);
  
  /* IPv4 */

  match_init_catchall(&match);
diff --git a/northd/northd.c b/northd/northd.c
index a174a4dcd..34c56f95e 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -8634,7 +8634,7 @@ build_lswitch_lflows_admission_control(struct 
ovn_datapath *od,
  ovs_assert(od->nbs);
  
  /* Default action for recirculated ICMP error 'packet too big'. */

-ovn_lflow_add(lflows, od, S_SWITCH_IN_CHECK_PORT_SEC, 110,
+ovn_lflow_add(lflows, od, S_SWITCH_IN_CHECK_PORT_SEC, 105,
"((ip4 && icmp4.type == 3 && icmp4.code == 4) ||"
" (ip6 && icmp6.type == 2 && icmp6.code == 0)) &&"
" flags.tunnel_rx == 1", debug_drop_action(), lflow_ref);
@@ -11822,7 +11822,24 @@ build_lswitch_icmp_packet_toobig_admin_flows(
  {
  ovs_assert(op->nbsp);
  
+ds_clear(match);

  if (!lsp_is_router(op->nbsp)) {
+struct eth_addr mac;
+if (!op->nbsp->n_addresses ||
+ 

[ovs-dev] [PATCH] route-table: Avoid routes from non-standard routing tables.

[Ilya Maximets]

Currently, ovs-vswitchd is subscribed to all the routing changes in the
kernel.  On each change, it marks the internal routing table cache as
invalid, then resets it and dumps all the routes from the kernel from
scratch.  The reason for that is kernel routing updates not being
reliable in a sense that it's hard to tell which route is getting
removed or modified.  Userspace application has to track the order in
which route entries are dumped from the kernel.  Updates can get lost
or even duplicated and the kernel doesn't provide a good mechanism to
distinguish one route from another.  To my knowledge, dumping all the
routes from a kernel after each change is the only way to keep the
cache consistent.  Some more info can be found in the following never
addressed issues:
  https://bugzilla.redhat.com/1337860
  https://bugzilla.redhat.com/1337855

It seems to be believed that NetworkManager "mostly" does incremental
updates right.  But it is still not completely correct, will re-dump
the whole table in certain cases, and it takes a huge amount of very
complicated code to do the accounting and route comparisons.

Going back to ovs-vswitchd, it currently dumps routes from all the
routing tables.  If it will get conflicting routes from multiple
tables, the cache will not be useful.  The routing cache in userspace
is primarily used for checking the egress port for tunneled traffic
and this way also detecting link state changes for a tunnel port.
For userspace datapath it is used for actual routing of the packet
after sending to a native tunnel.
With kernel datapath we don't really have a mechanism to know which
routing table will actually be used by the kernel after encapsulation,
so our lookups on a cache may be incorrect because of this as well.

So, unless all the relevant routes are in the standard tables, the
lookup in userspace route cache is unreliable.

Luckily, most setups are not using any complicated routing in
non-standard tables that OVS has to be aware of.

It is possible, but unlikely, that standard routing tables are
completely empty while some other custom table is not, and all the OVS
tunnel traffic is directed to that table.  That would be the only
scenario where dumping non-standard tables would make sense.  But it
seems like this kind of setup will likely need a way to tell OVS from
which table the routes should be taken, or we'll need to dump routing
rules and keep a separate cache for each table, so we can first match
on rules and then lookup correct routes in a specific table.  I'm not
sure if trying to implement all that is justified.

For now, stop considering routes from non-standard tables to avoid
mixing different tables together and also wasting CPU resources.

This fixes a high CPU usage in ovs-vswitchd in case a BGP daemon is
running on a same host and in a same network namespace with OVS using
its own custom routing table.

Unfortunately, there seems to be no way to tell the kernel to send
updates only for particular tables.  So, we'll still receive and parse
all of them.  But they will not result in a full cache invalidation in
most cases.

Linux kernel v4.20 introduced filtering support for RTM_GETROUTE dumps.
So, we can make use of it and dump only standard tables when we get a
relevant route update.  NETLINK_GET_STRICT_CHK has to be enabled on
the socket for filtering to work.  There is no reason to not enable it
by default, if supported.  It is not used outside of NETLINK_ROUTE.

Fixes: f0e167f0dbad ("route-table: Handle route updates more robustly.")
Fixes: ea83a2fcd0d3 ("lib: Show tunnel egress interface in ovsdb")
Reported-at: https://github.com/openvswitch/ovs-issues/issues/185
Reported-at: 
https://mail.openvswitch.org/pipermail/ovs-discuss/2022-October/052091.html
Signed-off-by: Ilya Maximets 
---
 lib/netlink-protocol.h | 10 ++
 lib/netlink-socket.c   |  8 +
 lib/route-table.c  | 80 +-
 tests/system-route.at  | 64 +
 4 files changed, 146 insertions(+), 16 deletions(-)

diff --git a/lib/netlink-protocol.h b/lib/netlink-protocol.h
index 6eaa7035a..e4bb28ac9 100644
--- a/lib/netlink-protocol.h
+++ b/lib/netlink-protocol.h
@@ -155,6 +155,11 @@ enum {
 #define NLA_TYPE_MASK   ~(NLA_F_NESTED | NLA_F_NET_BYTEORDER)
 #endif
 
+/* Introduced in v4.4. */
+#ifndef NLM_F_DUMP_FILTERED
+#define NLM_F_DUMP_FILTERED 0x20
+#endif
+
 /* These were introduced all together in 2.6.14.  (We want our programs to
  * support the newer kernel features even if compiled with older headers.) */
 #ifndef NETLINK_ADD_MEMBERSHIP
@@ -168,6 +173,11 @@ enum {
 #define NETLINK_LISTEN_ALL_NSID 8
 #endif
 
+/* Strict checking of netlink arguments introduced in Linux kernel v4.20. */
+#ifndef NETLINK_GET_STRICT_CHK
+#define NETLINK_GET_STRICT_CHK 12
+#endif
+
 /* These were introduced all together in 2.6.23.  (We want our programs to
  * support the newer kernel features even if compiled with older headers.) */
 #ifndef

[ovs-dev] [PATCH ovn] tests: Ignore transaction errors in MAC Binding.

[Xavier Simonart]

Fixes: 65f9f010b426 ("tests: Check unit tests logs for errors.")
Signed-off-by: Xavier Simonart 
---
 tests/ovn-macros.at | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/ovn-macros.at b/tests/ovn-macros.at
index 3410afb74..5c69facd2 100644
--- a/tests/ovn-macros.at
+++ b/tests/ovn-macros.at
@@ -48,12 +48,15 @@ m4_define([OVN_CLEANUP_SBOX],[
 # "has no network name*" as localnet network_name is often set "after" 
creating localnet.
 # "receive tunnel port not found*" happening when closing other hv.
 # "Failed to locate tunnel to reach main chassis" happening when 
controller started e.g. before ovn-bridge-mappings is set
+# "Transaction causes multiple rows in \"MAC_Binding\" table to have 
identical values"
+#  happening as ovn-controller has a race condition over MAC binding table 
with other controllers (GARP).
 AT_CHECK([check_logs "
 $error
 /connection failed (No such file or directory)/d
 /has no network name*/d
 /receive tunnel port not found*/d
 /Failed to locate tunnel to reach main chassis/d
+/Transaction causes multiple rows.*MAC_Binding/d
 " $sbox])
 ])
 
-- 
2.31.1

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

Re: [ovs-dev] [PATCH ovn v3] tests: Remove table numbers from "action parsing".

[0-day Robot]

Bleep bloop.  Greetings Xavier Simonart, 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.


git-am:
error: sha1 information is lacking or useless (tests/ovn.at).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 tests: Remove table numbers from "action parsing".
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".


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

[ovs-dev] [PATCH ovn v3 2/2] tests: Remove table numbers from "action parsing".

[Xavier Simonart]

This patch uses the recently introduced macros defining openflow table numbers.

Signed-off-by: Xavier Simonart 

---
v2: - Handled Ales' comments (i.e. fix few remaining hard-coded numbers)
- Rebase on origin/main
v3: - Rebase on origin/main
---
 tests/ovn-macros.at |   4 +
 tests/ovn.at| 245 +++-
 2 files changed, 132 insertions(+), 117 deletions(-)

diff --git a/tests/ovn-macros.at b/tests/ovn-macros.at
index 3410afb74..45ec06c02 100644
--- a/tests/ovn-macros.at
+++ b/tests/ovn-macros.at
@@ -1042,6 +1042,8 @@ m4_define([OFTABLE_OUTPUT_LARGE_PKT_DETECT], [37])
 m4_define([OFTABLE_OUTPUT_LARGE_PKT_PROCESS], [38])
 m4_define([OFTABLE_REMOTE_OUTPUT], [39])
 m4_define([OFTABLE_LOCAL_OUTPUT], [40])
+m4_define([OFTABLE_LOG_EGRESS_PIPELINE], [42])
+m4_define([OFTABLE_SAVE_INPORT], [64])
 m4_define([OFTABLE_LOG_TO_PHY], [65])
 m4_define([OFTABLE_MAC_BINDING], [66])
 m4_define([OFTABLE_MAC_LOOKUP], [67])
@@ -1057,3 +1059,5 @@ m4_define([OFTABLE_ECMP_NH_MAC], [76])
 m4_define([OFTABLE_ECMP_NH], [77])
 m4_define([OFTABLE_CHK_LB_AFFINITY], [78])
 m4_define([OFTABLE_MAC_CACHE_USE], [79])
+
+m4_define([OFTABLE_SAVE_INPORT_HEX], [m4_eval(OFTABLE_SAVE_INPORT, 16)])
diff --git a/tests/ovn.at b/tests/ovn.at
index 2a5417685..de103a5bf 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -979,6 +979,17 @@ AT_CLEANUP
 AT_SETUP([action parsing])
 dnl Unindented text is input (a set of OVN logical actions).
 dnl Indented text is expected output.
+
+# lflow table hard-coded to 10 in test-ovn, so next is 11.
+m4_define([lflow_table], [11])
+
+m4_define([NEXT], [m4_if(
+$1, ingress, [m4_eval($2 + OFTABLE_LOG_INGRESS_PIPELINE)],
+$1, egress, [m4_eval($2 + OFTABLE_LOG_EGRESS_PIPELINE)])])
+
+m4_define([oflow_in_table], [NEXT(ingress, lflow_table)])
+m4_define([oflow_out_table], [NEXT(egress, lflow_table)])
+
 AT_DATA([test-cases.txt], [
 # drop
 drop;
@@ -990,18 +1001,18 @@ next; drop;
 
 # output
 output;
-encodes as resubmit(,64)
+encodes as resubmit(,OFTABLE_SAVE_INPORT)
 
 # next
 next;
-encodes as resubmit(,19)
-next(11);
+encodes as resubmit(,oflow_in_table)
+next(lflow_table);
 formats as next;
-encodes as resubmit(,19)
+encodes as resubmit(,oflow_in_table)
 next(0);
-encodes as resubmit(,8)
+encodes as resubmit(,OFTABLE_LOG_INGRESS_PIPELINE)
 next(23);
-encodes as resubmit(,31)
+encodes as resubmit(,NEXT(ingress, 23))
 
 next();
 Syntax error at `)' expecting "pipeline" or "table".
@@ -1010,29 +1021,29 @@ next(10;
 next(24);
 "next" action cannot advance beyond table 23.
 
-next(table=11);
+next(table=lflow_table);
 formats as next;
-encodes as resubmit(,19)
+encodes as resubmit(,oflow_in_table)
 next(pipeline=ingress);
 formats as next;
-encodes as resubmit(,19)
-next(table=11, pipeline=ingress);
+encodes as resubmit(,oflow_in_table)
+next(table=lflow_table, pipeline=ingress);
 formats as next;
-encodes as resubmit(,19)
-next(pipeline=ingress, table=11);
+encodes as resubmit(,oflow_in_table)
+next(pipeline=ingress, table=lflow_table);
 formats as next;
-encodes as resubmit(,19)
+encodes as resubmit(,oflow_in_table)
 
 next(pipeline=egress);
-formats as next(pipeline=egress, table=11);
-encodes as resubmit(,53)
+formats as next(pipeline=egress, table=lflow_table);
+encodes as resubmit(,oflow_out_table)
 
 next(pipeline=egress, table=5);
-encodes as resubmit(,47)
+encodes as resubmit(,NEXT(egress, 5))
 
 next(table=10);
 formats as next(10);
-encodes as resubmit(,18)
+encodes as resubmit(,NEXT(ingress, 10))
 
 # Loading a constant value.
 tcp.dst=80;
@@ -1054,7 +1065,7 @@ ip.ttl=4;
 has prereqs eth.type == 0x800 || eth.type == 0x86dd
 outport="eth0"; next; outport="LOCAL"; next;
 formats as outport = "eth0"; next; outport = "LOCAL"; next;
-encodes as 
set_field:0x5->reg15,resubmit(,19),set_field:0xfffe->reg15,resubmit(,19)
+encodes as 
set_field:0x5->reg15,resubmit(,oflow_in_table),set_field:0xfffe->reg15,resubmit(,oflow_in_table)
 
 inport[[1]] = 1;
 Cannot select subfield of string field inport.
@@ -1152,35 +1163,35 @@ pkt.mark = "foo";
 
 # load balancing.
 ct_lb;
-encodes as ct(table=19,zone=NXM_NX_REG13[[0..15]],nat)
+encodes as ct(table=oflow_in_table,zone=NXM_NX_REG13[[0..15]],nat)
 has prereqs ip
 ct_lb();
 formats as ct_lb;
-encodes as ct(table=19,zone=NXM_NX_REG13[[0..15]],nat)
+encodes as ct(table=oflow_in_table,zone=NXM_NX_REG13[[0..15]],nat)
 has prereqs ip
 ct_lb(192.168.1.2:80, 192.168.1.3:80);
 Syntax error at `192.168.1.2' expecting backends.
 ct_lb(backends=192.168.1.2:80,192.168.1.3:80);
 encodes as group:1
-uses group: id(1), 
name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:100,actions=ct(nat(dst=192.168.1.2:80),commit,table=19,zone=NXM_NX_REG13[[0..15]],exec(set_field:2/2->ct_label)),bucket=bucket_id=1,weight:100,actions=ct(nat(dst=192.16

[ovs-dev] [PATCH ovn v3 1/2] tests: Make "action parsing" support expansion.

[Xavier Simonart]

There are only two changes:
- The AT_DATA content is not double quoted between square brackets.
- All '[' have been replaced by '[[' and all ']' by ']]'
This patch will be used in subsequent patch to remove hard-coded
table numbers.

Signed-off-by: Xavier Simonart 

---
v2: - Rebase on origin/main
v3: - Rebase on origin/main
---
 tests/ovn.at | 594 +--
 1 file changed, 297 insertions(+), 297 deletions(-)

diff --git a/tests/ovn.at b/tests/ovn.at
index 9ec1e3bab..2a5417685 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -979,8 +979,8 @@ AT_CLEANUP
 AT_SETUP([action parsing])
 dnl Unindented text is input (a set of OVN logical actions).
 dnl Indented text is expected output.
-AT_DATA([test-cases.txt],
-[[# drop
+AT_DATA([test-cases.txt], [
+# drop
 drop;
 encodes as drop
 drop; next;
@@ -1039,12 +1039,12 @@ tcp.dst=80;
 formats as tcp.dst = 80;
 encodes as set_field:80->tcp_dst
 has prereqs ip.proto == 0x6 && (eth.type == 0x800 || eth.type == 0x86dd)
-eth.dst[40] = 1;
+eth.dst[[40]] = 1;
 encodes as set_field:01:00:00:00:00:00/01:00:00:00:00:00->eth_dst
 vlan.pcp = 2;
 encodes as set_field:0x4000/0xe000->vlan_tci
-has prereqs vlan.tci[12]
-vlan.tci[13..15] = 2;
+has prereqs vlan.tci[[12]]
+vlan.tci[[13..15]] = 2;
 encodes as set_field:0x4000/0xe000->vlan_tci
 inport = "";
 encodes as set_field:0->reg14
@@ -1056,11 +1056,11 @@ outport="eth0"; next; outport="LOCAL"; next;
 formats as outport = "eth0"; next; outport = "LOCAL"; next;
 encodes as 
set_field:0x5->reg15,resubmit(,19),set_field:0xfffe->reg15,resubmit(,19)
 
-inport[1] = 1;
+inport[[1]] = 1;
 Cannot select subfield of string field inport.
-ip.proto[1] = 1;
+ip.proto[[1]] = 1;
 Cannot select subfield of nominal field ip.proto.
-eth.dst[40] == 1;
+eth.dst[[40]] == 1;
 Syntax error at `==' expecting `=' or `<->'.
 ip = 1;
 Predicate symbol ip used where lvalue required.
@@ -1080,50 +1080,50 @@ vlan.present = 0;
 # Moving one field into another.
 reg0=reg1;
 formats as reg0 = reg1;
-encodes as move:NXM_NX_XXREG0[64..95]->NXM_NX_XXREG0[96..127]
-vlan.pcp = reg0[0..2];
-encodes as move:NXM_NX_XXREG0[96..98]->NXM_OF_VLAN_TCI[13..15]
-has prereqs vlan.tci[12]
-reg0[10] = vlan.pcp[1];
-encodes as move:NXM_OF_VLAN_TCI[14]->NXM_NX_XXREG0[106]
-has prereqs vlan.tci[12]
+encodes as move:NXM_NX_XXREG0[[64..95]]->NXM_NX_XXREG0[[96..127]]
+vlan.pcp = reg0[[0..2]];
+encodes as move:NXM_NX_XXREG0[[96..98]]->NXM_OF_VLAN_TCI[[13..15]]
+has prereqs vlan.tci[[12]]
+reg0[[10]] = vlan.pcp[[1]];
+encodes as move:NXM_OF_VLAN_TCI[[14]]->NXM_NX_XXREG0[[106]]
+has prereqs vlan.tci[[12]]
 outport = inport;
-encodes as move:NXM_NX_REG14[]->NXM_NX_REG15[]
+encodes as move:NXM_NX_REG14[[]]->NXM_NX_REG15[[]]
 
-reg0[0] = vlan.present;
+reg0[[0]] = vlan.present;
 Predicate symbol vlan.present used where lvalue required.
-reg0 = reg1[0..10];
+reg0 = reg1[[0..10]];
 Can't assign 11-bit value to 32-bit destination.
 inport = reg0;
 Can't assign integer field (reg0) to string field (inport).
 inport = big_string;
 String fields inport and big_string are incompatible for assignment.
-ip.proto = reg0[0..7];
+ip.proto = reg0[[0..7]];
 Field ip.proto is not modifiable.
 
 # Exchanging fields.
 reg0 <-> reg1;
-encodes as 
push:NXM_NX_XXREG0[64..95],push:NXM_NX_XXREG0[96..127],pop:NXM_NX_XXREG0[64..95],pop:NXM_NX_XXREG0[96..127]
-vlan.pcp <-> reg0[0..2];
-encodes as 
push:NXM_NX_XXREG0[96..98],push:NXM_OF_VLAN_TCI[13..15],pop:NXM_NX_XXREG0[96..98],pop:NXM_OF_VLAN_TCI[13..15]
-has prereqs vlan.tci[12]
-reg0[10] <-> vlan.pcp[1];
-encodes as 
push:NXM_OF_VLAN_TCI[14],push:NXM_NX_XXREG0[106],pop:NXM_OF_VLAN_TCI[14],pop:NXM_NX_XXREG0[106]
-has prereqs vlan.tci[12]
+encodes as 
push:NXM_NX_XXREG0[[64..95]],push:NXM_NX_XXREG0[[96..127]],pop:NXM_NX_XXREG0[[64..95]],pop:NXM_NX_XXREG0[[96..127]]
+vlan.pcp <-> reg0[[0..2]];
+encodes as 
push:NXM_NX_XXREG0[[96..98]],push:NXM_OF_VLAN_TCI[[13..15]],pop:NXM_NX_XXREG0[[96..98]],pop:NXM_OF_VLAN_TCI[[13..15]]
+has prereqs vlan.tci[[12]]
+reg0[[10]] <-> vlan.pcp[[1]];
+encodes as 
push:NXM_OF_VLAN_TCI[[14]],push:NXM_NX_XXREG0[[106]],pop:NXM_OF_VLAN_TCI[[14]],pop:NXM_NX_XXREG0[[106]]
+has prereqs vlan.tci[[12]]
 outport <-> inport;
-encodes as 
push:NXM_NX_REG14[],push:NXM_NX_REG15[],pop:NXM_NX_REG14[],pop:NXM_NX_REG15[]
+encodes as 
push:NXM_NX_REG14[[]],push:NXM_NX_REG15[[]],pop:NXM_NX_REG14[[]],pop:NXM_NX_REG15[[]]
 
-reg0[0] <-> vlan.present;
+reg0[[0]] <-> vlan.present;
 Predicate symbol vlan.present used where lvalue required.
-reg0 <-> reg1[0..10];
+reg0 <-> reg1[[0..10]];
 Can't exchange 32-bit field with 11-bit field.
 inport <-> reg0;
 Can't exchange string field (inport) with integer field (reg0).
 inport <-> big_string;
 String fields inport and big_string are incompatible for exchange.
-ip.proto <-> reg0[0..

Re: [ovs-dev] [PATCH ovn v3] tests: Remove table numbers from "action parsing".

[Xavier Simonart]

Hi Mark

Sorry, drop this patch - missing the 1/2.
Will resend.
Thanks
Xavier

On Fri, Mar 15, 2024 at 7:03 PM Xavier Simonart  wrote:

> This patch uses the recently introduced macros defining openflow table
> numbers.
>
> Signed-off-by: Xavier Simonart 
>
> ---
> v2: - Handled Ales' comments (i.e. fix few remaining hard-coded numbers)
> - Rebase on origin/main
> v3: - Rebase on origin/main
> ---
>  tests/ovn-macros.at |   4 +
>  tests/ovn.at| 245 +++-
>  2 files changed, 132 insertions(+), 117 deletions(-)
>
> diff --git a/tests/ovn-macros.at b/tests/ovn-macros.at
> index 3410afb74..45ec06c02 100644
> --- a/tests/ovn-macros.at
> +++ b/tests/ovn-macros.at
> @@ -1042,6 +1042,8 @@ m4_define([OFTABLE_OUTPUT_LARGE_PKT_DETECT], [37])
>  m4_define([OFTABLE_OUTPUT_LARGE_PKT_PROCESS], [38])
>  m4_define([OFTABLE_REMOTE_OUTPUT], [39])
>  m4_define([OFTABLE_LOCAL_OUTPUT], [40])
> +m4_define([OFTABLE_LOG_EGRESS_PIPELINE], [42])
> +m4_define([OFTABLE_SAVE_INPORT], [64])
>  m4_define([OFTABLE_LOG_TO_PHY], [65])
>  m4_define([OFTABLE_MAC_BINDING], [66])
>  m4_define([OFTABLE_MAC_LOOKUP], [67])
> @@ -1057,3 +1059,5 @@ m4_define([OFTABLE_ECMP_NH_MAC], [76])
>  m4_define([OFTABLE_ECMP_NH], [77])
>  m4_define([OFTABLE_CHK_LB_AFFINITY], [78])
>  m4_define([OFTABLE_MAC_CACHE_USE], [79])
> +
> +m4_define([OFTABLE_SAVE_INPORT_HEX], [m4_eval(OFTABLE_SAVE_INPORT, 16)])
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 2a5417685..de103a5bf 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -979,6 +979,17 @@ AT_CLEANUP
>  AT_SETUP([action parsing])
>  dnl Unindented text is input (a set of OVN logical actions).
>  dnl Indented text is expected output.
> +
> +# lflow table hard-coded to 10 in test-ovn, so next is 11.
> +m4_define([lflow_table], [11])
> +
> +m4_define([NEXT], [m4_if(
> +$1, ingress, [m4_eval($2 + OFTABLE_LOG_INGRESS_PIPELINE)],
> +$1, egress, [m4_eval($2 + OFTABLE_LOG_EGRESS_PIPELINE)])])
> +
> +m4_define([oflow_in_table], [NEXT(ingress, lflow_table)])
> +m4_define([oflow_out_table], [NEXT(egress, lflow_table)])
> +
>  AT_DATA([test-cases.txt], [
>  # drop
>  drop;
> @@ -990,18 +1001,18 @@ next; drop;
>
>  # output
>  output;
> -encodes as resubmit(,64)
> +encodes as resubmit(,OFTABLE_SAVE_INPORT)
>
>  # next
>  next;
> -encodes as resubmit(,19)
> -next(11);
> +encodes as resubmit(,oflow_in_table)
> +next(lflow_table);
>  formats as next;
> -encodes as resubmit(,19)
> +encodes as resubmit(,oflow_in_table)
>  next(0);
> -encodes as resubmit(,8)
> +encodes as resubmit(,OFTABLE_LOG_INGRESS_PIPELINE)
>  next(23);
> -encodes as resubmit(,31)
> +encodes as resubmit(,NEXT(ingress, 23))
>
>  next();
>  Syntax error at `)' expecting "pipeline" or "table".
> @@ -1010,29 +1021,29 @@ next(10;
>  next(24);
>  "next" action cannot advance beyond table 23.
>
> -next(table=11);
> +next(table=lflow_table);
>  formats as next;
> -encodes as resubmit(,19)
> +encodes as resubmit(,oflow_in_table)
>  next(pipeline=ingress);
>  formats as next;
> -encodes as resubmit(,19)
> -next(table=11, pipeline=ingress);
> +encodes as resubmit(,oflow_in_table)
> +next(table=lflow_table, pipeline=ingress);
>  formats as next;
> -encodes as resubmit(,19)
> -next(pipeline=ingress, table=11);
> +encodes as resubmit(,oflow_in_table)
> +next(pipeline=ingress, table=lflow_table);
>  formats as next;
> -encodes as resubmit(,19)
> +encodes as resubmit(,oflow_in_table)
>
>  next(pipeline=egress);
> -formats as next(pipeline=egress, table=11);
> -encodes as resubmit(,53)
> +formats as next(pipeline=egress, table=lflow_table);
> +encodes as resubmit(,oflow_out_table)
>
>  next(pipeline=egress, table=5);
> -encodes as resubmit(,47)
> +encodes as resubmit(,NEXT(egress, 5))
>
>  next(table=10);
>  formats as next(10);
> -encodes as resubmit(,18)
> +encodes as resubmit(,NEXT(ingress, 10))
>
>  # Loading a constant value.
>  tcp.dst=80;
> @@ -1054,7 +1065,7 @@ ip.ttl=4;
>  has prereqs eth.type == 0x800 || eth.type == 0x86dd
>  outport="eth0"; next; outport="LOCAL"; next;
>  formats as outport = "eth0"; next; outport = "LOCAL"; next;
> -encodes as
> set_field:0x5->reg15,resubmit(,19),set_field:0xfffe->reg15,resubmit(,19)
> +encodes as
> set_field:0x5->reg15,resubmit(,oflow_in_table),set_field:0xfffe->reg15,resubmit(,oflow_in_table)
>
>  inport[[1]] = 1;
>  Cannot select subfield of string field inport.
> @@ -1152,35 +1163,35 @@ pkt.mark = "foo";
>
>  # load balancing.
>  ct_lb;
> -encodes as ct(table=19,zone=NXM_NX_REG13[[0..15]],nat)
> +encodes as ct(table=oflow_in_table,zone=NXM_NX_REG13[[0..15]],nat)
>  has prereqs ip
>  ct_lb();
>  formats as ct_lb;
> -encodes as ct(table=19,zone=NXM_NX_REG13[[0..15]],nat)
> +encodes as ct(table=oflow_in_table,zone=NXM_NX_REG13[[0..15]],nat)
>  has prereqs ip
>  ct_lb(192.168.1.2:80, 192.

[ovs-dev] [PATCH ovn v3] tests: Remove table numbers from "action parsing".

[Xavier Simonart]

This patch uses the recently introduced macros defining openflow table numbers.

Signed-off-by: Xavier Simonart 

---
v2: - Handled Ales' comments (i.e. fix few remaining hard-coded numbers)
- Rebase on origin/main
v3: - Rebase on origin/main
---
 tests/ovn-macros.at |   4 +
 tests/ovn.at| 245 +++-
 2 files changed, 132 insertions(+), 117 deletions(-)

diff --git a/tests/ovn-macros.at b/tests/ovn-macros.at
index 3410afb74..45ec06c02 100644
--- a/tests/ovn-macros.at
+++ b/tests/ovn-macros.at
@@ -1042,6 +1042,8 @@ m4_define([OFTABLE_OUTPUT_LARGE_PKT_DETECT], [37])
 m4_define([OFTABLE_OUTPUT_LARGE_PKT_PROCESS], [38])
 m4_define([OFTABLE_REMOTE_OUTPUT], [39])
 m4_define([OFTABLE_LOCAL_OUTPUT], [40])
+m4_define([OFTABLE_LOG_EGRESS_PIPELINE], [42])
+m4_define([OFTABLE_SAVE_INPORT], [64])
 m4_define([OFTABLE_LOG_TO_PHY], [65])
 m4_define([OFTABLE_MAC_BINDING], [66])
 m4_define([OFTABLE_MAC_LOOKUP], [67])
@@ -1057,3 +1059,5 @@ m4_define([OFTABLE_ECMP_NH_MAC], [76])
 m4_define([OFTABLE_ECMP_NH], [77])
 m4_define([OFTABLE_CHK_LB_AFFINITY], [78])
 m4_define([OFTABLE_MAC_CACHE_USE], [79])
+
+m4_define([OFTABLE_SAVE_INPORT_HEX], [m4_eval(OFTABLE_SAVE_INPORT, 16)])
diff --git a/tests/ovn.at b/tests/ovn.at
index 2a5417685..de103a5bf 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -979,6 +979,17 @@ AT_CLEANUP
 AT_SETUP([action parsing])
 dnl Unindented text is input (a set of OVN logical actions).
 dnl Indented text is expected output.
+
+# lflow table hard-coded to 10 in test-ovn, so next is 11.
+m4_define([lflow_table], [11])
+
+m4_define([NEXT], [m4_if(
+$1, ingress, [m4_eval($2 + OFTABLE_LOG_INGRESS_PIPELINE)],
+$1, egress, [m4_eval($2 + OFTABLE_LOG_EGRESS_PIPELINE)])])
+
+m4_define([oflow_in_table], [NEXT(ingress, lflow_table)])
+m4_define([oflow_out_table], [NEXT(egress, lflow_table)])
+
 AT_DATA([test-cases.txt], [
 # drop
 drop;
@@ -990,18 +1001,18 @@ next; drop;
 
 # output
 output;
-encodes as resubmit(,64)
+encodes as resubmit(,OFTABLE_SAVE_INPORT)
 
 # next
 next;
-encodes as resubmit(,19)
-next(11);
+encodes as resubmit(,oflow_in_table)
+next(lflow_table);
 formats as next;
-encodes as resubmit(,19)
+encodes as resubmit(,oflow_in_table)
 next(0);
-encodes as resubmit(,8)
+encodes as resubmit(,OFTABLE_LOG_INGRESS_PIPELINE)
 next(23);
-encodes as resubmit(,31)
+encodes as resubmit(,NEXT(ingress, 23))
 
 next();
 Syntax error at `)' expecting "pipeline" or "table".
@@ -1010,29 +1021,29 @@ next(10;
 next(24);
 "next" action cannot advance beyond table 23.
 
-next(table=11);
+next(table=lflow_table);
 formats as next;
-encodes as resubmit(,19)
+encodes as resubmit(,oflow_in_table)
 next(pipeline=ingress);
 formats as next;
-encodes as resubmit(,19)
-next(table=11, pipeline=ingress);
+encodes as resubmit(,oflow_in_table)
+next(table=lflow_table, pipeline=ingress);
 formats as next;
-encodes as resubmit(,19)
-next(pipeline=ingress, table=11);
+encodes as resubmit(,oflow_in_table)
+next(pipeline=ingress, table=lflow_table);
 formats as next;
-encodes as resubmit(,19)
+encodes as resubmit(,oflow_in_table)
 
 next(pipeline=egress);
-formats as next(pipeline=egress, table=11);
-encodes as resubmit(,53)
+formats as next(pipeline=egress, table=lflow_table);
+encodes as resubmit(,oflow_out_table)
 
 next(pipeline=egress, table=5);
-encodes as resubmit(,47)
+encodes as resubmit(,NEXT(egress, 5))
 
 next(table=10);
 formats as next(10);
-encodes as resubmit(,18)
+encodes as resubmit(,NEXT(ingress, 10))
 
 # Loading a constant value.
 tcp.dst=80;
@@ -1054,7 +1065,7 @@ ip.ttl=4;
 has prereqs eth.type == 0x800 || eth.type == 0x86dd
 outport="eth0"; next; outport="LOCAL"; next;
 formats as outport = "eth0"; next; outport = "LOCAL"; next;
-encodes as 
set_field:0x5->reg15,resubmit(,19),set_field:0xfffe->reg15,resubmit(,19)
+encodes as 
set_field:0x5->reg15,resubmit(,oflow_in_table),set_field:0xfffe->reg15,resubmit(,oflow_in_table)
 
 inport[[1]] = 1;
 Cannot select subfield of string field inport.
@@ -1152,35 +1163,35 @@ pkt.mark = "foo";
 
 # load balancing.
 ct_lb;
-encodes as ct(table=19,zone=NXM_NX_REG13[[0..15]],nat)
+encodes as ct(table=oflow_in_table,zone=NXM_NX_REG13[[0..15]],nat)
 has prereqs ip
 ct_lb();
 formats as ct_lb;
-encodes as ct(table=19,zone=NXM_NX_REG13[[0..15]],nat)
+encodes as ct(table=oflow_in_table,zone=NXM_NX_REG13[[0..15]],nat)
 has prereqs ip
 ct_lb(192.168.1.2:80, 192.168.1.3:80);
 Syntax error at `192.168.1.2' expecting backends.
 ct_lb(backends=192.168.1.2:80,192.168.1.3:80);
 encodes as group:1
-uses group: id(1), 
name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:100,actions=ct(nat(dst=192.168.1.2:80),commit,table=19,zone=NXM_NX_REG13[[0..15]],exec(set_field:2/2->ct_label)),bucket=bucket_id=1,weight:100,actions=ct(nat(dst=192.16

Re: [ovs-dev] [PATCH ovn v5 3/4] utilities: Add ovn-debug binary tool.

[0-day Robot]

Bleep bloop.  Greetings Ales Musil, 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: Line lacks whitespace around operator
#283 FILE: utilities/ovn-debug.c:87:
lflow-stage-to-ltable STAGE_NAME\n\

WARNING: Line lacks whitespace around operator
#285 FILE: utilities/ovn-debug.c:89:
lflow-stage-to-oftable STAGE_NAME\n\

Lines checked: 355, Warnings: 2, Errors: 0


Please check this out.  If you feel there has been an error, please email 
acon...@redhat.com

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn v5 2/4] checkpatch: Add rule to check for hardcoded table numbers.

[0-day Robot]

Bleep bloop.  Greetings Ales Musil, 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: Use of hardcoded table= or resubmit=(,) is discouraged 
in tests. Consider using MACRO instead.
#29 FILE: tests/checkpatch.at:612:
+table=12(ls_in_hairpin  ), priority=1000 , match=(reg0[[14]] == 1), 
action=(next(pipeline=ingress, table=??);)

WARNING: Use of hardcoded table= or resubmit=(,) is discouraged 
in tests. Consider using MACRO instead.
#33 FILE: tests/checkpatch.at:616:
table=12(ls_in_hairpin  ), priority=1000 , match=(reg0[[14]] == 1), 
action=(next(pipeline=ingress, table=??);)

WARNING: Use of hardcoded table= or resubmit=(,) is discouraged 
in tests. Consider using MACRO instead.
#38 FILE: tests/checkpatch.at:621:
+table=??(ls_in_hairpin  ), priority=1000 , match=(reg0[[14]] == 1), 
action=(next(pipeline=ingress, table=13);)

WARNING: Use of hardcoded table= or resubmit=(,) is discouraged 
in tests. Consider using MACRO instead.
#42 FILE: tests/checkpatch.at:625:
table=??(ls_in_hairpin  ), priority=1000 , match=(reg0[[14]] == 1), 
action=(next(pipeline=ingress, table=13);)

WARNING: Use of hardcoded table= or resubmit=(,) is discouraged 
in tests. Consider using MACRO instead.
#47 FILE: tests/checkpatch.at:630:
+priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.1 
actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,47)

WARNING: Use of hardcoded table= or resubmit=(,) is discouraged 
in tests. Consider using MACRO instead.
#51 FILE: tests/checkpatch.at:634:
priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.1 
actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,47)

WARNING: Use of hardcoded table= or resubmit=(,) is discouraged 
in tests. Consider using MACRO instead.
#56 FILE: tests/checkpatch.at:639:
+C_H_E_C_K([as gw1 ovs-ofctl dump-flows br-int table=42 | grep 
"dl_dst=00:00:02:01:02:04" | wc -l], [0], [[1]])

WARNING: Use of hardcoded table= or resubmit=(,) is discouraged 
in tests. Consider using MACRO instead.
#60 FILE: tests/checkpatch.at:643:
C_H_E_C_K([as gw1 ovs-ofctl dump-flows br-int table=42 | grep 
"dl_dst=00:00:02:01:02:04" | wc -l], [0], [[1]])

Lines checked: 104, Warnings: 8, Errors: 0


Please check this out.  If you feel there has been an error, please email 
acon...@redhat.com

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v11 4/5] dpif: Probe support for OVS_ACTION_ATTR_DROP.

[Eric Garver]

On Fri, Mar 15, 2024 at 12:37:54PM -0400, Eric Garver wrote:
> On Fri, Mar 15, 2024 at 04:55:50PM +0100, Eelco Chaudron wrote:
> > 
> > 
> > On 11 Mar 2024, at 18:51, Eric Garver wrote:
> > 
> > > Kernel support has been added for this action. As such, we need to probe
> > > the datapath for support.
> > >
> > > Signed-off-by: Eric Garver 
> > 
> > Thanks for submitting the changes requested. Some comments below.
> > 
> > Cheers,
> > 
> > Eelco
> > 
> > 
> > > ---
> > >  include/linux/openvswitch.h |  2 +-
> > >  lib/dpif.c  |  6 ++-
> > >  lib/dpif.h  |  2 +-
> > >  ofproto/ofproto-dpif.c  | 77 ++---
> > >  ofproto/ofproto-dpif.h  |  4 +-
> > >  5 files changed, 81 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
> > > index a265e05ad253..fce6456f47c8 100644
> > > --- a/include/linux/openvswitch.h
> > > +++ b/include/linux/openvswitch.h
> > > @@ -1086,11 +1086,11 @@ enum ovs_action_attr {
> > >   OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */
> > >   OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */
> > >   OVS_ACTION_ATTR_DEC_TTL,  /* Nested OVS_DEC_TTL_ATTR_*. */
> > > + OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */
> > >
> > >  #ifndef __KERNEL__
> > >   OVS_ACTION_ATTR_TUNNEL_PUSH,   /* struct ovs_action_push_tnl*/
> > >   OVS_ACTION_ATTR_TUNNEL_POP,/* u32 port number. */
> > > - OVS_ACTION_ATTR_DROP,  /* u32 xlate_error. */
> > >   OVS_ACTION_ATTR_LB_OUTPUT, /* u32 bond-id. */
> > >  #endif
> > >   __OVS_ACTION_ATTR_MAX,/* Nothing past this will be accepted
> > > diff --git a/lib/dpif.c b/lib/dpif.c
> > > index 0f480bec48d0..23eb18495a66 100644
> > > --- a/lib/dpif.c
> > > +++ b/lib/dpif.c
> > > @@ -28,6 +28,7 @@
> > >  #include "dpctl.h"
> > >  #include "dpif-netdev.h"
> > >  #include "flow.h"
> > > +#include "netdev-offload.h"
> > >  #include "netdev-provider.h"
> > >  #include "netdev.h"
> > >  #include "netlink.h"
> > > @@ -1935,9 +1936,10 @@ dpif_supports_tnl_push_pop(const struct dpif *dpif)
> > >  }
> > >
> > >  bool
> > > -dpif_supports_explicit_drop_action(const struct dpif *dpif)
> > > +dpif_may_support_explicit_drop_action(const struct dpif *dpif)
> > >  {
> > > -return dpif_is_netdev(dpif);
> > > +/* TC does not support offloading this action. */
> > > +return dpif_is_netdev(dpif) || !netdev_is_flow_api_enabled();
> > >  }
> > >
> > >  bool
> > > diff --git a/lib/dpif.h b/lib/dpif.h
> > > index 0f2dc2ef3c55..a764e8a592bd 100644
> > > --- a/lib/dpif.h
> > > +++ b/lib/dpif.h
> > > @@ -940,7 +940,7 @@ int dpif_get_pmds_for_port(const struct dpif * dpif, 
> > > odp_port_t port_no,
> > >
> > >  char *dpif_get_dp_version(const struct dpif *);
> > >  bool dpif_supports_tnl_push_pop(const struct dpif *);
> > > -bool dpif_supports_explicit_drop_action(const struct dpif *);
> > > +bool dpif_may_support_explicit_drop_action(const struct dpif *);
> > >  bool dpif_synced_dp_layers(struct dpif *);
> > >
> > >  /* Log functions. */
> > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > > index 2ec1cd1854ab..7bacd2120c78 100644
> > > --- a/ofproto/ofproto-dpif.c
> > > +++ b/ofproto/ofproto-dpif.c
> > > @@ -221,6 +221,7 @@ static void ct_zone_config_init(struct dpif_backer 
> > > *backer);
> > >  static void ct_zone_config_uninit(struct dpif_backer *backer);
> > >  static void ct_zone_timeout_policy_sweep(struct dpif_backer *backer);
> > >  static void ct_zone_limits_commit(struct dpif_backer *backer);
> > > +static bool recheck_support_explicit_drop_action(struct dpif_backer 
> > > *backer);
> > >
> > >  static inline struct ofproto_dpif *
> > >  ofproto_dpif_cast(const struct ofproto *ofproto)
> > > @@ -391,6 +392,10 @@ type_run(const char *type)
> > >  udpif_set_threads(backer->udpif, n_handlers, n_revalidators);
> > >  }
> > >
> > > +if (recheck_support_explicit_drop_action(backer)) {
> > > +backer->need_revalidate = REV_RECONFIGURE;
> > > +}
> > > +
> > >  if (backer->need_revalidate) {
> > >  struct ofproto_dpif *ofproto;
> > >  struct simap_node *node;
> > > @@ -807,6 +812,7 @@ open_dpif_backer(const char *type, struct dpif_backer 
> > > **backerp)
> > >
> > >  shash_add(&all_dpif_backers, type, backer);
> > >
> > > +atomic_init(&backer->rt_support.explicit_drop_action, false);
> > >  check_support(backer);
> > >  atomic_count_init(&backer->tnl_count, 0);
> > >
> > > @@ -855,7 +861,10 @@ ovs_native_tunneling_is_on(struct ofproto_dpif 
> > > *ofproto)
> > >  bool
> > >  ovs_explicit_drop_action_supported(struct ofproto_dpif *ofproto)
> > >  {
> > > -return ofproto->backer->rt_support.explicit_drop_action;
> > > +bool value;
> > 
> > nit: I would add a new line here.
> 
> ACK.
> 
> > > +
> > > atomic_read_relaxed(&ofproto->backer->rt_support.explicit_drop_action,
> > > +   

Re: [ovs-dev] [PATCH ovn] tests: Fix flaky "lr multiple gw ports" test.

[Mark Michelson]

On 2/13/24 02:15, Ales Musil wrote:

On Mon, Feb 12, 2024 at 5:27 PM Xavier Simonart  wrote:


The test was confused as 192.168.0.1 was configured for DR-S3 port
(configured as a gateway-chassis hv4) as well as the encap_ip for hv1.

Hence packets which were supposed to be sent towards hv1 got sent
to hv4.

Signed-off-by: Xavier Simonart 
---
  tests/ovn.at | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/ovn.at b/tests/ovn.at
index 902dd3793..7cf4d02d3 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -33322,7 +33322,7 @@ for i in 1 2 3 4 5; do
  as hv$i
  ovs-vsctl add-br br-phys
  ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
-ovn_attach n1 br-phys 192.168.0.$i 24 $encap
+ovn_attach n1 br-phys 192.168.1.$i 24 $encap
  done

  # Add a vif on HV1
--
2.41.0

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



Looks good to me, thanks.

Acked-by: Ales Musil 



Thanks Xavier and Ales. I pushed this to main, 24.03, 23.09, and 23.06.

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

[ovs-dev] [PATCH ovn v5 4/4] tests: Use the ovn-debug binary to determine table numbers.

[Ales Musil]

Use the ovn-debug commands to determine OpenFlow table numbers
based on stage name. With this there is no need to hardcode them
and it should be future proof for stage shifts/updates.

Signed-off-by: Ales Musil 
Acked-by: Mark Michelson 
---
v5: Rebase on top of main.
v4: Rebase on top of main.
Add ack from Mark.
---
 tests/ovn-controller.at  | 342 +++---
 tests/ovn.at | 389 ++-
 tests/system-ovn-kmod.at |  16 +-
 tests/system-ovn.at  |  20 +-
 4 files changed, 438 insertions(+), 329 deletions(-)

diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
index 37f1ded1b..fdcc5aab2 100644
--- a/tests/ovn-controller.at
+++ b/tests/ovn-controller.at
@@ -941,6 +941,10 @@ check ovn-nbctl lsp-add ls1 ls1-lp1 \
 wait_for_ports_up
 ovn-appctl -t ovn-controller vlog/set file:dbg
 
+# Get the OF table numbers
+acl_eval=$(ovn-debug lflow-stage-to-oftable ls_out_acl_eval)
+acl_action=$(ovn-debug lflow-stage-to-oftable ls_out_acl_action)
+
 dp_key=$(printf "%x" $(fetch_column datapath tunnel_key external_ids:name=ls1))
 port_key=$(printf "%x" $(fetch_column port_binding tunnel_key 
logical_port=ls1-lp1))
 
@@ -958,14 +962,14 @@ for i in $(seq 10); do
 check ovn-nbctl add address_set as1 addresses 10.0.0.$i
 check ovn-nbctl --wait=hv sync
 if test "$i" = 3; then
-AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int 
table=46,reg15=0x$port_key | \
+AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int 
table=$acl_eval,reg15=0x$port_key | \
 grep -v reply | awk '{print $7, $8}' | sort], [0], [dnl
-priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.1 
actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,47)
-priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.2 
actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,47)
-priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.3 
actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,47)
+priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.1 
actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,$acl_action)
+priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.2 
actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,$acl_action)
+priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.3 
actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,$acl_action)
 ])
 fi
-AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int table=46 | grep -c 
"priority=1100"], [0], [$i
+AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int table=$acl_eval | grep -c 
"priority=1100"], [0], [$i
 ])
 done
 
@@ -980,15 +984,15 @@ for i in $(seq 10); do
 check ovn-nbctl remove address_set as1 addresses 10.0.0.$i
 check ovn-nbctl --wait=hv sync
 if test "$i" = 9; then
-AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int 
table=46,reg15=0x$port_key | \
+AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int 
table=$acl_eval,reg15=0x$port_key | \
 grep -v reply | awk '{print $7, $8}'], [0], [dnl
-priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.10 
actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,47)
+priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.10 
actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,$acl_action)
 ])
 fi
 if test "$i" = 10; then
-AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int table=46 | grep 
"priority=1100"], [1], [ignore])
+AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int table=$acl_eval | grep 
"priority=1100"], [1], [ignore])
 else
-AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int table=46 | grep -c 
"priority=1100"], [0], [$((10 - $i))
+AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int table=$acl_eval | grep 
-c "priority=1100"], [0], [$((10 - $i))
 ])
 fi
 done
@@ -1006,17 +1010,17 @@ for i in $(seq 10); do
 check ovn-nbctl add address_set as1 addresses 10.0.0.$i,10.0.1.$i
 check ovn-nbctl --wait=hv sync
 if test "$i" = 3; then
-AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int 
table=46,reg15=0x$port_key | \
+AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int 
table=$acl_eval,reg15=0x$port_key | \
 grep -v reply | awk '{print $7, $8}' | sort], [0], [dnl
-priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.1 
actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,47)
-priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.2 
actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,47)
-priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.3 
actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,47)
-priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.1.1 
actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,47)
-priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.1.2 
actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,47)
-priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.1.3 
actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,47)
+priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw

[ovs-dev] [PATCH ovn v5 3/4] utilities: Add ovn-debug binary tool.

[Ales Musil]

Add ovn-debug binary tool that can be extended with commands that
might be useful for tests/debugging of OVN environment. Currently
the tool supports only two commands:

1) "lflow-stage-to-ltable STAGE_NAME" that converts stage name into
   logical flow table id.

2) "lflow-stage-to-oftable STAGE_NAME" that converts stage name into
   OpenFlow table id.

For now it will be used in tests to get rid remaining hardcoded table
numbers.

Signed-off-by: Ales Musil 
Acked-by: Mark Michelson 
---
v5: Rebase on top current main.
v4: Rebase on top current main.
Address nit from Dumitru.
---
 NEWS   |   5 ++
 README.rst |   1 +
 debian/ovn-common.install  |   1 +
 debian/ovn-common.manpages |   1 +
 rhel/ovn-fedora.spec.in|   2 +
 utilities/.gitignore   |   2 +
 utilities/automake.mk  |  10 ++-
 utilities/ovn-debug.8.xml  |  28 +++
 utilities/ovn-debug.c  | 155 +
 9 files changed, 204 insertions(+), 1 deletion(-)
 create mode 100644 utilities/ovn-debug.8.xml
 create mode 100644 utilities/ovn-debug.c

diff --git a/NEWS b/NEWS
index 125fde500..c2e01bfe7 100644
--- a/NEWS
+++ b/NEWS
@@ -5,6 +5,11 @@ Post v24.03.0
 cloned to all unknown ports connected to the same Logical Switch.
   - Added a new logical switch port option "disable_arp_nd_rsp" to
 disable adding the ARP responder flows if set to true.
+  - Add ovn-debug tool containing two commands.
+"lflow-stage-to-ltable STAGE_NAME" that converts stage name into logical
+flow table id.
+"lflow-stage-to-oftable STAGE_NAME" that converts stage name into OpenFlow
+table id.
 
 OVN v24.03.0 - 01 Mar 2024
 --
diff --git a/README.rst b/README.rst
index 6fb717742..428cd8ee8 100644
--- a/README.rst
+++ b/README.rst
@@ -56,6 +56,7 @@ The main components of this distribution are:
 - ovn-sbctl, a tool for interfacing with the southbound database.
 - ovn-trace, a debugging utility that allows for tracing of packets through
   the logical network.
+- ovn-debug, a tool to simplify debugging of OVN setup.
 - Scripts and specs for building RPMs.
 
 What other documentation is available?
diff --git a/debian/ovn-common.install b/debian/ovn-common.install
index 050d1c63a..fc48f07e4 100644
--- a/debian/ovn-common.install
+++ b/debian/ovn-common.install
@@ -5,6 +5,7 @@ usr/bin/ovn-ic-nbctl
 usr/bin/ovn-ic-sbctl
 usr/bin/ovn-trace
 usr/bin/ovn_detrace.py
+usr/bin/ovn-debug
 usr/share/ovn/scripts/ovn-ctl
 usr/share/ovn/scripts/ovndb-servers.ocf
 usr/share/ovn/scripts/ovn-lib
diff --git a/debian/ovn-common.manpages b/debian/ovn-common.manpages
index 1fa3d9cb3..e864512e3 100644
--- a/debian/ovn-common.manpages
+++ b/debian/ovn-common.manpages
@@ -11,3 +11,4 @@ utilities/ovn-ic-nbctl.8
 utilities/ovn-ic-sbctl.8
 utilities/ovn-trace.8
 utilities/ovn-detrace.1
+utilities/ovn-debug.8
diff --git a/rhel/ovn-fedora.spec.in b/rhel/ovn-fedora.spec.in
index 03c1f27c5..670f1ca9e 100644
--- a/rhel/ovn-fedora.spec.in
+++ b/rhel/ovn-fedora.spec.in
@@ -495,6 +495,7 @@ fi
 %{_bindir}/ovn-appctl
 %{_bindir}/ovn-ic-nbctl
 %{_bindir}/ovn-ic-sbctl
+%{_bindir}/ovn-debug
 %{_datadir}/ovn/scripts/ovn-ctl
 %{_datadir}/ovn/scripts/ovn-lib
 %{_datadir}/ovn/scripts/ovndb-servers.ocf
@@ -515,6 +516,7 @@ fi
 %{_mandir}/man8/ovn-ic.8*
 %{_mandir}/man5/ovn-ic-nb.5*
 %{_mandir}/man5/ovn-ic-sb.5*
+%{_mandir}/man8/ovn-debug.8*
 %{_prefix}/lib/ocf/resource.d/ovn/ovndb-servers
 %config(noreplace) %{_sysconfdir}/logrotate.d/ovn
 %{_unitdir}/ovn-db@.service
diff --git a/utilities/.gitignore b/utilities/.gitignore
index da237563b..3ae97b00f 100644
--- a/utilities/.gitignore
+++ b/utilities/.gitignore
@@ -13,6 +13,8 @@
 /ovn-trace.8
 /ovn_detrace.py
 /ovn-detrace.1
+/ovn-debug
+/ovn-debug.8
 /ovn-docker-overlay-driver
 /ovn-docker-underlay-driver
 /ovn-lib
diff --git a/utilities/automake.mk b/utilities/automake.mk
index ebb74ec34..de4f6efb5 100644
--- a/utilities/automake.mk
+++ b/utilities/automake.mk
@@ -11,7 +11,8 @@ man_MANS += \
 utilities/ovn-ic-sbctl.8 \
 utilities/ovn-trace.8 \
 utilities/ovn-detrace.1 \
-utilities/ovn-appctl.8
+utilities/ovn-appctl.8 \
+utilities/ovn-debug.8
 
 MAN_ROOTS += \
 utilities/ovn-detrace.1.in
@@ -34,6 +35,7 @@ EXTRA_DIST += \
 utilities/ovn-ic-sbctl.8.xml \
 utilities/ovn-appctl.8.xml \
 utilities/ovn-trace.8.xml \
+utilities/ovn-debug.8.xml \
 utilities/ovn_detrace.py.in \
 utilities/ovndb-servers.ocf \
 utilities/checkpatch.py \
@@ -63,6 +65,7 @@ CLEANFILES += \
 utilities/ovn-ic-nbctl.8 \
 utilities/ovn-ic-sbctl.8 \
 utilities/ovn-trace.8 \
+utilities/ovn-debug.8 \
 utilities/ovn-detrace.1 \
 utilities/ovn-detrace \
 utilities/ovn_detrace.py \
@@ -120,4 +123,9 @@ UNINSTALL_LOCAL += ovn-detrace-uninstall
 ovn-detrace-uninstall:
rm -f $(DESTDIR)$(bindir)/ovn-detrace
 
+# ovn-debug
+bin_PROGRAMS += utilities/ovn-debug
+utilities_ovn_debug_SOURCES = utilities/ovn-de

[ovs-dev] [PATCH ovn v5 2/4] checkpatch: Add rule to check for hardcoded table numbers.

[Ales Musil]

To avoid issues with hardcoded table numbers in future add rule
into check patch. The rule is only warning because there are still
legitimate use cases and not everything can be abstracted.

Signed-off-by: Ales Musil 
Acked-by: Mark Michelson 
---
v5: Rebase on top of main.
v4: Rebase on top of main.
Address comments from Dumitru:
- Fix the regex.
- Add test for the new check.
---
 tests/checkpatch.at | 39 +++
 utilities/checkpatch.py | 12 
 2 files changed, 51 insertions(+)

diff --git a/tests/checkpatch.at b/tests/checkpatch.at
index e7322fff4..6ac0e51f3 100755
--- a/tests/checkpatch.at
+++ b/tests/checkpatch.at
@@ -605,3 +605,42 @@ try_checkpatch \
 Subject: netdev: This is a way to long commit summary and therefor it 
should report a WARNING!"
 
 AT_CLEANUP
+
+AT_SETUP([checkpatch - hardcoded table numbers])
+try_checkpatch \
+   "COMMON_PATCH_HEADER([tests/something.at])
++table=12(ls_in_hairpin  ), priority=1000 , match=(reg0[[14]] == 1), 
action=(next(pipeline=ingress, table=??);)
+" \
+"WARNING: Use of hardcoded table= or resubmit=(,) is 
discouraged in tests. Consider using MACRO instead.
+#8 FILE: tests/something.at:1:
+table=12(ls_in_hairpin  ), priority=1000 , match=(reg0[[14]] == 1), 
action=(next(pipeline=ingress, table=??);)
+"
+
+try_checkpatch \
+   "COMMON_PATCH_HEADER([tests/something.at])
++table=??(ls_in_hairpin  ), priority=1000 , match=(reg0[[14]] == 1), 
action=(next(pipeline=ingress, table=13);)
+" \
+"WARNING: Use of hardcoded table= or resubmit=(,) is 
discouraged in tests. Consider using MACRO instead.
+#8 FILE: tests/something.at:1:
+table=??(ls_in_hairpin  ), priority=1000 , match=(reg0[[14]] == 1), 
action=(next(pipeline=ingress, table=13);)
+"
+
+try_checkpatch \
+   "COMMON_PATCH_HEADER([tests/something.at])
++priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.1 
actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,47)
+" \
+"WARNING: Use of hardcoded table= or resubmit=(,) is 
discouraged in tests. Consider using MACRO instead.
+#8 FILE: tests/something.at:1:
+priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.1 
actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,47)
+"
+
+try_checkpatch \
+   "COMMON_PATCH_HEADER([tests/something.at])
++C_H_E_C_K([as gw1 ovs-ofctl dump-flows br-int table=42 | grep 
"dl_dst=00:00:02:01:02:04" | wc -l], [0], [[1]])
+" \
+"WARNING: Use of hardcoded table= or resubmit=(,) is 
discouraged in tests. Consider using MACRO instead.
+#8 FILE: tests/something.at:1:
+C_H_E_C_K([as gw1 ovs-ofctl dump-flows br-int table=42 | grep 
"dl_dst=00:00:02:01:02:04" | wc -l], [0], [[1]])
+"
+
+AT_CLEANUP
diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index 52d3fa845..35204daa2 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -202,6 +202,7 @@ __regex_if_macros = re.compile(r'^ +(%s) 
\([\S]([\s\S]+[\S])*\) { +\\' %
__parenthesized_constructs)
 __regex_nonascii_characters = re.compile("[^\u-\u007f]")
 __regex_efgrep = re.compile(r'.*[ef]grep.*$')
+__regex_hardcoded_table = 
re.compile(r'.*(table=[0-9]+)|.*(resubmit\(,[0-9]+\))')
 
 skip_leading_whitespace_check = False
 skip_trailing_whitespace_check = False
@@ -371,6 +372,10 @@ def has_efgrep(line):
 """Returns TRUE if the current line contains 'egrep' or 'fgrep'."""
 return __regex_efgrep.match(line) is not None
 
+def has_hardcoded_table(line):
+"""Return TRUE if the current line contains table= or
+   resubmit(,)"""
+return __regex_hardcoded_table.match(line) is not None
 
 def filter_comments(current_line, keep=False):
 """remove all of the c-style comments in a line"""
@@ -656,6 +661,13 @@ checks = [
  'check': lambda x: has_efgrep(x),
  'print':
  lambda: print_error("grep -E/-F should be used instead of egrep/fgrep")},
+
+{'regex': r'\.at$', 'match_name': None,
+ 'check': lambda x: has_hardcoded_table(x),
+ 'print':
+ lambda: print_warning("Use of hardcoded table= or"
+   " resubmit=(,) is discouraged in tests."
+   " Consider using MACRO instead.")},
 ]
 
 
-- 
2.44.0

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

[ovs-dev] [PATCH ovn v5 1/4] tests: Remove hardcoded numbers from comments.

[Ales Musil]

There were some comments left with hardcoded numbers. Even if it
wouldn't break any test table shift/change it would just leave the
comment outdated.

Signed-off-by: Ales Musil 
Acked-by: Mark Michelson 
---
v5: Rebase on top of main.
v4: Rebase on top of main.
Align the northd.at comment with others.
---
 tests/ovn-northd.at | 2 +-
 tests/ovn.at| 8 
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 89aed5adc..7893b0540 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -2144,7 +2144,7 @@ AT_CLEANUP
 
 # This test case tests that when a logical switch has load balancers associated
 # (with VIPs configured), the below logical flow is added by ovn-northd.
-# table=1 (ls_out_pre_lb  ), priority=100  , match=(ip), action=(reg0[[0]] 
= 1; next;)
+# table=ls_out_pre_lb, priority=100, match=(ip), action=(reg0[[0]] = 1; next;)
 # This test case is added for the BZ -
 # https://bugzilla.redhat.com/show_bug.cgi?id=1849162
 #
diff --git a/tests/ovn.at b/tests/ovn.at
index d49f89f30..f8bde6430 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -18499,8 +18499,8 @@ AT_CHECK([cat 2.packets], [0], [expout])
 
 # There should be total of 9 flows present with conjunction action and 2 flows
 # with conj match. Eg.
-# table=44, priority=2001,conj_id=2,metadata=0x1 actions=resubmit(,45)
-# table=44, priority=2001,conj_id=3,metadata=0x1 actions=drop
+# table=ls_out_acl_eval, priority=2001,conj_id=2,metadata=0x1 
actions=resubmit(,ls_out_acl_action)
+# table=ls_out_acl_eval, priority=2001,conj_id=3,metadata=0x1 actions=drop
 # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.6 actions=conjunction(2,2/2)
 # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(2,2/2)
 # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.5 actions=conjunction(2,2/2)
@@ -18540,7 +18540,7 @@ AT_CHECK([cat 2.packets], [0], [])
 # properly.
 # There should be total of 6 flows present with conjunction action and 1 flow
 # with conj match. Eg.
-# table=44, priority=2001,conj_id=3,metadata=0x1 actions=drop
+# table=ls_out_acl_eval, priority=2001,conj_id=3,metadata=0x1 actions=drop
 # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.7 actions=conjunction(4,2/2)
 # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9 actions=conjunction(4,2/2)
 # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8 actions=conjunction(4,2/2)
@@ -34577,7 +34577,7 @@ in_port_sec=OFTABLE_CHK_IN_PORT_SEC
 in_port_sec_nd=OFTABLE_CHK_IN_PORT_SEC_ND
 out_port_sec=OFTABLE_CHK_OUT_PORT_SEC
 
-# There should be no flows in table OFTABLE_CHK_IN_PORT_SEC, 74 and 75 in hv1 
and hv2
+# There should be no flows in table OFTABLE_CHK_IN_PORT_SEC, 
OFTABLE_CHK_IN_PORT_SEC_ND and OFTABLE_CHK_OUT_PORT_SEC in hv1 and hv2
 > hv1_t${in_port_sec}_flows.expected
 > hv1_t${in_port_sec_nd}_flows.expected
 > hv1_t${out_port_sec}_flows.expected
-- 
2.44.0

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

[ovs-dev] [PATCH ovn v5 0/4] Remove most of the hardcoded table numbers

[Ales Musil]

This series has the remaing patches that weren't yet
accepted with addition that fixes table numbers in comments.

A lot of tests were relying on exact table numbers for
flow comparison either logical or OpenFlows.

In order to take care of table references into OpenFlow through logical
flow stage, new binary is added called ovn-debug. The new binary provides
command that will translate stage name into OpenFlow table number.

Only test that is still hardcoded is "action parsing", because
it compares the direct output from the test binary.

Ales Musil (4):
  tests: Remove hardcoded numbers from comments.
  checkpatch: Add rule to check for hardcoded table numbers.
  utilities: Add ovn-debug binary tool.
  tests: Use the ovn-debug binary to determine table numbers.

 NEWS   |   5 +
 README.rst |   1 +
 debian/ovn-common.install  |   1 +
 debian/ovn-common.manpages |   1 +
 rhel/ovn-fedora.spec.in|   2 +
 tests/checkpatch.at|  39 
 tests/ovn-controller.at| 342 ++--
 tests/ovn-northd.at|   2 +-
 tests/ovn.at   | 397 +
 tests/system-ovn-kmod.at   |  16 +-
 tests/system-ovn.at|  20 +-
 utilities/.gitignore   |   2 +
 utilities/automake.mk  |  10 +-
 utilities/checkpatch.py|  12 ++
 utilities/ovn-debug.8.xml  |  28 +++
 utilities/ovn-debug.c  | 155 +++
 16 files changed, 698 insertions(+), 335 deletions(-)
 create mode 100644 utilities/ovn-debug.8.xml
 create mode 100644 utilities/ovn-debug.c

-- 
2.44.0

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

Re: [ovs-dev] [PATCH ovn] pinctrl: Fix prefix delegation.

[Mark Michelson]

On 2/13/24 02:21, Ales Musil wrote:

On Mon, Feb 12, 2024 at 5:12 PM Xavier Simonart  wrote:


Prefix delegation suffered from potential issues:
- An unicast SOLICIT message was sometimes sent instead of a RENEW.
- Pînctrl was sometimes not waking up as expected when timers such
   as T1 or T2 expired.
- If reply was not received for a REQUEST, no new SOLICIT was sent.
- State update to RENEW, done by ipv6_prefixd_should_inject, was
   sometimes done before send_ipv6_prefixd (as part of may_inject),
   and sometimes not, depending for instance of garp. This influenced
   how T1 and T2 were calculated. State change was also influenced
   by other prefixes (e.g. another prefix in SOLICIT might skip an
   update to RENEW).

This patch fixes those issue by:
[1] Do not try to send RENEW in PENDING state as this resulted in
   SOLICIT sent unicast.
[2] Make sure to wake up in DONE, PENDING, RENEW and REBIND states.
[3] Make sure to wake up and send REQUEST in REQUEST state in case
   no reply received.
[4] Ensure that RENEW and REBIND states are updated indeoendently of
   other network elements such as garps or other prefixes.

The patch also modifies the "IPv6 prefix delegation" system test to
test scenarios 1 and 2, and reduces renewal-time to reduce test
duration.

This issue was highlighted by frequent failures of "IPv6 prefix
delegation" test in Build_and_Test ovsrobot.

Signed-off-by: Xavier Simonart 
---
  controller/pinctrl.c  | 38 +--
  tests/system-common-macros.at | 38 ++-
  2 files changed, 56 insertions(+), 20 deletions(-)

diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index 98b29de9f..3f041b295 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -1171,7 +1171,7 @@ ipv6_prefixd_send(struct rconn *swconn, struct
ipv6_prefixd_state *pfd)
  return pfd->next_announce;
  }

-if (pfd->state == PREFIX_DONE) {
+if ((pfd->state == PREFIX_PENDING) || (pfd->state == PREFIX_DONE)) {
  goto out;
  }

@@ -1222,33 +1222,58 @@ static bool ipv6_prefixd_should_inject(void)
  struct ipv6_prefixd_state *pfd = iter->data;
  long long int cur_time = time_msec();

-if (pfd->state == PREFIX_SOLICIT) {
+if (pfd->state == PREFIX_SOLICIT || pfd->state == PREFIX_REQUEST)
{
  return true;
  }
  if (pfd->state == PREFIX_DONE &&
  cur_time > pfd->last_complete + pfd->t1) {
-pfd->state = PREFIX_RENEW;
  return true;
  }
  if (pfd->state == PREFIX_RENEW &&
  cur_time > pfd->last_complete + pfd->t2) {
-pfd->state = PREFIX_REBIND;
  pfd->uuid.len = 0;
  return true;
  }
  if (pfd->state == PREFIX_REBIND &&
  cur_time > pfd->last_complete + pfd->vlife_time) {
-pfd->state = PREFIX_SOLICIT;
  return true;
  }
  }
  return false;
  }

+static void ipv6_prefixd_update_state(struct ipv6_prefixd_state *pfd)
+{
+long long int cur_time = time_msec();
+
+if (pfd->state == PREFIX_DONE &&
+cur_time > pfd->last_complete + pfd->t1) {
+pfd->state = PREFIX_RENEW;
+return;
+}
+if (pfd->state == PREFIX_RENEW &&
+cur_time > pfd->last_complete + pfd->t2) {
+pfd->state = PREFIX_REBIND;
+pfd->uuid.len = 0;
+return;
+}
+if (pfd->state == PREFIX_REBIND &&
+cur_time > pfd->last_complete + pfd->vlife_time) {
+pfd->state = PREFIX_SOLICIT;
+return;
+}
+}
+
  static void
  ipv6_prefixd_wait(long long int timeout)
  {
-if (ipv6_prefixd_should_inject()) {
+/* We need to wake up in all states :
+ * - In SOLICIT and REQUEST states we need to wakeup to handle
+ *   next_announce timer.
+ * - In DONE, PENDING, RENEW and REBIND states, we need to wake up to
+ *   handle T1, T2 timers.
+ */
+if (!shash_is_empty(&ipv6_prefixd)) {
  poll_timer_wait_until(timeout);
  }
  }
@@ -1266,6 +1291,7 @@ send_ipv6_prefixd(struct rconn *swconn, long long
int *send_prefixd_time)
  if (*send_prefixd_time > next_msg) {
  *send_prefixd_time = next_msg;
  }
+ipv6_prefixd_update_state(pfd);
  }
  }

diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at
index 4bfc74582..9e7f40176 100644
--- a/tests/system-common-macros.at
+++ b/tests/system-common-macros.at
@@ -418,15 +418,14 @@ ovn-nbctl lsp-add public public1 \
  ovn-nbctl set logical_router_port rp-public options:prefix_delegation=true
  ovn-nbctl set logical_router_port rp-public options:prefix=true
  ovn-nbctl set logical_router_port rp-sw0 options:prefix=true
-ovn-nbctl set logical_router_port rp-sw1 options:prefix=true

  OVN_POPULATE_ARP

  ovn-nbctl --wait=hv sync

  cat > /etc/dhcp/dhcpd.conf < /tmp/rp-public
-ovn-nbctl set logical_router_port rp-sw0 options:prefix=false

Re: [ovs-dev] [PATCH ovn v4 4/4] tests: Use the ovn-debug binary to determine table numbers.

[Mark Michelson]

Thanks Ales,

Acked-by: Mark Michelson 

This is probably not a huge surprise given how long it has been, but 
this series no longer applies cleanly to main. Can you please post a 
rebased version of the series as a v5? Thanks!


On 2/12/24 10:55, Ales Musil wrote:

Use the ovn-debug commands to determine OpenFlow table numbers
based on stage name. With this there is no need to hardcode them
and it should be future proof for stage shifts/updates.

Signed-off-by: Ales Musil 
Acked-by: Mark Michelson 
---
v4: Rebase on top of main.
 Add ack from Mark.
---
  tests/ovn-controller.at  | 342 +++---
  tests/ovn.at | 389 ++-
  tests/system-ovn-kmod.at |  16 +-
  tests/system-ovn.at  |  20 +-
  4 files changed, 438 insertions(+), 329 deletions(-)

diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
index f77e032d4..66e870876 100644
--- a/tests/ovn-controller.at
+++ b/tests/ovn-controller.at
@@ -901,6 +901,10 @@ check ovn-nbctl lsp-add ls1 ls1-lp1 \
  wait_for_ports_up
  ovn-appctl -t ovn-controller vlog/set file:dbg
  
+# Get the OF table numbers

+acl_eval=$(ovn-debug lflow-stage-to-oftable ls_out_acl_eval)
+acl_action=$(ovn-debug lflow-stage-to-oftable ls_out_acl_action)
+
  dp_key=$(printf "%x" $(fetch_column datapath tunnel_key 
external_ids:name=ls1))
  port_key=$(printf "%x" $(fetch_column port_binding tunnel_key 
logical_port=ls1-lp1))
  
@@ -918,14 +922,14 @@ for i in $(seq 10); do

  check ovn-nbctl add address_set as1 addresses 10.0.0.$i
  check ovn-nbctl --wait=hv sync
  if test "$i" = 3; then
-AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int 
table=46,reg15=0x$port_key | \
+AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int 
table=$acl_eval,reg15=0x$port_key | \
  grep -v reply | awk '{print $7, $8}' | sort], [0], [dnl
-priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.1 
actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,47)
-priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.2 
actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,47)
-priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.3 
actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,47)
+priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.1 
actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,$acl_action)
+priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.2 
actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,$acl_action)
+priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.3 
actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,$acl_action)
  ])
  fi
-AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int table=46 | grep -c 
"priority=1100"], [0], [$i
+AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int table=$acl_eval | grep -c 
"priority=1100"], [0], [$i
  ])
  done
  
@@ -940,15 +944,15 @@ for i in $(seq 10); do

  check ovn-nbctl remove address_set as1 addresses 10.0.0.$i
  check ovn-nbctl --wait=hv sync
  if test "$i" = 9; then
-AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int 
table=46,reg15=0x$port_key | \
+AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int 
table=$acl_eval,reg15=0x$port_key | \
  grep -v reply | awk '{print $7, $8}'], [0], [dnl
-priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.10 
actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,47)
+priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.10 
actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,$acl_action)
  ])
  fi
  if test "$i" = 10; then
-AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int table=46 | grep 
"priority=1100"], [1], [ignore])
+AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int table=$acl_eval | grep 
"priority=1100"], [1], [ignore])
  else
-AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int table=46 | grep -c 
"priority=1100"], [0], [$((10 - $i))
+AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int table=$acl_eval | grep -c 
"priority=1100"], [0], [$((10 - $i))
  ])
  fi
  done
@@ -966,17 +970,17 @@ for i in $(seq 10); do
  check ovn-nbctl add address_set as1 addresses 10.0.0.$i,10.0.1.$i
  check ovn-nbctl --wait=hv sync
  if test "$i" = 3; then
-AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int 
table=46,reg15=0x$port_key | \
+AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int 
table=$acl_eval,reg15=0x$port_key | \
  grep -v reply | awk '{print $7, $8}' | sort], [0], [dnl
-priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.1 
actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,47)
-priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.2 
actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,47)
-priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.3 
actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,47)
-priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.1.1 
actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,47)
-priority=1100,ip,reg15=0x$por

Re: [ovs-dev] [PATCH v11 4/5] dpif: Probe support for OVS_ACTION_ATTR_DROP.

[Eelco Chaudron]

On 15 Mar 2024, at 17:37, Eric Garver wrote:

> On Fri, Mar 15, 2024 at 04:55:50PM +0100, Eelco Chaudron wrote:
>>
>>
>> On 11 Mar 2024, at 18:51, Eric Garver wrote:
>>
>>> Kernel support has been added for this action. As such, we need to probe
>>> the datapath for support.
>>>
>>> Signed-off-by: Eric Garver 
>>
>> Thanks for submitting the changes requested. Some comments below.
>>
>> Cheers,
>>
>> Eelco
>>
>>
>>> ---
>>>  include/linux/openvswitch.h |  2 +-
>>>  lib/dpif.c  |  6 ++-
>>>  lib/dpif.h  |  2 +-
>>>  ofproto/ofproto-dpif.c  | 77 ++---
>>>  ofproto/ofproto-dpif.h  |  4 +-
>>>  5 files changed, 81 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
>>> index a265e05ad253..fce6456f47c8 100644
>>> --- a/include/linux/openvswitch.h
>>> +++ b/include/linux/openvswitch.h
>>> @@ -1086,11 +1086,11 @@ enum ovs_action_attr {
>>> OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */
>>> OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */
>>> OVS_ACTION_ATTR_DEC_TTL,  /* Nested OVS_DEC_TTL_ATTR_*. */
>>> +   OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */
>>>
>>>  #ifndef __KERNEL__
>>> OVS_ACTION_ATTR_TUNNEL_PUSH,   /* struct ovs_action_push_tnl*/
>>> OVS_ACTION_ATTR_TUNNEL_POP,/* u32 port number. */
>>> -   OVS_ACTION_ATTR_DROP,  /* u32 xlate_error. */
>>> OVS_ACTION_ATTR_LB_OUTPUT, /* u32 bond-id. */
>>>  #endif
>>> __OVS_ACTION_ATTR_MAX,/* Nothing past this will be accepted
>>> diff --git a/lib/dpif.c b/lib/dpif.c
>>> index 0f480bec48d0..23eb18495a66 100644
>>> --- a/lib/dpif.c
>>> +++ b/lib/dpif.c
>>> @@ -28,6 +28,7 @@
>>>  #include "dpctl.h"
>>>  #include "dpif-netdev.h"
>>>  #include "flow.h"
>>> +#include "netdev-offload.h"
>>>  #include "netdev-provider.h"
>>>  #include "netdev.h"
>>>  #include "netlink.h"
>>> @@ -1935,9 +1936,10 @@ dpif_supports_tnl_push_pop(const struct dpif *dpif)
>>>  }
>>>
>>>  bool
>>> -dpif_supports_explicit_drop_action(const struct dpif *dpif)
>>> +dpif_may_support_explicit_drop_action(const struct dpif *dpif)
>>>  {
>>> -return dpif_is_netdev(dpif);
>>> +/* TC does not support offloading this action. */
>>> +return dpif_is_netdev(dpif) || !netdev_is_flow_api_enabled();
>>>  }
>>>
>>>  bool
>>> diff --git a/lib/dpif.h b/lib/dpif.h
>>> index 0f2dc2ef3c55..a764e8a592bd 100644
>>> --- a/lib/dpif.h
>>> +++ b/lib/dpif.h
>>> @@ -940,7 +940,7 @@ int dpif_get_pmds_for_port(const struct dpif * dpif, 
>>> odp_port_t port_no,
>>>
>>>  char *dpif_get_dp_version(const struct dpif *);
>>>  bool dpif_supports_tnl_push_pop(const struct dpif *);
>>> -bool dpif_supports_explicit_drop_action(const struct dpif *);
>>> +bool dpif_may_support_explicit_drop_action(const struct dpif *);
>>>  bool dpif_synced_dp_layers(struct dpif *);
>>>
>>>  /* Log functions. */
>>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>>> index 2ec1cd1854ab..7bacd2120c78 100644
>>> --- a/ofproto/ofproto-dpif.c
>>> +++ b/ofproto/ofproto-dpif.c
>>> @@ -221,6 +221,7 @@ static void ct_zone_config_init(struct dpif_backer 
>>> *backer);
>>>  static void ct_zone_config_uninit(struct dpif_backer *backer);
>>>  static void ct_zone_timeout_policy_sweep(struct dpif_backer *backer);
>>>  static void ct_zone_limits_commit(struct dpif_backer *backer);
>>> +static bool recheck_support_explicit_drop_action(struct dpif_backer 
>>> *backer);
>>>
>>>  static inline struct ofproto_dpif *
>>>  ofproto_dpif_cast(const struct ofproto *ofproto)
>>> @@ -391,6 +392,10 @@ type_run(const char *type)
>>>  udpif_set_threads(backer->udpif, n_handlers, n_revalidators);
>>>  }
>>>
>>> +if (recheck_support_explicit_drop_action(backer)) {
>>> +backer->need_revalidate = REV_RECONFIGURE;
>>> +}
>>> +
>>>  if (backer->need_revalidate) {
>>>  struct ofproto_dpif *ofproto;
>>>  struct simap_node *node;
>>> @@ -807,6 +812,7 @@ open_dpif_backer(const char *type, struct dpif_backer 
>>> **backerp)
>>>
>>>  shash_add(&all_dpif_backers, type, backer);
>>>
>>> +atomic_init(&backer->rt_support.explicit_drop_action, false);
>>>  check_support(backer);
>>>  atomic_count_init(&backer->tnl_count, 0);
>>>
>>> @@ -855,7 +861,10 @@ ovs_native_tunneling_is_on(struct ofproto_dpif 
>>> *ofproto)
>>>  bool
>>>  ovs_explicit_drop_action_supported(struct ofproto_dpif *ofproto)
>>>  {
>>> -return ofproto->backer->rt_support.explicit_drop_action;
>>> +bool value;
>>
>> nit: I would add a new line here.
>
> ACK.
>
>>> +atomic_read_relaxed(&ofproto->backer->rt_support.explicit_drop_action,
>>> +&value);
>>> +return value;
>>>  }
>>>
>>>  bool
>>> @@ -1379,6 +1388,41 @@ check_ct_timeout_policy(struct dpif_backer *backer)
>>>  return !error;
>>>  }
>>>
>>> +/* Tests whether backer's datapath supports the OVS_ACTION_ATTR_DROP 
>>> ac

Re: [ovs-dev] [PATCH v11 4/5] dpif: Probe support for OVS_ACTION_ATTR_DROP.

[Eric Garver]

On Fri, Mar 15, 2024 at 04:55:50PM +0100, Eelco Chaudron wrote:
> 
> 
> On 11 Mar 2024, at 18:51, Eric Garver wrote:
> 
> > Kernel support has been added for this action. As such, we need to probe
> > the datapath for support.
> >
> > Signed-off-by: Eric Garver 
> 
> Thanks for submitting the changes requested. Some comments below.
> 
> Cheers,
> 
> Eelco
> 
> 
> > ---
> >  include/linux/openvswitch.h |  2 +-
> >  lib/dpif.c  |  6 ++-
> >  lib/dpif.h  |  2 +-
> >  ofproto/ofproto-dpif.c  | 77 ++---
> >  ofproto/ofproto-dpif.h  |  4 +-
> >  5 files changed, 81 insertions(+), 10 deletions(-)
> >
> > diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
> > index a265e05ad253..fce6456f47c8 100644
> > --- a/include/linux/openvswitch.h
> > +++ b/include/linux/openvswitch.h
> > @@ -1086,11 +1086,11 @@ enum ovs_action_attr {
> > OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */
> > OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */
> > OVS_ACTION_ATTR_DEC_TTL,  /* Nested OVS_DEC_TTL_ATTR_*. */
> > +   OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */
> >
> >  #ifndef __KERNEL__
> > OVS_ACTION_ATTR_TUNNEL_PUSH,   /* struct ovs_action_push_tnl*/
> > OVS_ACTION_ATTR_TUNNEL_POP,/* u32 port number. */
> > -   OVS_ACTION_ATTR_DROP,  /* u32 xlate_error. */
> > OVS_ACTION_ATTR_LB_OUTPUT, /* u32 bond-id. */
> >  #endif
> > __OVS_ACTION_ATTR_MAX,/* Nothing past this will be accepted
> > diff --git a/lib/dpif.c b/lib/dpif.c
> > index 0f480bec48d0..23eb18495a66 100644
> > --- a/lib/dpif.c
> > +++ b/lib/dpif.c
> > @@ -28,6 +28,7 @@
> >  #include "dpctl.h"
> >  #include "dpif-netdev.h"
> >  #include "flow.h"
> > +#include "netdev-offload.h"
> >  #include "netdev-provider.h"
> >  #include "netdev.h"
> >  #include "netlink.h"
> > @@ -1935,9 +1936,10 @@ dpif_supports_tnl_push_pop(const struct dpif *dpif)
> >  }
> >
> >  bool
> > -dpif_supports_explicit_drop_action(const struct dpif *dpif)
> > +dpif_may_support_explicit_drop_action(const struct dpif *dpif)
> >  {
> > -return dpif_is_netdev(dpif);
> > +/* TC does not support offloading this action. */
> > +return dpif_is_netdev(dpif) || !netdev_is_flow_api_enabled();
> >  }
> >
> >  bool
> > diff --git a/lib/dpif.h b/lib/dpif.h
> > index 0f2dc2ef3c55..a764e8a592bd 100644
> > --- a/lib/dpif.h
> > +++ b/lib/dpif.h
> > @@ -940,7 +940,7 @@ int dpif_get_pmds_for_port(const struct dpif * dpif, 
> > odp_port_t port_no,
> >
> >  char *dpif_get_dp_version(const struct dpif *);
> >  bool dpif_supports_tnl_push_pop(const struct dpif *);
> > -bool dpif_supports_explicit_drop_action(const struct dpif *);
> > +bool dpif_may_support_explicit_drop_action(const struct dpif *);
> >  bool dpif_synced_dp_layers(struct dpif *);
> >
> >  /* Log functions. */
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index 2ec1cd1854ab..7bacd2120c78 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -221,6 +221,7 @@ static void ct_zone_config_init(struct dpif_backer 
> > *backer);
> >  static void ct_zone_config_uninit(struct dpif_backer *backer);
> >  static void ct_zone_timeout_policy_sweep(struct dpif_backer *backer);
> >  static void ct_zone_limits_commit(struct dpif_backer *backer);
> > +static bool recheck_support_explicit_drop_action(struct dpif_backer 
> > *backer);
> >
> >  static inline struct ofproto_dpif *
> >  ofproto_dpif_cast(const struct ofproto *ofproto)
> > @@ -391,6 +392,10 @@ type_run(const char *type)
> >  udpif_set_threads(backer->udpif, n_handlers, n_revalidators);
> >  }
> >
> > +if (recheck_support_explicit_drop_action(backer)) {
> > +backer->need_revalidate = REV_RECONFIGURE;
> > +}
> > +
> >  if (backer->need_revalidate) {
> >  struct ofproto_dpif *ofproto;
> >  struct simap_node *node;
> > @@ -807,6 +812,7 @@ open_dpif_backer(const char *type, struct dpif_backer 
> > **backerp)
> >
> >  shash_add(&all_dpif_backers, type, backer);
> >
> > +atomic_init(&backer->rt_support.explicit_drop_action, false);
> >  check_support(backer);
> >  atomic_count_init(&backer->tnl_count, 0);
> >
> > @@ -855,7 +861,10 @@ ovs_native_tunneling_is_on(struct ofproto_dpif 
> > *ofproto)
> >  bool
> >  ovs_explicit_drop_action_supported(struct ofproto_dpif *ofproto)
> >  {
> > -return ofproto->backer->rt_support.explicit_drop_action;
> > +bool value;
> 
> nit: I would add a new line here.

ACK.

> > +atomic_read_relaxed(&ofproto->backer->rt_support.explicit_drop_action,
> > +&value);
> > +return value;
> >  }
> >
> >  bool
> > @@ -1379,6 +1388,41 @@ check_ct_timeout_policy(struct dpif_backer *backer)
> >  return !error;
> >  }
> >
> > +/* Tests whether backer's datapath supports the OVS_ACTION_ATTR_DROP 
> > action. */
> > +static bool
> > +check_drop_action(struct dpi

Re: [ovs-dev] [PATCH v11 5/5] tests: system-traffic: Add coverage for drop action.

[Eelco Chaudron]

On 11 Mar 2024, at 18:51, Eric Garver wrote:

> Exercise the drop action in the datapath. This specific tests triggers
> an xlate_error.
>
> For the kernel datapath skb drop reasons can then be seen while this
> test runs.
>
>  # perf trace -e skb:kfree_skb --filter="reason >= 0x3"
>0.000 ping/1275884 skb:kfree_skb(skbaddr: 0x8acd76546000, \
>   location: 0xc0ee3634, protocol: 2048, reason: 196611)
>
> Signed-off-by: Eric Garver 

This patch looks good to me. I re-ran all GitLab test cases and they pass just 
fine.

//Eelco


Acked-by: Eelco Chaudron 

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

Re: [ovs-dev] [PATCH ovn v2 2/2] tests: Remove table numbers from "action parsing".

[Mark Michelson]

I tried to apply this series just now, but patch 2 fails to apply due to 
recent changes in tests/ovn.at in OVN main. Xavier, could you rebase and 
post a v3 of this series? I will merge this as soon as possible afterwards.


On 2/13/24 02:39, Ales Musil wrote:

On Mon, Feb 12, 2024 at 3:48 PM Xavier Simonart  wrote:


This patch uses the recently introduced macros defining openflow table
numbers.

Signed-off-by: Xavier Simonart 

---
v2: - Handled Ales' comments (i.e. fix few remaining hard-coded numbers)
 - Rebase on origin/main
---
  tests/ovn-macros.at |   4 +
  tests/ovn.at| 245 +++-
  2 files changed, 132 insertions(+), 117 deletions(-)

diff --git a/tests/ovn-macros.at b/tests/ovn-macros.at
index 84e50d76f..db107f43a 100644
--- a/tests/ovn-macros.at
+++ b/tests/ovn-macros.at
@@ -995,6 +995,8 @@ m4_define([OFTABLE_OUTPUT_LARGE_PKT_DETECT], [37])
  m4_define([OFTABLE_OUTPUT_LARGE_PKT_PROCESS], [38])
  m4_define([OFTABLE_REMOTE_OUTPUT], [39])
  m4_define([OFTABLE_LOCAL_OUTPUT], [40])
+m4_define([OFTABLE_LOG_EGRESS_PIPELINE], [42])
+m4_define([OFTABLE_SAVE_INPORT], [64])
  m4_define([OFTABLE_LOG_TO_PHY], [65])
  m4_define([OFTABLE_MAC_BINDING], [66])
  m4_define([OFTABLE_MAC_LOOKUP], [67])
@@ -1010,3 +1012,5 @@ m4_define([OFTABLE_ECMP_NH_MAC], [76])
  m4_define([OFTABLE_ECMP_NH], [77])
  m4_define([OFTABLE_CHK_LB_AFFINITY], [78])
  m4_define([OFTABLE_MAC_CACHE_USE], [79])
+
+m4_define([OFTABLE_SAVE_INPORT_HEX], [m4_eval(OFTABLE_SAVE_INPORT, 16)])
diff --git a/tests/ovn.at b/tests/ovn.at
index 25c94d34a..5ae2cc62f 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -979,6 +979,17 @@ AT_CLEANUP
  AT_SETUP([action parsing])
  dnl Unindented text is input (a set of OVN logical actions).
  dnl Indented text is expected output.
+
+# lflow table hard-coded to 10 in test-ovn, so next is 11.
+m4_define([lflow_table], [11])
+
+m4_define([NEXT], [m4_if(
+$1, ingress, [m4_eval($2 + OFTABLE_LOG_INGRESS_PIPELINE)],
+$1, egress, [m4_eval($2 + OFTABLE_LOG_EGRESS_PIPELINE)])])
+
+m4_define([oflow_in_table], [NEXT(ingress, lflow_table)])
+m4_define([oflow_out_table], [NEXT(egress, lflow_table)])
+
  AT_DATA([test-cases.txt], [
  # drop
  drop;
@@ -990,18 +1001,18 @@ next; drop;

  # output
  output;
-encodes as resubmit(,64)
+encodes as resubmit(,OFTABLE_SAVE_INPORT)

  # next
  next;
-encodes as resubmit(,19)
-next(11);
+encodes as resubmit(,oflow_in_table)
+next(lflow_table);
  formats as next;
-encodes as resubmit(,19)
+encodes as resubmit(,oflow_in_table)
  next(0);
-encodes as resubmit(,8)
+encodes as resubmit(,OFTABLE_LOG_INGRESS_PIPELINE)
  next(23);
-encodes as resubmit(,31)
+encodes as resubmit(,NEXT(ingress, 23))

  next();
  Syntax error at `)' expecting "pipeline" or "table".
@@ -1010,29 +1021,29 @@ next(10;
  next(24);
  "next" action cannot advance beyond table 23.

-next(table=11);
+next(table=lflow_table);
  formats as next;
-encodes as resubmit(,19)
+encodes as resubmit(,oflow_in_table)
  next(pipeline=ingress);
  formats as next;
-encodes as resubmit(,19)
-next(table=11, pipeline=ingress);
+encodes as resubmit(,oflow_in_table)
+next(table=lflow_table, pipeline=ingress);
  formats as next;
-encodes as resubmit(,19)
-next(pipeline=ingress, table=11);
+encodes as resubmit(,oflow_in_table)
+next(pipeline=ingress, table=lflow_table);
  formats as next;
-encodes as resubmit(,19)
+encodes as resubmit(,oflow_in_table)

  next(pipeline=egress);
-formats as next(pipeline=egress, table=11);
-encodes as resubmit(,53)
+formats as next(pipeline=egress, table=lflow_table);
+encodes as resubmit(,oflow_out_table)

  next(pipeline=egress, table=5);
-encodes as resubmit(,47)
+encodes as resubmit(,NEXT(egress, 5))

  next(table=10);
  formats as next(10);
-encodes as resubmit(,18)
+encodes as resubmit(,NEXT(ingress, 10))

  # Loading a constant value.
  tcp.dst=80;
@@ -1054,7 +1065,7 @@ ip.ttl=4;
  has prereqs eth.type == 0x800 || eth.type == 0x86dd
  outport="eth0"; next; outport="LOCAL"; next;
  formats as outport = "eth0"; next; outport = "LOCAL"; next;
-encodes as
set_field:0x5->reg15,resubmit(,19),set_field:0xfffe->reg15,resubmit(,19)
+encodes as
set_field:0x5->reg15,resubmit(,oflow_in_table),set_field:0xfffe->reg15,resubmit(,oflow_in_table)

  inport[[1]] = 1;
  Cannot select subfield of string field inport.
@@ -1152,35 +1163,35 @@ pkt.mark = "foo";

  # load balancing.
  ct_lb;
-encodes as ct(table=19,zone=NXM_NX_REG13[[0..15]],nat)
+encodes as ct(table=oflow_in_table,zone=NXM_NX_REG13[[0..15]],nat)
  has prereqs ip
  ct_lb();
  formats as ct_lb;
-encodes as ct(table=19,zone=NXM_NX_REG13[[0..15]],nat)
+encodes as ct(table=oflow_in_table,zone=NXM_NX_REG13[[0..15]],nat)
  has prereqs ip
  ct_lb(192.168.1.2:80, 192.168.1.3:80);
  Syntax error at `192.168.1.2' expecting backends.

Re: [ovs-dev] [PATCH v11 4/5] dpif: Probe support for OVS_ACTION_ATTR_DROP.

[Eelco Chaudron]

On 11 Mar 2024, at 18:51, Eric Garver wrote:

> Kernel support has been added for this action. As such, we need to probe
> the datapath for support.
>
> Signed-off-by: Eric Garver 

Thanks for submitting the changes requested. Some comments below.

Cheers,

Eelco


> ---
>  include/linux/openvswitch.h |  2 +-
>  lib/dpif.c  |  6 ++-
>  lib/dpif.h  |  2 +-
>  ofproto/ofproto-dpif.c  | 77 ++---
>  ofproto/ofproto-dpif.h  |  4 +-
>  5 files changed, 81 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
> index a265e05ad253..fce6456f47c8 100644
> --- a/include/linux/openvswitch.h
> +++ b/include/linux/openvswitch.h
> @@ -1086,11 +1086,11 @@ enum ovs_action_attr {
>   OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */
>   OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */
>   OVS_ACTION_ATTR_DEC_TTL,  /* Nested OVS_DEC_TTL_ATTR_*. */
> + OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */
>
>  #ifndef __KERNEL__
>   OVS_ACTION_ATTR_TUNNEL_PUSH,   /* struct ovs_action_push_tnl*/
>   OVS_ACTION_ATTR_TUNNEL_POP,/* u32 port number. */
> - OVS_ACTION_ATTR_DROP,  /* u32 xlate_error. */
>   OVS_ACTION_ATTR_LB_OUTPUT, /* u32 bond-id. */
>  #endif
>   __OVS_ACTION_ATTR_MAX,/* Nothing past this will be accepted
> diff --git a/lib/dpif.c b/lib/dpif.c
> index 0f480bec48d0..23eb18495a66 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -28,6 +28,7 @@
>  #include "dpctl.h"
>  #include "dpif-netdev.h"
>  #include "flow.h"
> +#include "netdev-offload.h"
>  #include "netdev-provider.h"
>  #include "netdev.h"
>  #include "netlink.h"
> @@ -1935,9 +1936,10 @@ dpif_supports_tnl_push_pop(const struct dpif *dpif)
>  }
>
>  bool
> -dpif_supports_explicit_drop_action(const struct dpif *dpif)
> +dpif_may_support_explicit_drop_action(const struct dpif *dpif)
>  {
> -return dpif_is_netdev(dpif);
> +/* TC does not support offloading this action. */
> +return dpif_is_netdev(dpif) || !netdev_is_flow_api_enabled();
>  }
>
>  bool
> diff --git a/lib/dpif.h b/lib/dpif.h
> index 0f2dc2ef3c55..a764e8a592bd 100644
> --- a/lib/dpif.h
> +++ b/lib/dpif.h
> @@ -940,7 +940,7 @@ int dpif_get_pmds_for_port(const struct dpif * dpif, 
> odp_port_t port_no,
>
>  char *dpif_get_dp_version(const struct dpif *);
>  bool dpif_supports_tnl_push_pop(const struct dpif *);
> -bool dpif_supports_explicit_drop_action(const struct dpif *);
> +bool dpif_may_support_explicit_drop_action(const struct dpif *);
>  bool dpif_synced_dp_layers(struct dpif *);
>
>  /* Log functions. */
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 2ec1cd1854ab..7bacd2120c78 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -221,6 +221,7 @@ static void ct_zone_config_init(struct dpif_backer 
> *backer);
>  static void ct_zone_config_uninit(struct dpif_backer *backer);
>  static void ct_zone_timeout_policy_sweep(struct dpif_backer *backer);
>  static void ct_zone_limits_commit(struct dpif_backer *backer);
> +static bool recheck_support_explicit_drop_action(struct dpif_backer *backer);
>
>  static inline struct ofproto_dpif *
>  ofproto_dpif_cast(const struct ofproto *ofproto)
> @@ -391,6 +392,10 @@ type_run(const char *type)
>  udpif_set_threads(backer->udpif, n_handlers, n_revalidators);
>  }
>
> +if (recheck_support_explicit_drop_action(backer)) {
> +backer->need_revalidate = REV_RECONFIGURE;
> +}
> +
>  if (backer->need_revalidate) {
>  struct ofproto_dpif *ofproto;
>  struct simap_node *node;
> @@ -807,6 +812,7 @@ open_dpif_backer(const char *type, struct dpif_backer 
> **backerp)
>
>  shash_add(&all_dpif_backers, type, backer);
>
> +atomic_init(&backer->rt_support.explicit_drop_action, false);
>  check_support(backer);
>  atomic_count_init(&backer->tnl_count, 0);
>
> @@ -855,7 +861,10 @@ ovs_native_tunneling_is_on(struct ofproto_dpif *ofproto)
>  bool
>  ovs_explicit_drop_action_supported(struct ofproto_dpif *ofproto)
>  {
> -return ofproto->backer->rt_support.explicit_drop_action;
> +bool value;

nit: I would add a new line here.

> +atomic_read_relaxed(&ofproto->backer->rt_support.explicit_drop_action,
> +&value);
> +return value;
>  }
>
>  bool
> @@ -1379,6 +1388,41 @@ check_ct_timeout_policy(struct dpif_backer *backer)
>  return !error;
>  }
>
> +/* Tests whether backer's datapath supports the OVS_ACTION_ATTR_DROP action. 
> */
> +static bool
> +check_drop_action(struct dpif_backer *backer)
> +{
> +struct odputil_keybuf keybuf;
> +uint8_t actbuf[NL_A_U32_SIZE];
> +struct ofpbuf actions;
> +struct ofpbuf key;
> +struct flow flow;
> +bool supported;
> +bool hw_offload;
> +
> +struct odp_flow_key_parms odp_parms = {
> +.flow = &flow,
> +.probe = true,
> +

Re: [ovs-dev] [PATCH ovn v3] controller: Avoid double controller action for ICMP errors.

[Mark Michelson]

On 2/12/24 07:00, Dumitru Ceara wrote:

On 2/6/24 10:15, Ales Musil wrote:

The fields that are not directly supported by OvS were encoded
via additional controller action that changed the required value.
This was most notably needed for ICMP need frag messages.

Encode the field value loads as note action instead. This allows
us to find the note and act accordingly in the first controller
action without the need to send it to pinctrl again, parse and
change the packet. This way we can also encode any future fields
that might be needed as this method should be flexible enough.

This change is completely transparent to the user and shouldn't
cause any disruptions.

Signed-off-by: Ales Musil 
---
v3: Rebase on top of current main.
 Fix some cosmetic stuff pointed out by Dumitru.
 Add back the handler for old way to keep the ICMP
 error working during upgrade.
v2: Fix the wrong checksum for the ICMP packet.
---


Thanks for the update, Ales!  Looks good to me:

Acked-by: Dumitru Ceara 

Regards,
Dumitru



Thanks Ales and Dumitru,

I pushed this change to main.


___
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 v11 3/5] dpif: Support atomic_bool field type.

[Eelco Chaudron]

On 11 Mar 2024, at 18:51, Eric Garver wrote:

> The next commit will convert a dp feature from bool to atomic_bool. As
> such we have to add support to the macros and functions. We must pass by
> reference instead of pass by value because all the atomic operations
> require a reference.
>
> Signed-off-by: Eric Garver 

This patch looks good to me.

Cheers,

Eelco

Acked-by: Eelco Chaudron 

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

[ovs-dev] [PATCH ovn] ovn-ic: Avoid igmp/mld traffic flooding.

[Lorenzo Bianconi]

Avoid recirculating IGMP/MLD packets more than one time from stage
ls_out_pre_lb in the egress pipeline to ovn table 37 in order to avoid
packet looping for ovn-ic deployment.

Acked-by: Mohammad Heib 
Signed-off-by: Lorenzo Bianconi 
---
 controller/pinctrl.c |  2 ++
 include/ovn/logical-fields.h |  3 +++
 lib/logical-fields.c |  4 
 northd/northd.c  | 11 +++
 4 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index 98b29de9f..ba4775e20 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -660,6 +660,8 @@ pinctrl_forward_pkt(struct rconn *swconn, int64_t dp_key,
 put_load(dp_key, MFF_LOG_DATAPATH, 0, 64, &ofpacts);
 put_load(in_port_key, MFF_LOG_INPORT, 0, 32, &ofpacts);
 put_load(out_port_key, MFF_LOG_OUTPORT, 0, 32, &ofpacts);
+/* Avoid re-injecting packet already consumed. */
+put_load(1, MFF_LOG_FLAGS, MLF_IGMP_IGMP_SPOOF_INJECT_BIT, 1, &ofpacts);
 
 struct ofpact_resubmit *resubmit = ofpact_put_RESUBMIT(&ofpacts);
 resubmit->in_port = OFPP_CONTROLLER;
diff --git a/include/ovn/logical-fields.h b/include/ovn/logical-fields.h
index ce79b501c..84c287e0a 100644
--- a/include/ovn/logical-fields.h
+++ b/include/ovn/logical-fields.h
@@ -82,6 +82,7 @@ enum mff_log_flags_bits {
 MLF_LOCALNET_BIT = 15,
 MLF_RX_FROM_TUNNEL_BIT = 16,
 MLF_ICMP_SNAT_BIT = 17,
+MLF_IGMP_IGMP_SPOOF_INJECT_BIT = 18,
 };
 
 /* MFF_LOG_FLAGS_REG flag assignments */
@@ -137,6 +138,8 @@ enum mff_log_flags {
 MLF_RX_FROM_TUNNEL = (1 << MLF_RX_FROM_TUNNEL_BIT),
 
 MLF_ICMP_SNAT = (1 << MLF_ICMP_SNAT_BIT),
+
+MLF_IGMP_IGMP_SPOOF = (1 << MLF_IGMP_IGMP_SPOOF_INJECT_BIT),
 };
 
 /* OVN logical fields
diff --git a/lib/logical-fields.c b/lib/logical-fields.c
index 20219a67a..7e60c7c86 100644
--- a/lib/logical-fields.c
+++ b/lib/logical-fields.c
@@ -139,6 +139,10 @@ ovn_init_symtab(struct shash *symtab)
  flags_str);
 snprintf(flags_str, sizeof flags_str, "flags[%d]", MLF_RX_FROM_TUNNEL_BIT);
 expr_symtab_add_subfield(symtab, "flags.tunnel_rx", NULL, flags_str);
+snprintf(flags_str, sizeof flags_str, "flags[%d]",
+ MLF_IGMP_IGMP_SPOOF_INJECT_BIT);
+expr_symtab_add_subfield(symtab, "flags.igmp_loopback", NULL,
+ flags_str);
 
 /* Connection tracking state. */
 expr_symtab_add_field_scoped(symtab, "ct_mark", MFF_CT_MARK, NULL, false,
diff --git a/northd/northd.c b/northd/northd.c
index 1839b7d8b..2c860082d 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -6089,7 +6089,8 @@ build_interconn_mcast_snoop_flows(struct ovn_datapath *od,
 continue;
 }
 /* Punt IGMP traffic to controller. */
-char *match = xasprintf("inport == %s && igmp", op->json_key);
+char *match = xasprintf("inport == %s && igmp && "
+"flags.igmp_loopback == 0", op->json_key);
 ovn_lflow_metered(lflows, od, S_SWITCH_OUT_PRE_LB, 120, match,
   "clone { igmp; }; next;",
   copp_meter_get(COPP_IGMP, od->nbs->copp,
@@ -6098,7 +6099,8 @@ build_interconn_mcast_snoop_flows(struct ovn_datapath *od,
 free(match);
 
 /* Punt MLD traffic to controller. */
-match = xasprintf("inport == %s && (mldv1 || mldv2)", op->json_key);
+match = xasprintf("inport == %s && (mldv1 || mldv2) && "
+  "flags.igmp_loopback == 0", op->json_key);
 ovn_lflow_metered(lflows, od, S_SWITCH_OUT_PRE_LB, 120, match,
   "clone { igmp; }; next;",
   copp_meter_get(COPP_IGMP, od->nbs->copp,
@@ -9285,14 +9287,15 @@ build_lswitch_destination_lookup_bmcast(struct 
ovn_datapath *od,
 ds_put_cstr(actions, "igmp;");
 /* Punt IGMP traffic to controller. */
 ovn_lflow_metered(lflows, od, S_SWITCH_IN_L2_LKUP, 100,
-  "igmp", ds_cstr(actions),
+  "flags.igmp_loopback == 0 && igmp", ds_cstr(actions),
   copp_meter_get(COPP_IGMP, od->nbs->copp,
  meter_groups),
   lflow_ref);
 
 /* Punt MLD traffic to controller. */
 ovn_lflow_metered(lflows, od, S_SWITCH_IN_L2_LKUP, 100,
-  "mldv1 || mldv2", ds_cstr(actions),
+  "flags.igmp_loopback == 0 && (mldv1 || mldv2)",
+  ds_cstr(actions),
   copp_meter_get(COPP_IGMP, od->nbs->copp,
  meter_groups),
   lflow_ref);
-- 
2.44.0

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

Re: [ovs-dev] [PATCH v11 2/5] dpif: Make get_datapath_cap() access support by pointer.

[Eelco Chaudron]

On 11 Mar 2024, at 18:51, Eric Garver wrote:

> This avoids copying the support struct onto the stack.
>
> Signed-off-by: Eric Garver 
> ---

This change looks good to me. Thanks for catching this.

//Eelco

Acked-by: Eelco Chaudron 

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

Re: [ovs-dev] [RFC ovn] ovn-ic: avoid igmp/mld traffic flooding

[Lorenzo Bianconi]

> Hi Lorenzo,

Hi Mohammad,

> 
> i have applied and tested this change and i can confirm that it fixes the
> issue.
> if you please can rebase on top of the main.
> 
> 
> Acked-by: Mohammad Heib 

thx for reviewing the patch. I will post v1 soon.

Regards,
Lorenzo

> 
> On Thu, Oct 19, 2023 at 7:47 PM Lorenzo Bianconi <
> lorenzo.bianc...@redhat.com> wrote:
> 
> > Avoid recirculating IGMP/MLD packets more than one time from stage
> > ls_out_pre_lb in the egress pipeline to ovn table 37 in order to avoid
> > packet looping for ovn-ic deployment.
> >
> > Signed-off-by: Lorenzo Bianconi 
> > ---
> >  controller/pinctrl.c |  2 ++
> >  include/ovn/logical-fields.h |  3 +++
> >  lib/logical-fields.c |  4 
> >  northd/northd.c  | 11 +++
> >  4 files changed, 16 insertions(+), 4 deletions(-)
> >
> > diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> > index 3c1cecfde..d4ae7de46 100644
> > --- a/controller/pinctrl.c
> > +++ b/controller/pinctrl.c
> > @@ -650,6 +650,8 @@ pinctrl_forward_pkt(struct rconn *swconn, int64_t
> > dp_key,
> >  put_load(dp_key, MFF_LOG_DATAPATH, 0, 64, &ofpacts);
> >  put_load(in_port_key, MFF_LOG_INPORT, 0, 32, &ofpacts);
> >  put_load(out_port_key, MFF_LOG_OUTPORT, 0, 32, &ofpacts);
> > +/* Avoid re-injecting packet already consumed. */
> > +put_load(1, MFF_LOG_FLAGS, MLF_IGMP_IGMP_SPOOF_INJECT_BIT, 1,
> > &ofpacts);
> >
> >  struct ofpact_resubmit *resubmit = ofpact_put_RESUBMIT(&ofpacts);
> >  resubmit->in_port = OFPP_CONTROLLER;
> > diff --git a/include/ovn/logical-fields.h b/include/ovn/logical-fields.h
> > index a7b64ef67..0652af380 100644
> > --- a/include/ovn/logical-fields.h
> > +++ b/include/ovn/logical-fields.h
> > @@ -77,6 +77,7 @@ enum mff_log_flags_bits {
> >  MLF_CHECK_PORT_SEC_BIT = 12,
> >  MLF_LOOKUP_COMMIT_ECMP_NH_BIT = 13,
> >  MLF_USE_LB_AFF_SESSION_BIT = 14,
> > +MLF_IGMP_IGMP_SPOOF_INJECT_BIT = 15,
> >  };
> >
> >  /* MFF_LOG_FLAGS_REG flag assignments */
> > @@ -124,6 +125,8 @@ enum mff_log_flags {
> >  MLF_LOOKUP_COMMIT_ECMP_NH = (1 << MLF_LOOKUP_COMMIT_ECMP_NH_BIT),
> >
> >  MLF_USE_LB_AFF_SESSION = (1 << MLF_USE_LB_AFF_SESSION_BIT),
> > +
> > +MLF_IGMP_IGMP_SPOOF = (1 << MLF_IGMP_IGMP_SPOOF_INJECT_BIT),
> >  };
> >
> >  /* OVN logical fields
> > diff --git a/lib/logical-fields.c b/lib/logical-fields.c
> > index fd509d9ee..3d8d5f950 100644
> > --- a/lib/logical-fields.c
> > +++ b/lib/logical-fields.c
> > @@ -129,6 +129,10 @@ ovn_init_symtab(struct shash *symtab)
> >   MLF_USE_SNAT_ZONE);
> >  expr_symtab_add_subfield(symtab, "flags.use_snat_zone", NULL,
> >   flags_str);
> > +snprintf(flags_str, sizeof flags_str, "flags[%d]",
> > + MLF_IGMP_IGMP_SPOOF_INJECT_BIT);
> > +expr_symtab_add_subfield(symtab, "flags.igmp_loopback", NULL,
> > + flags_str);
> >
> >  /* Connection tracking state. */
> >  expr_symtab_add_field_scoped(symtab, "ct_mark", MFF_CT_MARK, NULL,
> > false,
> > diff --git a/northd/northd.c b/northd/northd.c
> > index 916068d44..557fcca72 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -7323,7 +7323,8 @@ build_interconn_mcast_snoop_flows(struct
> > ovn_datapath *od,
> >  continue;
> >  }
> >  /* Punt IGMP traffic to controller. */
> > -char *match = xasprintf("inport == %s && igmp", op->json_key);
> > +char *match = xasprintf("inport == %s && igmp && "
> > +"flags.igmp_loopback == 0", op->json_key);
> >  ovn_lflow_metered(lflows, od, S_SWITCH_OUT_PRE_LB, 120, match,
> >"clone { igmp; }; next;",
> >copp_meter_get(COPP_IGMP, od->nbs->copp,
> > @@ -7331,7 +7332,8 @@ build_interconn_mcast_snoop_flows(struct
> > ovn_datapath *od,
> >  free(match);
> >
> >  /* Punt MLD traffic to controller. */
> > -match = xasprintf("inport == %s && (mldv1 || mldv2)",
> > op->json_key);
> > +match = xasprintf("inport == %s && (mldv1 || mldv2) && "
> > +  "flags.igmp_loopback == 0", op->json_key);
> >  ovn_lflow_metered(lflows, od, S_SWITCH_OUT_PRE_LB, 120, match,
> >"clone { igmp; }; next;",
> >copp_meter_get(COPP_IGMP, od->nbs->copp,
> > @@ -10317,13 +10319,14 @@ build_lswitch_destination_lookup_bmcast(struct
> > ovn_datapath *od,
> >  ds_put_cstr(actions, "igmp;");
> >  /* Punt IGMP traffic to controller. */
> >  ovn_lflow_metered(lflows, od, S_SWITCH_IN_L2_LKUP, 100,
> > -  "igmp", ds_cstr(actions),
> > +  "flags.igmp_loopback == 0 && igmp",
> > ds_cstr(actions),
> >copp_meter_get(COPP_IGMP, od->nbs->copp,
> >   meter_groups));
> >
> >  /* Punt MLD tra

Re: [ovs-dev] [PATCH v11 1/5] dpif: Stub out unimplemented action OVS_ACTION_ATTR_DEC_TTL.

[Eelco Chaudron]

On 11 Mar 2024, at 18:51, Eric Garver wrote:

> This is prep for adding a different OVS_ACTION_ATTR_ enum value. This
> action, OVS_ACTION_ATTR_DEC_TTL, is not actually implemented. However,
> to make -Werror happy we must add a case to all existing switches.
>
> Signed-off-by: Eric Garver 

Thanks Eric for following up on this series!!

This patch looks fine to me.

Acked-by: Eelco Chaudron 

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

Re: [ovs-dev] [PATCH ovn v3] controller: Release container lport when releasing parent port.

[Numan Siddique]

On Wed, Feb 28, 2024 at 5:43 AM Mohammad Heib  wrote:
>
> Currently if the user sets the container parent_port:requested-chassis
> option after the VIF/CIF is bonded to the chassis, this will migrate
> the VIF/CIF flows to the new chassis but will still have the
> container flows installed in the old chassis which can allow unwanted
> tagged traffic to reach VMS/containers on the old chassis.
>
> This patch will resolve the above issue by remove the CIF flows
> from the old chassis and prevent the CIF from being bonded to a
> chassis different from the parent port VIF binding chassis.
>
> Rreported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2220938
> Signed-off-by: Mohammad Heib 

Thanks v3.  I applied this patch to main and backported it down till
branch-22.03 with the below small change.


--
diff --git a/controller/binding.c b/controller/binding.c
index c2d15a6c43..8ac2ce3e2b 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -1709,10 +1709,10 @@ consider_container_lport(const struct
sbrec_port_binding *pb,
 }

 ovs_assert(parent_b_lport && parent_b_lport->pb);
-bool can_bind = lport_can_bind_on_this_chassis(b_ctx_in->chassis_rec, pb);
 /* cannot bind to this chassis if the parent_port cannot be bounded. */
-can_bind &= lport_can_bind_on_this_chassis(b_ctx_in->chassis_rec,
-   parent_b_lport->pb);
+bool can_bind = lport_can_bind_on_this_chassis(b_ctx_in->chassis_rec,
+   parent_b_lport->pb) &&
+lport_can_bind_on_this_chassis(b_ctx_in->chassis_rec, pb);

 return consider_vif_lport_(pb, can_bind, b_ctx_in, b_ctx_out,
container_b_lport);

--

Thanks
Numan

> ---
>  controller/binding.c  |  3 +++
>  controller/physical.c |  9 
>  tests/ovn.at  | 53 +++
>  3 files changed, 65 insertions(+)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index 2afc5d48a..c2d15a6c4 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -1710,6 +1710,9 @@ consider_container_lport(const struct 
> sbrec_port_binding *pb,
>
>  ovs_assert(parent_b_lport && parent_b_lport->pb);
>  bool can_bind = lport_can_bind_on_this_chassis(b_ctx_in->chassis_rec, 
> pb);
> +/* cannot bind to this chassis if the parent_port cannot be bounded. */
> +can_bind &= lport_can_bind_on_this_chassis(b_ctx_in->chassis_rec,
> +   parent_b_lport->pb);
>
>  return consider_vif_lport_(pb, can_bind, b_ctx_in, b_ctx_out,
> container_b_lport);
> diff --git a/controller/physical.c b/controller/physical.c
> index 7ef259da4..86d4b4578 100644
> --- a/controller/physical.c
> +++ b/controller/physical.c
> @@ -1631,6 +1631,15 @@ consider_port_binding(struct ovsdb_idl_index 
> *sbrec_port_binding_by_name,
>  nested_container = true;
>  parent_port = lport_lookup_by_name(
>  sbrec_port_binding_by_name, binding->parent_port);
> +
> +if (parent_port
> +&& !lport_can_bind_on_this_chassis(chassis, parent_port)) {
> +/* Even though there is an ofport for this container
> + * parent port, it is requested on different chassis ignore
> + * this container port.
> + */
> +return;
> +}
>  }
>  } else if (!strcmp(binding->type, "localnet")
>   || !strcmp(binding->type, "l2gateway")) {
> diff --git a/tests/ovn.at b/tests/ovn.at
> index d26c95054..6f0fc1043 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -38351,3 +38351,56 @@ OVS_WAIT_UNTIL([test 1 = $(as hv ovs-ofctl 
> dump-flows br-int | grep -E "pkt_mark
>  OVN_CLEANUP([hv])
>  AT_CLEANUP
>  ])
> +
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([ovn-controller - cleanup VIF/CIF related flows/fields when 
> updating requested-chassis])
> +ovn_start
> +
> +net_add n1
> +sim_add hv1
> +ovs-vsctl add-br br-phys
> +ovn_attach n1 br-phys 192.168.0.1
> +check ovs-vsctl -- add-port br-int vif1 -- \
> +set Interface vif1 external-ids:iface-id=lsp1 \
> +ofport-request=8
> +
> +check ovn-nbctl ls-add lsw0
> +
> +check ovn-nbctl lsp-add lsw0 lsp1
> +check ovn-nbctl lsp-add lsw0 sw0-port1.1 lsp1 7
> +
> +# wait for the VIF to be claimed to this chassis
> +wait_row_count Chassis 1 name=hv1
> +hv1_uuid=$(fetch_column Chassis _uuid name=hv1)
> +wait_for_ports_up lsp1
> +wait_for_ports_up sw0-port1.1
> +wait_column "$hv1_uuid" Port_Binding chassis logical_port=lsp1
> +wait_column "$hv1_uuid" Port_Binding chassis logical_port=sw0-port1.1
> +
> +# check that flows is installed
> +OVS_WAIT_FOR_OUTPUT([as hv1 ovs-ofctl dump-flows br-int table=0 |grep 
> priority=100 | grep -c in_port=8], [0],[dnl
> +1
> +])
> +OVS_WAIT_FOR_OUTPUT([as hv1 ovs-ofctl dump-flow

Re: [ovs-dev] [PATCH v2 12/12] documentation: Document ovs-flowviz.

[Eelco Chaudron]

On 13 Mar 2024, at 10:03, 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 


Thank you for including this documentation. It provides a nice insight into how 
to use the ovs-flowviz tool.

I have a few grammar suggestions based on my review as a non-native speaker. 
Please note that these suggestions may not all be applicable, but I hope they 
help improve clarity.

Cheers,

Eelco

> ---
>  Documentation/automake.mk   |   4 +-
>  Documentation/conf.py   |   2 +
>  Documentation/ref/index.rst |   1 +
>  Documentation/ref/ovs-flowviz.8.rst | 531 
>  Documentation/topics/flow-visualization.rst | 271 ++
>  Documentation/topics/index.rst  |   1 +
>  6 files changed, 809 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/ref/ovs-flowviz.8.rst
>  create mode 100644 Documentation/topics/flow-visualization.rst
>
> diff --git a/Documentation/automake.mk b/Documentation/automake.mk
> index 47d2e336a..539870aa2 100644
> --- a/Documentation/automake.mk
> +++ b/Documentation/automake.mk
> @@ -45,7 +45,7 @@ DOC_SOURCE = \
>   Documentation/topics/fuzzing/ovs-fuzzing-infrastructure.rst \
>   Documentation/topics/fuzzing/ovs-fuzzers.rst \
>   Documentation/topics/fuzzing/security-analysis-of-ovs-fuzzers.rst \
> - Documentation/topics/testing.rst \
> + Documentation/topics/flow-visualization.rst \
>   Documentation/topics/integration.rst \
>   Documentation/topics/language-bindings.rst \
>   Documentation/topics/networking-namespaces.rst \
> @@ -55,6 +55,7 @@ DOC_SOURCE = \
>   Documentation/topics/ovsdb-replication.rst \
>   Documentation/topics/porting.rst \
>   Documentation/topics/record-replay.rst \
> + Documentation/topics/testing.rst \
>   Documentation/topics/tracing.rst \
>   Documentation/topics/usdt-probes.rst \
>   Documentation/topics/userspace-checksum-offloading.rst \
> @@ -162,6 +163,7 @@ RST_MANPAGES = \
>   ovs-actions.7.rst \
>   ovs-appctl.8.rst \
>   ovs-ctl.8.rst \
> + ovs-flowviz.8.rst \
>   ovs-l3ping.8.rst \
>   ovs-parse-backtrace.8.rst \
>   ovs-pki.8.rst \
> diff --git a/Documentation/conf.py b/Documentation/conf.py
> index 085ca2cd6..e41cf6031 100644
> --- a/Documentation/conf.py
> +++ b/Documentation/conf.py
> @@ -120,6 +120,8 @@ _man_pages = [
>   u'utility for configuring running Open vSwitch daemons'),
>  ('ovs-ctl.8',
>   u'OVS startup helper script'),
> +('ovs-flowviz.8',
> + u'utility for visualizing OpenFlow and datapath flows'),
>  ('ovs-l3ping.8',
>   u'check network deployment for L3 tunneling problems'),
>  ('ovs-parse-backtrace.8',
> diff --git a/Documentation/ref/index.rst b/Documentation/ref/index.rst
> index 03ada932f..7f2fe6177 100644
> --- a/Documentation/ref/index.rst
> +++ b/Documentation/ref/index.rst
> @@ -42,6 +42,7 @@ time:
> ovs-actions.7
> ovs-appctl.8
> ovs-ctl.8
> +   ovs-flowviz.8
> ovs-l3ping.8
> ovs-pki.8
> ovs-sim.1
> diff --git a/Documentation/ref/ovs-flowviz.8.rst 
> b/Documentation/ref/ovs-flowviz.8.rst
> new file mode 100644
> index 0..da1135918
> --- /dev/null
> +++ b/Documentation/ref/ovs-flowviz.8.rst
> @@ -0,0 +1,531 @@
> +..
> +  Licensed under the Apache License, Version 2.0 (the "License"); you may
> +  not use this file except in compliance with the License. You may obtain
> +  a copy of the License at
> +
> +  http://www.apache.org/licenses/LICENSE-2.0
> +
> +  Unless required by applicable law or agreed to in writing, software
> +  distributed under the License is distributed on an "AS IS" BASIS, 
> WITHOUT
> +  WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See 
> the
> +  License for the specific language governing permissions and limitations
> +  under the License.
> +
> +  Convention for heading levels in Open vSwitch documentation:
> +
> +  ===  Heading 0 (reserved for the title in a document)
> +  ---  Heading 1
> +  ~~~  Heading 2
> +  +++  Heading 3
> +  '''  Heading 4
> +
> +  Avoid deeper levels because they do not render well.
> +
> +===
> +ovs-flowviz
> +===
> +
> +Synopsis
> +
> +
> +``ovs-flowviz``
> +[``[-i | --input] <[alias,]file>``]
> +[``[-c | --config] ``]
> +[``[-f | --filter] ``]
> +[``[-h | --highlight] ``]
> +[``--style 

Re: [ovs-dev] [PATCH v2 03/12] python: ovs: flowviz: Add console formatting.

[Adrian Moreno]

On 3/15/24 12:29, Eelco Chaudron wrote:



On 13 Mar 2024, at 10:03, Adrian Moreno wrote:


Add a flow formatting framework and one implementation for console
printing using rich.

The flow formatting framework is a simple set of classes that can be
used to write different flow formatting implementations. It supports
styles to be described by any class, highlighting and config-file based
style definition.

The first flow formatting implementation is also introduced: the
ConsoleFormatter. It uses the an advanced rich-text printing library
[1].

The console printing supports:
- Heatmap: printing the packet/byte statistics of each flow in a color
   that represents its relative size: blue (low) -> red (high).
- Printing a banner with the file name and alias.
- Extensive style definition via config file.

This console format is added to both OpenFlow and Datapath flows.

Examples:
- Highlight drops in datapath flows:
$ ovs-flowviz -i flows.txt --highlight "drop" datapath console
- Quickly detect where most packets are going using heatmap and
   paginated output:
$ ovs-ofctl dump-flows br-int | ovs-flowviz openflow console -h

[1] https://rich.readthedocs.io/en/stable/introduction.html

Signed-off-by: Adrian Moreno 


Thanks for making these changes, one small nit. Guess your cursor was at a 
different place than you thought it was :)

If this is the only change in your next rev, add my ‘Acked-by: Eelco Chaudron 
’.

//Eelco


---
  python/automake.mk|   2 +
  python/ovs/flowviz/console.py | 175 
  python/ovs/flowviz/format.py  | 371 ++
  python/ovs/flowviz/main.py|  58 +-
  python/ovs/flowviz/odp/cli.py |  25 +++
  python/ovs/flowviz/ofp/cli.py |  26 +++
  python/ovs/flowviz/process.py |  83 +++-
  python/setup.py   |   4 +-
  8 files changed, 736 insertions(+), 8 deletions(-)
  create mode 100644 python/ovs/flowviz/console.py
  create mode 100644 python/ovs/flowviz/format.py

diff --git a/python/automake.mk b/python/automake.mk
index fd5e74081..bd53c5405 100644
--- a/python/automake.mk
+++ b/python/automake.mk
@@ -65,6 +65,8 @@ ovs_pytests = \

  ovs_flowviz = \
python/ovs/flowviz/__init__.py \
+   python/ovs/flowviz/console.py \
+   python/ovs/flowviz/format.py \
python/ovs/flowviz/main.py \
python/ovs/flowviz/odp/__init__.py \
python/ovs/flowviz/odp/cli.py \
diff --git a/python/ovs/flowviz/console.py b/python/ovs/flowviz/console.py
new file mode 100644
index 0..4a3443360
--- /dev/null
+++ b/python/ovs/flowviz/console.py
@@ -0,0 +1,175 @@
+# Copyright (c) 2023 Red Hat, Inc.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at:
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import colorsys
+
+from rich.console import Console
+from rich.color import Color
+from rich.emoji import Emoji
+from rich.panel import Panel
+from rich.text import Text
+from rich.style import Style
+
+from ovs.flowviz.format import FlowFormatter, FlowBuffer, FlowStyle
+
+
+def file_header(name):
+return Panel(
+Text(
+Emoji.replace(":scroll:")
++ " "
++ name
++ " "
++ Emoji.replace(":scroll:"),
+style="bold",
+justify="center",
+)
+)
+
+
+class ConsoleBuffer(FlowBuffer):
+"""ConsoleBuffer implements FlowBuffer to provide console-based text
+formatting based on rich.Text.
+
+Append functions accept a rich.Style.
+
+Args:
+rtext(rich.Text): Optional; text instance to reuse
+"""
+
+def __init__(self, rtext):
+self._text = rtext or Text()
+
+@property
+def text(self):
+return self._text
+
+def _append(self, string, style):
+"""Append to internal text."""
+return self._text.append(string, style)
+
+def append_key(self, kv, style):
+"""Append a key.
+Args:
+kv (KeyValue): the KeyValue instance to append
+style (rich.Style): the style to use
+"""
+return self._append(kv.meta.kstring, style)
+
+def append_delim(self, kv, style):
+"""Append a delimiter.
+Args:
+kv (KeyValue): the KeyValue instance to append
+style (rich.Style): the style to use
+"""
+return self._append(kv.meta.delim, style)
+
+def append_end_delim(self, kv, style):
+"""Append an end delimiter.
+Args:
+kv (KeyValue): the KeyValue instance to append
+styl

Re: [ovs-dev] [PATCH v2 03/12] python: ovs: flowviz: Add console formatting.

[Eelco Chaudron]

On 13 Mar 2024, at 10:03, Adrian Moreno wrote:

> Add a flow formatting framework and one implementation for console
> printing using rich.
>
> The flow formatting framework is a simple set of classes that can be
> used to write different flow formatting implementations. It supports
> styles to be described by any class, highlighting and config-file based
> style definition.
>
> The first flow formatting implementation is also introduced: the
> ConsoleFormatter. It uses the an advanced rich-text printing library
> [1].
>
> The console printing supports:
> - Heatmap: printing the packet/byte statistics of each flow in a color
>   that represents its relative size: blue (low) -> red (high).
> - Printing a banner with the file name and alias.
> - Extensive style definition via config file.
>
> This console format is added to both OpenFlow and Datapath flows.
>
> Examples:
> - Highlight drops in datapath flows:
> $ ovs-flowviz -i flows.txt --highlight "drop" datapath console
> - Quickly detect where most packets are going using heatmap and
>   paginated output:
> $ ovs-ofctl dump-flows br-int | ovs-flowviz openflow console -h
>
> [1] https://rich.readthedocs.io/en/stable/introduction.html
>
> Signed-off-by: Adrian Moreno 

Thanks for making these changes, one small nit. Guess your cursor was at a 
different place than you thought it was :)

If this is the only change in your next rev, add my ‘Acked-by: Eelco Chaudron 
’.

//Eelco

> ---
>  python/automake.mk|   2 +
>  python/ovs/flowviz/console.py | 175 
>  python/ovs/flowviz/format.py  | 371 ++
>  python/ovs/flowviz/main.py|  58 +-
>  python/ovs/flowviz/odp/cli.py |  25 +++
>  python/ovs/flowviz/ofp/cli.py |  26 +++
>  python/ovs/flowviz/process.py |  83 +++-
>  python/setup.py   |   4 +-
>  8 files changed, 736 insertions(+), 8 deletions(-)
>  create mode 100644 python/ovs/flowviz/console.py
>  create mode 100644 python/ovs/flowviz/format.py
>
> diff --git a/python/automake.mk b/python/automake.mk
> index fd5e74081..bd53c5405 100644
> --- a/python/automake.mk
> +++ b/python/automake.mk
> @@ -65,6 +65,8 @@ ovs_pytests = \
>
>  ovs_flowviz = \
>   python/ovs/flowviz/__init__.py \
> + python/ovs/flowviz/console.py \
> + python/ovs/flowviz/format.py \
>   python/ovs/flowviz/main.py \
>   python/ovs/flowviz/odp/__init__.py \
>   python/ovs/flowviz/odp/cli.py \
> diff --git a/python/ovs/flowviz/console.py b/python/ovs/flowviz/console.py
> new file mode 100644
> index 0..4a3443360
> --- /dev/null
> +++ b/python/ovs/flowviz/console.py
> @@ -0,0 +1,175 @@
> +# Copyright (c) 2023 Red Hat, Inc.
> +#
> +# Licensed under the Apache License, Version 2.0 (the "License");
> +# you may not use this file except in compliance with the License.
> +# You may obtain a copy of the License at:
> +#
> +# http://www.apache.org/licenses/LICENSE-2.0
> +#
> +# Unless required by applicable law or agreed to in writing, software
> +# distributed under the License is distributed on an "AS IS" BASIS,
> +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> +# See the License for the specific language governing permissions and
> +# limitations under the License.
> +
> +import colorsys
> +
> +from rich.console import Console
> +from rich.color import Color
> +from rich.emoji import Emoji
> +from rich.panel import Panel
> +from rich.text import Text
> +from rich.style import Style
> +
> +from ovs.flowviz.format import FlowFormatter, FlowBuffer, FlowStyle
> +
> +
> +def file_header(name):
> +return Panel(
> +Text(
> +Emoji.replace(":scroll:")
> ++ " "
> ++ name
> ++ " "
> ++ Emoji.replace(":scroll:"),
> +style="bold",
> +justify="center",
> +)
> +)
> +
> +
> +class ConsoleBuffer(FlowBuffer):
> +"""ConsoleBuffer implements FlowBuffer to provide console-based text
> +formatting based on rich.Text.
> +
> +Append functions accept a rich.Style.
> +
> +Args:
> +rtext(rich.Text): Optional; text instance to reuse
> +"""
> +
> +def __init__(self, rtext):
> +self._text = rtext or Text()
> +
> +@property
> +def text(self):
> +return self._text
> +
> +def _append(self, string, style):
> +"""Append to internal text."""
> +return self._text.append(string, style)
> +
> +def append_key(self, kv, style):
> +"""Append a key.
> +Args:
> +kv (KeyValue): the KeyValue instance to append
> +style (rich.Style): the style to use
> +"""
> +return self._append(kv.meta.kstring, style)
> +
> +def append_delim(self, kv, style):
> +"""Append a delimiter.
> +Args:
> +kv (KeyValue): the KeyValue instance to append
> +style (rich.Style): the style to use
> +"""
> +return self._append(kv.

Re: [ovs-dev] [PATCH ovn 2/2] IC: Tansit switch don't flood mcast traffic to router ports if matches igmp group.

[0-day Robot]

Bleep bloop.  Greetings Mohammad Heib, 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: The subject, ': ', is over 70 characters, i.e., 82.
Subject: IC: Tansit switch don't flood mcast traffic to router ports if matches 
igmp group.
Lines checked: 105, 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

[ovs-dev] [PATCH ovn 2/2] IC: Tansit switch don't flood mcast traffic to router ports if matches igmp group.

[Mohammad Heib]

Crrently ovn transit switch forward mcast traffic that match an igmp
group to all ports participating in this group and to all router ports
that are connected to this TS switch and have mcast_flood enabled.

The above behavior can lead to packet duplicate if we have a VM in
a specific AZ that participates in igmp group because the gateway router
in this AZ will forward igmp membership report from the VM to the TS
which will be learned as an IGMP_group on the Tansit switch in different
AZs and every mcast traffic to that igmp group address from the different
AZs will be handled by the Tansit switch twice:
 - First time TS will send the traffic according to the igmp group
   which will reach the VM.

 - Second time TS will send the traffic to all router ports including
   the router that exists on the VM AZ which will forward the traffic
   to the VM again.

To avoid this issue this patch adds flows that forward mcast traffic
that match igmp group to the igmp group ports only, this flows only
apply to Transit switches.

Rreported-at: https://issues.redhat.com/browse/FDP-101
Signed-off-by: Mohammad Heib 
---
 northd/northd.c | 18 ++
 northd/ovn-northd.8.xml |  7 +++
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/northd/northd.c b/northd/northd.c
index 98c837a20..0c5122d27 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -9362,8 +9362,14 @@ build_lswitch_destination_lookup_bmcast(struct 
ovn_datapath *od,
 }
 
 
-/* Ingress table 25: Add IP multicast flows learnt from IGMP/MLD
- * (priority 90). */
+/* Ingress table 27: Add IP multicast flows learnt from IGMP/MLD
+ * (priority 90).
+ *
+ * Ingress table 27: Transit switch add IP multicast flows learnt
+ * from IGMP/MLD to forward traffic explicitly to the ports that are
+ * part of the IGMP/MLD group, and ignore MROUTERAS Ports.
+ * (priority 95).
+ */
 static void
 build_lswitch_ip_mcast_igmp_mld(struct ovn_igmp_group *igmp_group,
 struct lflow_table *lflows,
@@ -9377,6 +9383,9 @@ build_lswitch_ip_mcast_igmp_mld(struct ovn_igmp_group 
*igmp_group,
 ds_clear(match);
 ds_clear(actions);
 
+bool transit_switch =
+ovn_datapath_is_transit_switch(igmp_group->datapath);
+
 struct mcast_switch_info *mcast_sw_info =
 &igmp_group->datapath->mcast_info.sw;
 uint64_t table_size = mcast_sw_info->table_size;
@@ -9422,7 +9431,7 @@ build_lswitch_ip_mcast_igmp_mld(struct ovn_igmp_group 
*igmp_group,
 }
 
 /* Also flood traffic to all multicast routers with relay enabled. */
-if (mcast_sw_info->flood_relay) {
+if (mcast_sw_info->flood_relay && !transit_switch) {
 ds_put_cstr(actions,
 "clone { "
 "outport = \""MC_MROUTER_FLOOD "\"; "
@@ -9440,7 +9449,8 @@ build_lswitch_ip_mcast_igmp_mld(struct ovn_igmp_group 
*igmp_group,
   igmp_group->mcgroup.name);
 
 ovn_lflow_add(lflows, igmp_group->datapath, S_SWITCH_IN_L2_LKUP,
-  90, ds_cstr(match), ds_cstr(actions), NULL);
+  transit_switch? 95 : 90, ds_cstr(match),
+  ds_cstr(actions), NULL);
 }
 }
 
diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
index 17b414144..e25285d67 100644
--- a/northd/ovn-northd.8.xml
+++ b/northd/ovn-northd.8.xml
@@ -1933,6 +1933,13 @@ output;
 logical switch.
   
 
+  
+Priority-95 flows for transit switches only that forward registered
+IP multicast traffic to their corresponding multicast group , which
+ovn-northd creates based on learnt
+ entries.
+  
+
   
 Priority-90 flows that forward registered IP multicast traffic to
 their corresponding multicast group, which ovn-northd
-- 
2.34.3

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

[ovs-dev] [PATCH ovn 1/2] northd: Don't skip transit switch LSP when creating mcast groups.

[Mohammad Heib]

Currently when we enable IGMP on OVN-IC cluster with two or more AZs
and one vm from AZ1 send IGMP report, northd will create the following
multicast_group on each AZ:

AZ1:
 1. multicast_group that forward the mcast traffic from LS1 to the VM.
 2. multicast_group that forward the mcast traffic from LR1 to the LS1.

AZ2:
 1. multicast_group that forward the mcast traffic from TS to LR1 in
AZ1.

This design works fine if we have one logical network only on each AZ,
but if we have two or more logical network on the same AZ that separated
from each other and only connected via transit switch and both join the
same mcast network, the traffic will be delivered between those two
networks because ovn floods it to all routers that connected to the
transit switch (see logical flow table 27).

The above design is not the right way to handle such mcast traffic
because future changes for ovn that make it match explicitly on the igmp
group address and not forward traffic to routers can break the mcast
traffic in ovn-ic.

This patch updates the above design by adding the router port that
connects to the transit switch to the multicast_group even if the peer
port have mcast_flood enabled.

Signed-off-by: Mohammad Heib 
---
 northd/northd.c |  8 +---
 northd/northd.h |  6 ++
 tests/ovn.at| 10 ++
 3 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/northd/northd.c b/northd/northd.c
index 1839b7d8b..98c837a20 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -5377,11 +5377,13 @@ ovn_igmp_group_get_ports(const struct sbrec_igmp_group 
*sb_igmp_group,
 continue;
 }
 
-/* If this is already a port of a router on which relay is enabled,
- * skip it for the group. Traffic is flooded there anyway.
+/* If this is already a port of a router on which relay is enabled
+ * and it's not a transit switch to router port,skip it for the group.
+ * Traffic is flooded there anyway.
  */
 if (port->peer && port->peer->od &&
-port->peer->od->mcast_info.rtr.relay) {
+port->peer->od->mcast_info.rtr.relay &&
+!ovn_datapath_is_transit_switch(port->od)) {
 continue;
 }
 
diff --git a/northd/northd.h b/northd/northd.h
index 3f1cd8341..5e9fa4745 100644
--- a/northd/northd.h
+++ b/northd/northd.h
@@ -362,6 +362,12 @@ ovn_datapath_is_stale(const struct ovn_datapath *od)
 return !od->nbr && !od->nbs;
 };
 
+static inline bool
+ovn_datapath_is_transit_switch(const struct ovn_datapath *od)
+{
+return od->tunnel_key >= OVN_MIN_DP_KEY_GLOBAL;
+}
+
 /* Pipeline stages. */
 
 /* The two purposes for which ovn-northd uses OVN logical datapaths. */
diff --git a/tests/ovn.at b/tests/ovn.at
index 438c7690a..4a9e433b2 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -26903,10 +26903,20 @@ wait_row_count IGMP_Group 2 address=239.0.1.68
 wait_row_count IGMP_Group 2 address='"ff0a:dead:beef::1"'
 check ovn-nbctl --wait=hv sync
 
+#Validate that Multicast Group contains all registered ports for
+# specific igmp group.
+ts_dp=$(fetch_column datapath_binding _uuid external_ids:name=ts)
+ports=$(fetch_column multicast_group ports name="239.0.1.68" datapath=$ts_dp)
+check test X2 = X$(echo $ports | wc -w)
+
+
 ovn_as az2
 wait_row_count IGMP_Group 2 address=239.0.1.68
 wait_row_count IGMP_Group 2 address='"ff0a:dead:beef::1"'
 check ovn-nbctl --wait=hv sync
+ts_dp=$(fetch_column datapath_binding _uuid external_ids:name=ts)
+ports=$(fetch_column multicast_group ports name="239.0.1.68" datapath=$ts_dp)
+check test X2 = X$(echo $ports | wc -w)
 
 # Send an IP multicast packet from LSP2, it should be forwarded
 # to lsp1 and lsp3.
-- 
2.34.3

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

Re: [ovs-dev] [RFC ovn] ovn-ic: avoid igmp/mld traffic flooding

[Mohammad Heib]

Hi Lorenzo,

i have applied and tested this change and i can confirm that it fixes the
issue.
if you please can rebase on top of the main.


Acked-by: Mohammad Heib 

On Thu, Oct 19, 2023 at 7:47 PM Lorenzo Bianconi <
lorenzo.bianc...@redhat.com> wrote:

> Avoid recirculating IGMP/MLD packets more than one time from stage
> ls_out_pre_lb in the egress pipeline to ovn table 37 in order to avoid
> packet looping for ovn-ic deployment.
>
> Signed-off-by: Lorenzo Bianconi 
> ---
>  controller/pinctrl.c |  2 ++
>  include/ovn/logical-fields.h |  3 +++
>  lib/logical-fields.c |  4 
>  northd/northd.c  | 11 +++
>  4 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index 3c1cecfde..d4ae7de46 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -650,6 +650,8 @@ pinctrl_forward_pkt(struct rconn *swconn, int64_t
> dp_key,
>  put_load(dp_key, MFF_LOG_DATAPATH, 0, 64, &ofpacts);
>  put_load(in_port_key, MFF_LOG_INPORT, 0, 32, &ofpacts);
>  put_load(out_port_key, MFF_LOG_OUTPORT, 0, 32, &ofpacts);
> +/* Avoid re-injecting packet already consumed. */
> +put_load(1, MFF_LOG_FLAGS, MLF_IGMP_IGMP_SPOOF_INJECT_BIT, 1,
> &ofpacts);
>
>  struct ofpact_resubmit *resubmit = ofpact_put_RESUBMIT(&ofpacts);
>  resubmit->in_port = OFPP_CONTROLLER;
> diff --git a/include/ovn/logical-fields.h b/include/ovn/logical-fields.h
> index a7b64ef67..0652af380 100644
> --- a/include/ovn/logical-fields.h
> +++ b/include/ovn/logical-fields.h
> @@ -77,6 +77,7 @@ enum mff_log_flags_bits {
>  MLF_CHECK_PORT_SEC_BIT = 12,
>  MLF_LOOKUP_COMMIT_ECMP_NH_BIT = 13,
>  MLF_USE_LB_AFF_SESSION_BIT = 14,
> +MLF_IGMP_IGMP_SPOOF_INJECT_BIT = 15,
>  };
>
>  /* MFF_LOG_FLAGS_REG flag assignments */
> @@ -124,6 +125,8 @@ enum mff_log_flags {
>  MLF_LOOKUP_COMMIT_ECMP_NH = (1 << MLF_LOOKUP_COMMIT_ECMP_NH_BIT),
>
>  MLF_USE_LB_AFF_SESSION = (1 << MLF_USE_LB_AFF_SESSION_BIT),
> +
> +MLF_IGMP_IGMP_SPOOF = (1 << MLF_IGMP_IGMP_SPOOF_INJECT_BIT),
>  };
>
>  /* OVN logical fields
> diff --git a/lib/logical-fields.c b/lib/logical-fields.c
> index fd509d9ee..3d8d5f950 100644
> --- a/lib/logical-fields.c
> +++ b/lib/logical-fields.c
> @@ -129,6 +129,10 @@ ovn_init_symtab(struct shash *symtab)
>   MLF_USE_SNAT_ZONE);
>  expr_symtab_add_subfield(symtab, "flags.use_snat_zone", NULL,
>   flags_str);
> +snprintf(flags_str, sizeof flags_str, "flags[%d]",
> + MLF_IGMP_IGMP_SPOOF_INJECT_BIT);
> +expr_symtab_add_subfield(symtab, "flags.igmp_loopback", NULL,
> + flags_str);
>
>  /* Connection tracking state. */
>  expr_symtab_add_field_scoped(symtab, "ct_mark", MFF_CT_MARK, NULL,
> false,
> diff --git a/northd/northd.c b/northd/northd.c
> index 916068d44..557fcca72 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -7323,7 +7323,8 @@ build_interconn_mcast_snoop_flows(struct
> ovn_datapath *od,
>  continue;
>  }
>  /* Punt IGMP traffic to controller. */
> -char *match = xasprintf("inport == %s && igmp", op->json_key);
> +char *match = xasprintf("inport == %s && igmp && "
> +"flags.igmp_loopback == 0", op->json_key);
>  ovn_lflow_metered(lflows, od, S_SWITCH_OUT_PRE_LB, 120, match,
>"clone { igmp; }; next;",
>copp_meter_get(COPP_IGMP, od->nbs->copp,
> @@ -7331,7 +7332,8 @@ build_interconn_mcast_snoop_flows(struct
> ovn_datapath *od,
>  free(match);
>
>  /* Punt MLD traffic to controller. */
> -match = xasprintf("inport == %s && (mldv1 || mldv2)",
> op->json_key);
> +match = xasprintf("inport == %s && (mldv1 || mldv2) && "
> +  "flags.igmp_loopback == 0", op->json_key);
>  ovn_lflow_metered(lflows, od, S_SWITCH_OUT_PRE_LB, 120, match,
>"clone { igmp; }; next;",
>copp_meter_get(COPP_IGMP, od->nbs->copp,
> @@ -10317,13 +10319,14 @@ build_lswitch_destination_lookup_bmcast(struct
> ovn_datapath *od,
>  ds_put_cstr(actions, "igmp;");
>  /* Punt IGMP traffic to controller. */
>  ovn_lflow_metered(lflows, od, S_SWITCH_IN_L2_LKUP, 100,
> -  "igmp", ds_cstr(actions),
> +  "flags.igmp_loopback == 0 && igmp",
> ds_cstr(actions),
>copp_meter_get(COPP_IGMP, od->nbs->copp,
>   meter_groups));
>
>  /* Punt MLD traffic to controller. */
>  ovn_lflow_metered(lflows, od, S_SWITCH_IN_L2_LKUP, 100,
> -  "mldv1 || mldv2", ds_cstr(actions),
> +  "flags.igmp_loopback == 0 && (mldv1 || mldv2)",
> +  ds_cstr(actions),
>copp_me

Re: [ovs-dev] [PATCH v7 6/6] appctl: Add tests for unsupported output formats.

[Eelco Chaudron]

On 18 Jan 2024, at 16:26, jm...@redhat.com wrote:

> From: Jakob Meng 
>
> Signed-off-by: Jakob Meng 

Some comments below, you might want to consider adding a commit message.

> ---
>  tests/pmd.at| 5 +
>  tests/unixctl-py.at | 7 +++
>  2 files changed, 12 insertions(+)
>
> diff --git a/tests/pmd.at b/tests/pmd.at
> index cff80da15..82a514f36 100644
> --- a/tests/pmd.at
> +++ b/tests/pmd.at
> @@ -105,6 +105,11 @@ pmd thread numa_id  core_id :
>overhead: NOT AVAIL
>  ])
>
> +AT_CHECK([ovs-appctl --format json dpif-netdev/pmd-rxq-show], [2], [], [dnl
> +"dpif-netdev/pmd-rxq-show" command does not support output format "json" 
> (supported: 1, requested: 2)
> +ovs-appctl: ovs-vswitchd: server returned an error
> +])
> +

I don't see a need for adding this test separately, as it's already included in 
the test suite below. However, as mentioned earlier, we might consider adding a 
general JSON test to cover all aspects of this addition.

>  AT_CHECK([ovs-appctl dpif/show], [0], [dnl
>  dummy@ovs-dummy: hit:0 missed:0
>br0:
> diff --git a/tests/unixctl-py.at b/tests/unixctl-py.at
> index 26c137047..eeea386c7 100644
> --- a/tests/unixctl-py.at
> +++ b/tests/unixctl-py.at
> @@ -65,6 +65,13 @@ AT_CHECK([head -1 stderr], [0], [dnl
>  sed 's/ovs-appctl/appctl.py/' stderr > experr
>  AT_CHECK([PYAPPCTL_PY bond/hash mac vlan basis extra], [2], [], [experr])
>
> +AT_CHECK([APPCTL --format json dpif-netdev/pmd-rxq-show], [2], [], [stderr])

I believe here you should select a command that is unlikely to be converted to 
support JSON.
Maybe; ovs-appctl --format=json list-commands

> +AT_CHECK([head -1 stderr], [0], [dnl
> +"dpif-netdev/pmd-rxq-show" command does not support output format "json" 
> (supported: 1, requested: 2)
> +])
> +sed 's/ovs-appctl/appctl.py/' stderr > experr
> +AT_CHECK([PYAPPCTL_PY --format json dpif-netdev/pmd-rxq-show], [2], [], 
> [experr])
> +
>  OVS_VSWITCHD_STOP
>  AT_CLEANUP
>
> -- 
> 2.39.2

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

Re: [ovs-dev] [PATCH v7 5/6] appctl: Add option '--pretty' for pretty-printing JSON output.

[Eelco Chaudron]

On 18 Jan 2024, at 16:26, jm...@redhat.com wrote:

> From: Jakob Meng 
>
> Signed-off-by: Jakob Meng 

Some comments below and you might want to add a commit message to this patch.

//Eelco

> ---
>  NEWS   |  3 +++
>  lib/unixctl.c  |  4 ++--
>  lib/unixctl.h  |  1 +
>  tests/pmd.at   | 29 +++--
>  utilities/ovs-appctl.c | 22 +++---
>  5 files changed, 52 insertions(+), 7 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 94a347246..12905ac86 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -35,6 +35,9 @@ v3.3.0 - xx xxx 
> on mark and labels.
>   * Added new option [-f|--format] to choose the output format, e.g. 
> 'json'
> or 'text' (by default).
> + * Added new option [--pretty] to print JSON output in a readable 
> fashion.
> +   E.g. members of objects and elements of arrays are printed one per 
> line,
> +   with indentation.
> - Python:
>   * Added support for choosing the output format, e.g. 'json' or 'text'.
> - ovs-vsctl:
> diff --git a/lib/unixctl.c b/lib/unixctl.c
> index 3b9f300ba..1b216795d 100644
> --- a/lib/unixctl.c
> +++ b/lib/unixctl.c
> @@ -553,7 +553,7 @@ unixctl_client_create(const char *path, struct jsonrpc 
> **client)
>   * '*err' if not NULL. */
>  int
>  unixctl_client_transact(struct jsonrpc *client, const char *command, int 
> argc,
> -char *argv[], char **result, char **err)
> +char *argv[], int fmt_flags, char **result, char 
> **err)

Perhaps it would be more consistent to use format_flags here as well, mirroring 
the structure definition. Additionally, using an unsigned int might be 
preferable, as outlined below.

>  {
>  struct jsonrpc_msg *request, *reply;
>  struct json **json_args, *params;
> @@ -590,7 +590,7 @@ unixctl_client_transact(struct jsonrpc *client, const 
> char *command, int argc,
>  *result = xstrdup(json_string(reply->result));
>  } else if (reply->result->type == JSON_OBJECT ||
> reply->result->type == JSON_ARRAY) {
> -*result = json_to_string(reply->result, 0);
> +*result = json_to_string(reply->result, fmt_flags);
>  } else {
>  VLOG_WARN("%s: unexpected result type in JSON rpc reply: %s",
>jsonrpc_get_name(client),
> diff --git a/lib/unixctl.h b/lib/unixctl.h
> index 35ef6a548..fe9160894 100644
> --- a/lib/unixctl.h
> +++ b/lib/unixctl.h
> @@ -39,6 +39,7 @@ int unixctl_client_create(const char *path, struct jsonrpc 
> **client);
>  int unixctl_client_transact(struct jsonrpc *client,
>  const char *command,
>  int argc, char *argv[],
> +int fmt_flags,
>  char **result, char **error);
>
>  /* Command registration. */
> diff --git a/tests/pmd.at b/tests/pmd.at
> index 57660c407..cff80da15 100644
> --- a/tests/pmd.at
> +++ b/tests/pmd.at
> @@ -112,8 +112,33 @@ dummy@ovs-dummy: hit:0 missed:0
>  p0 1/1: (dummy-pmd: n_rxq=1, n_txq=1, numa_id=0)
>  ])
>
> -AT_CHECK([ovs-appctl --format json dpif/show], [0], [dnl
> -[[{"ofprotos":[{"name":"br0","ports":[{"netdev_type":"dummy-internal","ofp_port":"65534","odp_port":"100","netdev_config":{},"netdev_name":"br0"},{"netdev_type":"dummy-pmd","ofp_port":"1","odp_port":"1","netdev_config":{"numa_id":"0","n_txq":"1","n_rxq":"1"},"netdev_name":"p0"}]}],"name":"dummy@ovs-dummy","stats":{"n_hit":"0","n_missed":"0"}}]]])

It might be worth considering retaining this test to ensure it functions 
properly even without the pretty options. Additionally, we could think about 
adding a separate test (not in pmd.at) to be executed with make check, 
specifically testing the JSON format with both pretty and non-pretty options in 
the final patch.

> +AT_CHECK([ovs-appctl --format json --pretty dpif/show], [0], [dnl
> +[[
> +  {
> +"name": "dummy@ovs-dummy",
> +"ofprotos": [
> +  {
> +"name": "br0",
> +"ports": [
> +  {
> +"netdev_config": {
> +  },
> +"netdev_name": "br0",
> +"netdev_type": "dummy-internal",
> +"odp_port": "100",
> +"ofp_port": "65534"},
> +  {
> +"netdev_config": {
> +  "n_rxq": "1",
> +  "n_txq": "1",
> +  "numa_id": "0"},
> +"netdev_name": "p0",
> +"netdev_type": "dummy-pmd",
> +"odp_port": "1",
> +"ofp_port": "1"}]}],
> +"stats": {
> +  "n_hit": "0",
> +  "n_missed": "0"}}]]])
>
>  OVS_VSWITCHD_STOP
>  AT_CLEANUP
> diff --git a/utilities/ovs-appctl.c b/utilities/ovs-appctl.c
> index 02df8ba97..03c71ffad 100644
> --- a/utilities/ovs-appctl.c
> +++ b/utilities/ovs-appctl.c
> @@ -26,6 +26,7 @@
>  #include "daemon.h"
>  #include "dirs.h"
>  #include "openvswitch/dynamic-string.h"
> +#include "openvswitch/

Re: [ovs-dev] [PATCH v7 4/6] ofproto: Add JSON output for 'dpif/show' command.

[Eelco Chaudron]

On 18 Jan 2024, at 16:26, jm...@redhat.com wrote:

> From: Jakob Meng 
>
> The 'dpif/show' command now supports machine-readable JSON output in
> addition to the plain-text output for humans. An example would be:
>
>   ovs-appctl --format json dpif/show
>
> Reported-at: https://bugzilla.redhat.com/1824861
> Signed-off-by: Jakob Meng 

Some style comments below.

//Eelco


> ---
>  ofproto/ofproto-dpif.c | 129 -
>  tests/pmd.at   |   3 +
>  2 files changed, 119 insertions(+), 13 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 46d6c5014..fe4b8f3e1 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -20,6 +20,7 @@
>  #include "bond.h"
>  #include "bundle.h"
>  #include "byte-order.h"
> +#include "command-line.h"
>  #include "connectivity.h"
>  #include "connmgr.h"
>  #include "coverage.h"
> @@ -6423,8 +6424,103 @@ done:
>  return changed;
>  }
>
> +static struct json *
> +dpif_show_backer_json(const struct dpif_backer *backer)
> +{
> +struct json *json_backer = json_object_create();
> +
> +/* Add dpif name to JSON. */
> +json_object_put_string(json_backer, "name", dpif_name(backer->dpif));
> +
> +/* Add dpif stats to JSON. */
> +struct dpif_dp_stats dp_stats;
> +dpif_get_dp_stats(backer->dpif, &dp_stats);
> +struct json *json_dp_stats = json_object_create();

Moved the code around a bit, so it's more clear what are declarations vs logic.

/* Add dpif stats to JSON. */
struct dpif_dp_stats dp_stats;
struct json *json_dp_stats = json_object_create();

dpif_get_dp_stats(backer->dpif, &dp_stats);
json_object_put_format(json_dp_stats, "n_hit", "%"PRIu64, dp_stats.n_hit);

> +json_object_put_format(json_dp_stats, "n_hit", "%"PRIu64, 
> dp_stats.n_hit);
> +json_object_put_format(json_dp_stats, "n_missed", "%"PRIu64,
> +   dp_stats.n_missed);
> +json_object_put(json_backer, "stats", json_dp_stats);
> +
> +/* Add ofprotos to JSON. */
> +struct json *json_ofprotos = json_array_create_empty();
> +struct shash ofproto_shash;
> +shash_init(&ofproto_shash);
> +const struct shash_node **ofprotos = get_ofprotos(&ofproto_shash);

Maybe re-arrange the code a bit to have a more clear separation between
declarations and logic. I know new compilers are all fine with this
in-line definitions, but it looks more clear if at least they are grouped.
This goes for the entire function.

I'll let Ilya comment on this also, to see what his thoughts on this are.
I can make some more suggestions based on his feedback if needed.

> +for (size_t i = 0; i < shash_count(&ofproto_shash); i++) {
> +struct ofproto_dpif *ofproto = ofprotos[i]->data;
> +
> +if (ofproto->backer != backer) {
> +continue;
> +}
> +
> +struct json *json_ofproto = json_object_create();
> +
> +/* Add ofproto name to JSON array. */
> +json_object_put_string(json_ofproto, "name", ofproto->up.name);
> +
> +/* Add ofproto ports to JSON array. */
> +struct json *json_ofproto_ports = json_array_create_empty();
> +const struct shash_node **ports;
> +ports = shash_sort(&ofproto->up.port_by_name);
> +for (size_t j = 0; j < shash_count(&ofproto->up.port_by_name); j++) {
> +const struct shash_node *port = ports[j];
> +struct ofport *ofport = port->data;
> +
> +struct json * json_ofproto_port = json_object_create();
> +/* Add ofproto port netdev name to JSON sub array. */
> +json_object_put_string(json_ofproto_port, "netdev_name",
> +   netdev_get_name(ofport->netdev));
> +/* Add ofproto port ofp port to JSON sub array. */
> +json_object_put_format(json_ofproto_port, "ofp_port", "%u",
> +   ofport->ofp_port);
> +
> +/* Add ofproto port odp port to JSON sub array. */
> +odp_port_t odp_port = ofp_port_to_odp_port(ofproto,
> +   ofport->ofp_port);
> +if (odp_port != ODPP_NONE) {
> +json_object_put_format(json_ofproto_port, "odp_port",
> +   "%"PRIu32, odp_port);
> +} else {
> +json_object_put_string(json_ofproto_port, "odp_port", 
> "none");
> +}
> +
> +/* Add ofproto port netdev type to JSON sub array. */
> +json_object_put_string(json_ofproto_port, "netdev_type",
> +   netdev_get_type(ofport->netdev));
> +
> +/* Add ofproto port config to JSON sub array. */
> +struct json *json_port_config = json_object_create();
> +struct smap config;
> +smap_init(&config);
> +if (!netdev_get_config(ofport->netdev, &config)) {
> +struct

Re: [ovs-dev] [PATCH v7 2/6] python: Add global option for JSON output to Python tools.

[Eelco Chaudron]

> From: Jakob Meng 
>
> This patch introduces support for different output formats to the
> Python code, as did the previous commit for ovs-xxx tools like
> 'ovs-appctl --format json dpif/show'.
> In particular, tests/appctl.py gains a global option '-f,--format'
> which allows users to request JSON instead of plain-text for humans.
>
> This patch does not yet pass the choosen output format to commands.
> Doing so requires changes to all command_register() calls and all
> command callbacks. To improve readibility those changes have been
> split out into a follow up patch. Respectively, whenever an output
> format other than 'text' is choosen for tests/appctl.py, the script
> will fail.
>
> Reported-at: https://bugzilla.redhat.com/1824861
> Signed-off-by: Jakob Meng 

Hi Jakob,

See some comments below.

//Eelco

> ---
>  NEWS   |  2 ++
>  python/ovs/unixctl/__init__.py | 39 +-
>  python/ovs/unixctl/server.py   | 37 +++-
>  python/ovs/util.py |  8 +++
>  tests/appctl.py| 17 +++
>  tests/unixctl-py.at|  1 +
>  6 files changed, 98 insertions(+), 6 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 8631ea45e..94a347246 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -35,6 +35,8 @@ v3.3.0 - xx xxx 
> on mark and labels.
>   * Added new option [-f|--format] to choose the output format, e.g. 
> 'json'
> or 'text' (by default).
> +   - Python:
> + * Added support for choosing the output format, e.g. 'json' or 'text'.
> - ovs-vsctl:
>   * New commands 'set-zone-limit', 'del-zone-limit' and 'list-zone-limits'
> to manage the maximum number of connections in conntrack zones via
> diff --git a/python/ovs/unixctl/__init__.py b/python/ovs/unixctl/__init__.py
> index 8ee312943..6d9bd5715 100644
> --- a/python/ovs/unixctl/__init__.py
> +++ b/python/ovs/unixctl/__init__.py
> @@ -20,10 +20,14 @@ commands = {}
>
>
>  class _UnixctlCommand(object):
> -def __init__(self, usage, min_args, max_args, callback, aux):
> +# FIXME: Output format will be passed as 'output_fmts' to the command in
> +#later patch.
> +def __init__(self, usage, min_args, max_args,  # FIXME: output_fmts,
> + callback, aux):
>  self.usage = usage
>  self.min_args = min_args
>  self.max_args = max_args
> +# FIXME: self.output_fmts = output_fmts
>  self.callback = callback
>  self.aux = aux
>
> @@ -42,10 +46,13 @@ def _unixctl_help(conn, unused_argv, unused_aux):
>  conn.reply(reply)
>
>
> -def command_register(name, usage, min_args, max_args, callback, aux):
> +def command_register_fmt(name, usage, min_args, max_args, output_fmts,
> + callback, aux):
>  """ Registers a command with the given 'name' to be exposed by the
>  UnixctlServer. 'usage' describes the arguments to the command; it is used
> -only for presentation to the user in "help" output.
> +only for presentation to the user in "help" output.  'output_fmts' is a
> +bitmap that defines what output formats a command supports, e.g.
> +OVS_OUTPUT_FMT_TEXT | OVS_OUTPUT_FMT_JSON.
>
>  'callback' is called when the command is received.  It is passed a
>  UnixctlConnection object, the list of arguments as unicode strings, and
> @@ -63,8 +70,30 @@ def command_register(name, usage, min_args, max_args, 
> callback, aux):
>  assert callable(callback)
>
>  if name not in commands:
> -commands[name] = _UnixctlCommand(usage, min_args, max_args, callback,
> - aux)
> +commands[name] = _UnixctlCommand(usage, min_args, max_args,
> + # FIXME: output_fmts,
> + callback, aux)
> +
> +
> +# FIXME: command_register() will be replaced with command_register_fmt() in a
> +#later patch of this series. It is is kept temporarily to reduce the
> +#amount of changes in this patch.
> +def command_register(name, usage, min_args, max_args, callback, aux):
> +""" Registers a command with the given 'name' to be exposed by the
> +UnixctlServer. 'usage' describes the arguments to the command; it is used
> +only for presentation to the user in "help" output.
> +
> +'callback' is called when the command is received.  It is passed a
> +UnixctlConnection object, the list of arguments as unicode strings, and
> +'aux'.  Normally 'callback' should reply by calling
> +UnixctlConnection.reply() or UnixctlConnection.reply_error() before it
> +returns, but if the command cannot be handled immediately, then it can
> +defer the reply until later.  A given connection can only process a 
> single
> +request at a time, so a reply must be made eventually to avoid blocking
> +that connection."""
> +
> +command_register_fmt(name, usage, 

Re: [ovs-dev] [PATCH v7 1/6] Add global option for JSON output to ovs-appctl.

[Eelco Chaudron]

On 18 Jan 2024, at 16:26, jm...@redhat.com wrote:

> From: Jakob Meng 
>
> For monitoring systems such as Prometheus it would be beneficial if
> OVS would expose statistics in a machine-readable format.
>
> This patch introduces support for different output formats to ovs-xxx
> tools. They gain a global option '-f,--format' which allows users to
> request JSON instead of plain-text for humans. An example call
> implemented in a later patch is 'ovs-appctl --format json dpif/show'.
>
> For that a new 'set-options' command has been added to lib/unixctl.c
> which allows to change the output format of the following commands.
> It is supposed to be run by ovs-xxx tools transparently for the user
> when a specific output format has been requested. For example, when a
> user calls 'ovs-appctl --format json dpif/show', then ovs-appctl will
> call 'set-options' to set the output format as requested by the user
> and then the actual command 'dpif/show'.
> This ovs-appctl behaviour has been implemented in a backward compatible
> way. One can use an updated client (ovs-appctl) with an old server
> (ovs-vswitchd) and vice versa. Of course, JSON output only works when
> both sides have been updated.
>
> To demonstrate the necessary API changes for supporting output formats,
> unixctl_command_register() in lib/unixctl.* has been cloned to
> unixctl_command_register_fmt(). The latter will be replaced with the
> former in a later patch. The new function gained an int argument called
> 'output_fmts' with which commands have to announce their support for
> output formats.
>
> Function type unixctl_cb_func in lib/unixctl.h has gained a new arg
> 'enum ovs_output_fmt fmt'. For now, it has been added as a comment
> only for the huge changes reason mentioned earlier. The output format
> which has been passed via 'set-options' to ovs-vswitchd will be
> converted into a value of type 'enum ovs_output_fmt' in lib/unixctl.c
> and then passed to said 'fmt' arg of the choosen command handler (in a
> future patch). When a requested output format is not supported by a
> command, then process_command() in lib/unixctl.c will return an error.
>
> This patch does not yet pass the choosen output format to commands.
> Doing so requires changes to all unixctl_command_register() calls and
> all command callbacks. To improve readibility those changes have been
> split out into a follow up patch. Respectively, whenever an output
> format other than 'text' is choosen for ovs-xxx tools, they will fail.
> By default, the output format is plain-text as before.
>
> In popular tools like kubectl the option for output control is usually
> called '-o|--output' instead of '-f,--format'. But ovs-appctl already
> has an short option '-o' which prints the available ovs-appctl options
> ('--option'). The now choosen name also better alines with ovsdb-client
> where '-f,--format' controls output formatting.
>
> Reported-at: https://bugzilla.redhat.com/1824861
> Signed-off-by: Jakob Meng 

Hi Jakob,


Thank you for submitting this series; I believe it's a valuable addition to 
OVS! Apologies for the delayed response. I've reviewed the entire series, and 
most of the comments are minor change requests. I'll hold off on sending a new 
revision until Ilya has had a chance to review it, as he provided some comments 
on the previous revision.

Cheers,

Eelco

> ---
>  NEWS   |   2 +
>  lib/command-line.c |  36 +++
>  lib/command-line.h |  10 +++
>  lib/dpctl.h|   4 ++
>  lib/unixctl.c  | 142 ++---
>  lib/unixctl.h  |  16 -
>  utilities/ovs-appctl.c |  97 
>  utilities/ovs-dpctl.c  |   1 +
>  8 files changed, 271 insertions(+), 37 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 2153b4805..8631ea45e 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -33,6 +33,8 @@ v3.3.0 - xx xxx 
> "ovs-appctl dpctl/ct-del-limits default".
>   * 'dpctl/flush-conntrack' is now capable of flushing connections based
> on mark and labels.
> + * Added new option [-f|--format] to choose the output format, e.g. 
> 'json'
> +   or 'text' (by default).
> - ovs-vsctl:
>   * New commands 'set-zone-limit', 'del-zone-limit' and 'list-zone-limits'
> to manage the maximum number of connections in conntrack zones via
> diff --git a/lib/command-line.c b/lib/command-line.c
> index 967f4f5d5..775e0430a 100644
> --- a/lib/command-line.c
> +++ b/lib/command-line.c
> @@ -24,6 +24,7 @@
>  #include "ovs-thread.h"
>  #include "util.h"
>  #include "openvswitch/vlog.h"
> +#include "openvswitch/json.h"
>
>  VLOG_DEFINE_THIS_MODULE(command_line);
>
> @@ -53,6 +54,41 @@ ovs_cmdl_long_options_to_short_options(const struct option 
> options[])
>  return xstrdup(short_options);
>  }
>
> +const char *
> +ovs_output_fmt_to_string(enum ovs_output_fmt fmt)
> +{
> +switch (fmt) {
> +case OVS_OUTPUT_FMT_TEXT:
> +return "text";
> +
> +cas

[ovs-dev] [PATCH v3] ofproto-dpif-upcall: Try lock for udpif_key map during sweep.

[LIU Yulong]

A potential race condition happened with the following 3 threads:
* PMD thread replaced the old_ukey and transitioned the state to
  UKEY_DELETED.
* RCU thread is freeing the old_ukey mutex.
* While the revalidator thread is trying to lock the old_ukey mutex.

We added some timestamp to udpif_key state transition and
the udpif_key mutex free action, as well as the sweep try lock for
the same udpif_key. When the ovs-vswitchd goes core, the udpif_key
try_lock mutex had always a bit later than the udpif_key mutex free
action. For instance [3]:
ukey_destroy_time = 13217289156490
sweep_now = 13217289156568

The second time is 78us behind the first time.

Then vswitchd process aborts at the revalidator thread try_lock of
ukey->mutex because of the NULL pointer.

This patch adds the try_lock for the ukeys' basket udpif_key map to
avoid the PMD and revalidator access to the same map for replacing the
udpif_key and transitioning the udpif_key state.

More details can be found at:
[1] https://mail.openvswitch.org/pipermail/ovs-discuss/2024-March/052973.html
[2] https://mail.openvswitch.org/pipermail/ovs-discuss/2024-February/052949.html
[3] https://mail.openvswitch.org/pipermail/ovs-discuss/2024-March/052993.html

Signed-off-by: LIU Yulong 
---
v2: - Updated commit message to make 0-day Robot happy.
v3: - Updated commit message to make checkpatch.py happy.
- Add some investigation details.
---
---
 ofproto/ofproto-dpif-upcall.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 9a5c5c29c..ef13f820a 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -2974,6 +2974,10 @@ revalidator_sweep__(struct revalidator *revalidator, 
bool purge)
 struct umap *umap = &udpif->ukeys[i];
 size_t n_ops = 0;
 
+if (ovs_mutex_trylock(&umap->mutex)) {
+continue;
+}
+
 CMAP_FOR_EACH(ukey, cmap_node, &umap->cmap) {
 enum ukey_state ukey_state;
 
@@ -3013,9 +3017,7 @@ revalidator_sweep__(struct revalidator *revalidator, bool 
purge)
 if (ukey_state == UKEY_EVICTED) {
 /* The common flow deletion case involves deletion of the flow
  * during the dump phase and ukey deletion here. */
-ovs_mutex_lock(&umap->mutex);
 ukey_delete(umap, ukey);
-ovs_mutex_unlock(&umap->mutex);
 }
 
 if (n_ops == REVALIDATE_MAX_BATCH) {
@@ -3025,6 +3027,7 @@ revalidator_sweep__(struct revalidator *revalidator, bool 
purge)
 n_ops = 0;
 }
 }
+ovs_mutex_unlock(&umap->mutex);
 
 if (n_ops) {
 push_ukey_ops(udpif, umap, ops, n_ops);
-- 
2.27.0

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

Re: [ovs-dev] [FYI][PATCH] tracing/treewide: Remove second parameter of __assign_str()

[Alison Schofield]

On Fri, Feb 23, 2024 at 12:56:34PM -0500, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" 
> 
> [
>This is a treewide change. I will likely re-create this patch again in
>the second week of the merge window of v6.9 and submit it then. Hoping
>to keep the conflicts that it will cause to a minimum.
> ]
> 
> With the rework of how the __string() handles dynamic strings where it
> saves off the source string in field in the helper structure[1], the
> assignment of that value to the trace event field is stored in the helper
> value and does not need to be passed in again.
> 
> This means that with:
> 
>   __string(field, mystring)
> 
> Which use to be assigned with __assign_str(field, mystring), no longer
> needs the second parameter and it is unused. With this, __assign_str()
> will now only get a single parameter.
> 
> There's over 700 users of __assign_str() and because coccinelle does not
> handle the TRACE_EVENT() macro I ended up using the following sed script:
> 
>   git grep -l __assign_str | while read a ; do
>   sed -e 's/\(__assign_str([^,]*[^ ,]\) *,[^;]*/\1)/' $a > /tmp/test-file;
>   mv /tmp/test-file $a;
>   done
> 
> I then searched for __assign_str() that did not end with ';' as those
> were multi line assignments that the sed script above would fail to catch.
> 
> Note, the same updates will need to be done for:
> 
>   __assign_str_len()
>   __assign_rel_str()
>   __assign_rel_str_len()
>   __assign_bitmask()
>   __assign_rel_bitmask()
>   __assign_cpumask()
>   __assign_rel_cpumask()
> 
> [1] 
> https://lore.kernel.org/linux-trace-kernel/2024011442.634192...@goodmis.org/
> 
> Signed-off-by: Steven Rostedt (Google) 
> ---
>  arch/arm64/kernel/trace-events-emulation.h|   2 +-
>  arch/powerpc/include/asm/trace.h  |   4 +-
>  arch/x86/kvm/trace.h  |   2 +-
>  drivers/base/regmap/trace.h   |  18 +--
>  drivers/base/trace.h  |   2 +-
>  drivers/block/rnbd/rnbd-srv-trace.h   |  12 +-
>  drivers/cxl/core/trace.h  |  24 ++--

snip to CXL


> diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h
> index bdf117a33744..07ba4e033347 100644
> --- a/drivers/cxl/core/trace.h
> +++ b/drivers/cxl/core/trace.h

snip to poison

> @@ -668,8 +668,8 @@ TRACE_EVENT(cxl_poison,
>   ),
>  
>   TP_fast_assign(
> - __assign_str(memdev, dev_name(&cxlmd->dev));
> - __assign_str(host, dev_name(cxlmd->dev.parent));
> + __assign_str(memdev);
> + __assign_str(host);

I think I get that the above changes work because the TP_STRUCT__entry for
these did:
__string(memdev, dev_name(&cxlmd->dev))
__string(host, dev_name(cxlmd->dev.parent))

>   __entry->serial = cxlmd->cxlds->serial;
>   __entry->overflow_ts = cxl_poison_overflow(flags, overflow_ts);
>   __entry->dpa = cxl_poison_record_dpa(record);
> @@ -678,12 +678,12 @@ TRACE_EVENT(cxl_poison,
>   __entry->trace_type = trace_type;
>   __entry->flags = flags;
>   if (region) {
> - __assign_str(region, dev_name(®ion->dev));
> + __assign_str(region);
>   memcpy(__entry->uuid, ®ion->params.uuid, 16);
>   __entry->hpa = cxl_trace_hpa(region, cxlmd,
>__entry->dpa);
>   } else {
> - __assign_str(region, "");
> + __assign_str(region);
>   memset(__entry->uuid, 0, 16);
>   __entry->hpa = ULLONG_MAX;

For the above 2, there was no helper in TP_STRUCT__entry. A recently
posted patch is fixing that up to be __string(region, NULL) See [1],
with the actual assignment still happening in TP_fast_assign.

Does that assign logic need to move to the TP_STRUCT__entry definition
when you merge these changes? I'm not clear how much logic is able to be
included, ie like 'C' style code in the TP_STRUCT__entry.

[1]
https://lore.kernel.org/linux-cxl/20240314044301.2108650-1-alison.schofi...@intel.com/

Thanks for helping,
Alison


>   }



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

Re: [ovs-dev] [PATCH] ofproto-dpif-upcall: Try lock for umap iteration during sweep.

[Eelco Chaudron]

Hi,

Thanks for submitting a patch and researching the problem. I haven't had the 
chance to review your patch yet, as I still need to dedicate some time to 
review it, based on the complexity. However, if you've sent a new version in 
the meantime, please make sure to label it with a version number. For 
reference, here's an example of someone marking their patch as "v3" in the 
subject line, with additional context provided in the commit messages following 
the "---".

https://mail.openvswitch.org/pipermail/ovs-dev/2024-March/412232.html

Talking about the subject line, your patch would probably receive a message 
from the robot about your Subject line. It's advisable to run the checkpatch 
tool before sending out the patch, as it will likely identify similar issues. 
Below is an example of running the check on the last commit in your git 
repository:

./utilities/checkpatch.py -S -1

Lastly, you may consider revising your commit message to ensure clarity on why 
the issue is problematic (refer to the comment below).

Awaiting your v3 eagerly.

Thanks,

Eelco


On 15 Mar 2024, at 8:20, LIU Yulong wrote:

> A potential race condition happened with the following 3 threads:
> * PMD thread replaced the old_ukey and transitioned the state to
>   UKEY_DELETED.
> * RCU thread is freeing the old_ukey mutex.
> * While the revalidator thread is trying to lock the old_ukey mutex.

Can you explain why this is a problem in more details? The RCU thread should 
only free the structure if we go through quiescent state. The sweep is using a 
CMAP which should be fine. We also hold the ukey->mutex mutex, so the state 
should not change underneath us. Also taking the umap->mutex lock for the 
entire sweep phase was not the design goal of using a CMAP.

I hope that if I go over the actual code it will be clear. But the idea of the 
commit message is to tell the full story, so we can verify the code behaves as 
described.

> Then vswitchd process aborts at the revalidator thread try_lock of
> ukey->mutex because of the NULL pointer.
>
> This patch adds the try_lock for the ukeys' basket umap to avoid
> the PMD and revalidator access to the same umap for replacing the
> ukey and transitioning the ukey state.
>
> More details can be found at:
> [1] https://mail.openvswitch.org/pipermail/ovs-discuss/2024-March/052973.html
> [2] 
> https://mail.openvswitch.org/pipermail/ovs-discuss/2024-February/052949.html
>
> Signed-off-by: LIU Yulong 
> ---
>  ofproto/ofproto-dpif-upcall.c | 7 +--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 9a5c5c29c..ef13f820a 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -2974,6 +2974,10 @@ revalidator_sweep__(struct revalidator *revalidator, 
> bool purge)
>  struct umap *umap = &udpif->ukeys[i];
>  size_t n_ops = 0;
>
> +if (ovs_mutex_trylock(&umap->mutex)) {
> +continue;
> +}
> +
>  CMAP_FOR_EACH(ukey, cmap_node, &umap->cmap) {
>  enum ukey_state ukey_state;
>
> @@ -3013,9 +3017,7 @@ revalidator_sweep__(struct revalidator *revalidator, 
> bool purge)
>  if (ukey_state == UKEY_EVICTED) {
>  /* The common flow deletion case involves deletion of the 
> flow
>   * during the dump phase and ukey deletion here. */
> -ovs_mutex_lock(&umap->mutex);
>  ukey_delete(umap, ukey);
> -ovs_mutex_unlock(&umap->mutex);
>  }
>
>  if (n_ops == REVALIDATE_MAX_BATCH) {
> @@ -3025,6 +3027,7 @@ revalidator_sweep__(struct revalidator *revalidator, 
> bool purge)
>  n_ops = 0;
>  }
>  }
> +ovs_mutex_unlock(&umap->mutex);
>  if (n_ops) {
>  push_ukey_ops(udpif, umap, ops, n_ops);
> -- 
> 2.27.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] ofproto-dpif-upcall: Try lock for umap iteration during sweep.

[LIU Yulong]

A potential race condition happened with the following 3 threads:
* PMD thread replaced the old_ukey and transitioned the state to
  UKEY_DELETED.
* RCU thread is freeing the old_ukey mutex.
* While the revalidator thread is trying to lock the old_ukey mutex.
Then vswitchd process aborts at the revalidator thread try_lock of
ukey->mutex because of the NULL pointer.

This patch adds the try_lock for the ukeys' basket umap to avoid
the PMD and revalidator access to the same umap for replacing the
ukey and transitioning the ukey state.

More details can be found at:
[1] https://mail.openvswitch.org/pipermail/ovs-discuss/2024-March/052973.html
[2] https://mail.openvswitch.org/pipermail/ovs-discuss/2024-February/052949.html

Signed-off-by: LIU Yulong 
---
 ofproto/ofproto-dpif-upcall.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 9a5c5c29c..ef13f820a 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -2974,6 +2974,10 @@ revalidator_sweep__(struct revalidator *revalidator, 
bool purge)
 struct umap *umap = &udpif->ukeys[i];
 size_t n_ops = 0;
 
+if (ovs_mutex_trylock(&umap->mutex)) {
+continue;
+}
+
 CMAP_FOR_EACH(ukey, cmap_node, &umap->cmap) {
 enum ukey_state ukey_state;
 
@@ -3013,9 +3017,7 @@ revalidator_sweep__(struct revalidator *revalidator, bool 
purge)
 if (ukey_state == UKEY_EVICTED) {
 /* The common flow deletion case involves deletion of the flow
  * during the dump phase and ukey deletion here. */
-ovs_mutex_lock(&umap->mutex);
 ukey_delete(umap, ukey);
-ovs_mutex_unlock(&umap->mutex);
 }
 
 if (n_ops == REVALIDATE_MAX_BATCH) {
@@ -3025,6 +3027,7 @@ revalidator_sweep__(struct revalidator *revalidator, bool 
purge)
 n_ops = 0;
 }
 }
+ovs_mutex_unlock(&umap->mutex);
 
 if (n_ops) {
 push_ukey_ops(udpif, umap, ops, n_ops);
-- 
2.27.0

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