[ovs-dev] [PATCH v3 ovn] controller: split mg action in table 39 and 40 to fit kernel netlink buffer size
Introduce the capability to split multicast group openflow actions
created in consider_mc_group routine in multiple buffers if the
single buffer size is over netlink buffer size limits.
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2232152
Signed-off-by: Lorenzo Bianconi
---
Changes since v2:
- code refactoring
Changes since v1:
- set MC_OFPACTS_MAX_MSG_SIZE to 8K
- add dedicated unit-test in ovn.at
- add entry in NEWS
- improve comments
- cosmetics
---
NEWS| 1 +
controller/physical.c | 226 ++--
controller/pinctrl.c| 66
include/ovn/actions.h | 3 +
tests/ovn-controller.at | 45
tests/ovn.at| 57 ++
6 files changed, 320 insertions(+), 78 deletions(-)
diff --git a/NEWS b/NEWS
index 425dfe0a8..199fc8aeb 100644
--- a/NEWS
+++ b/NEWS
@@ -1,5 +1,6 @@
Post v23.09.0
-
+ - introduce the capability to support arbitrarily large multicast groups
OVN v23.09.0 - 15 Sep 2023
--
diff --git a/controller/physical.c b/controller/physical.c
index 72e88a203..7e6dd8a76 100644
--- a/controller/physical.c
+++ b/controller/physical.c
@@ -1969,6 +1969,57 @@ local_output_pb(int64_t tunnel_key, struct ofpbuf
*ofpacts)
put_resubmit(OFTABLE_CHECK_LOOPBACK, ofpacts);
}
+#define MC_OFPACTS_MAX_MSG_SIZE 8192
+#define MC_BUF_START_ID 0x9000
+
+static void
+mc_ofctrl_add_flow(const struct sbrec_multicast_group *mc,
+ struct match *match, struct ofpbuf *ofpacts,
+ struct ofpbuf *ofpacts_last, uint8_t stage,
+ size_t index, uint32_t *pflow_index,
+ uint16_t prio, struct ovn_desired_flow_table *flow_table)
+
+{
+/* do not overcome max netlink message size used by ovs-vswitchd to
+ * send netlink configuration to the kernel. */
+if (ofpacts->size < MC_OFPACTS_MAX_MSG_SIZE && index < mc->n_ports - 1) {
+return;
+}
+
+uint32_t flow_index = *pflow_index;
+bool is_first = flow_index == MC_BUF_START_ID;
+if (!is_first) {
+match_set_reg(match, MFF_REG6 - MFF_REG0, flow_index);
+prio += 10;
+}
+
+if (index == mc->n_ports - 1) {
+ofpbuf_put(ofpacts, ofpacts_last->data, ofpacts_last->size);
+} else {
+/* Split multicast groups with size greater than
+ * MC_OFPACTS_MAX_MSG_SIZE in order to not overcome the
+ * MAX_ACTIONS_BUFSIZE netlink buffer size supported by the kernel.
+ * In order to avoid all the action buffers to be squashed together by
+ * ovs, add a controller action for each configured openflow.
+ */
+size_t oc_offset = encode_start_controller_op(
+ACTION_OPCODE_MG_SPLIT_BUF, false, NX_CTLR_NO_METER, ofpacts);
+ovs_be32 val = htonl(++flow_index);
+ofpbuf_put(ofpacts, , sizeof val);
+val = htonl(mc->tunnel_key);
+ofpbuf_put(ofpacts, , sizeof val);
+ofpbuf_put(ofpacts, , sizeof stage);
+encode_finish_controller_op(oc_offset, ofpacts);
+}
+
+ofctrl_add_flow(flow_table, stage, prio, mc->header_.uuid.parts[0],
+match, ofpacts, >header_.uuid);
+ofpbuf_clear(ofpacts);
+/* reset MFF_REG6. */
+put_load(0, MFF_REG6, 0, 32, ofpacts);
+*pflow_index = flow_index;
+}
+
static void
consider_mc_group(struct ovsdb_idl_index *sbrec_port_binding_by_name,
enum mf_field_id mff_ovn_geneve,
@@ -1990,9 +2041,6 @@ consider_mc_group(struct ovsdb_idl_index
*sbrec_port_binding_by_name,
struct sset remote_chassis = SSET_INITIALIZER(_chassis);
struct sset vtep_chassis = SSET_INITIALIZER(_chassis);
-struct match match;
-match_outport_dp_and_port_keys(, dp_key, mc->tunnel_key);
-
/* Go through all of the ports in the multicast group:
*
*- For remote ports, add the chassis to 'remote_chassis' or
@@ -2014,9 +2062,20 @@ consider_mc_group(struct ovsdb_idl_index
*sbrec_port_binding_by_name,
* the redirect port was added.
*/
struct ofpbuf ofpacts, remote_ofpacts, remote_ofpacts_ramp;
+struct ofpbuf ofpacts_last, ofpacts_ramp_last;
ofpbuf_init(, 0);
ofpbuf_init(_ofpacts, 0);
ofpbuf_init(_ofpacts_ramp, 0);
+ofpbuf_init(_last, 0);
+ofpbuf_init(_ramp_last, 0);
+
+bool local_ports = false, remote_ports = false, remote_ramp_ports = false;
+
+/* local port loop. */
+uint32_t flow_index = MC_BUF_START_ID;
+put_load(0, MFF_REG6, 0, 32, );
+put_load(mc->tunnel_key, MFF_LOG_OUTPORT, 0, 32, _last);
+
for (size_t i = 0; i < mc->n_ports; i++) {
struct sbrec_port_binding *port = mc->ports[i];
@@ -2040,19 +2099,15 @@ consider_mc_group(struct ovsdb_idl_index
*sbrec_port_binding_by_name,
if (ldp->is_transit_switch) {
local_output_pb(port->tunnel_key, );
} else {
-
Re: [ovs-dev] [PATCH net v2 0/4] selftests: openvswitch: Minor fixes for some systems
On Wed, Oct 11, 2023 at 03:49:35PM -0400, Aaron Conole wrote: > A number of corner cases were caught when trying to run the selftests on > older systems. Missed skip conditions, some error cases, and outdated > python setups would all report failures but the issue would actually be > related to some other condition rather than the selftest suite. > > Address these individual cases. > > Aaron Conole (4): > selftests: openvswitch: Add version check for pyroute2 > selftests: openvswitch: Catch cases where the tests are killed > selftests: openvswitch: Skip drop testing on older kernels > selftests: openvswitch: Fix the ct_tuple for v4 > > .../selftests/net/openvswitch/openvswitch.sh | 21 +++- > .../selftests/net/openvswitch/ovs-dpctl.py| 48 ++- > 2 files changed, 66 insertions(+), 3 deletions(-) Thanks Aaron, this looks like a good incremental improvement to me. For series, Reviewed-by: Simon Horman ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 branch-2.17 1/2] conntrack: simplify cleanup path
Simon Horman writes: > On Thu, Oct 12, 2023 at 09:45:05AM +0200, Frode Nordahl wrote: >> On Tue, Oct 3, 2023 at 9:06 PM Aaron Conole wrote: >> > >> > The conntrack cleanup and allocation code is spread across multiple >> > list invocations. This was changed in mainline code when the timeout >> > expiration lists were refactored, but backporting those fixes would >> > be a rather large effort. Instead, take only the changes we need >> > to backport "contrack: Remove nat_conn introducing key directionality" >> > into branch-2.17. >> >> Thanks alot for your help in backporting this patch. >> >> We have a managed customer environment where circumstances make the >> issue trigger with a rate of 70% when performing a certain action. Up >> until now they have been running with a temporary package containing >> the patches from >> https://patchwork.ozlabs.org/project/openvswitch/list/?series=351579=* >> >> To test this series, they have first re-confirmed that they see the >> issue with a packaged version of OVS 2.17.7, and then switched to a >> packaged version of OVS 2.17.7 with these patches and confirmed that >> the issue is no longer occurring. The same package has been in >> production use for the past week, being exposed to real world traffic. >> No side effects or incidents to report. >> >> Tested-by: Frode Nordahl > > Thanks Frode, > > I think we can go ahead and apply this series to branch-2.17. > > Acked-by: Simon Horman Thanks Simon, Frode, and Paolo - I've merged this series to branch-2.17. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v6 1/3] netdev-dpdk: Sync and clean {get, set}_config() callbacks.
, Oct 13, 2023 at 11:07:
From: Jakob Meng
For better usability, the function pairs get_config() and
set_config() for netdevs should be symmetric: Options which are
accepted by set_config() should be returned by get_config() and the
latter should output valid options for set_config() only.
This patch moves key-value pairs which are no valid options from
get_config() to the get_status() callback. For example, get_config()
in lib/netdev-dpdk.c returned {configured,requested}_{rx,tx}_queues
previously. For requested rx queues the proper option name is n_rxq,
so requested_rx_queues has been renamed respectively. Tx queues
cannot be changed by the user, hence requested_tx_queues has been
dropped. Both configured_{rx,tx}_queues will be returned as
n_{r,t}xq in the get_status() callback.
The documentation in vswitchd/vswitch.xml for status columns as well
as tests have been updated accordingly.
Reported-at: https://bugzilla.redhat.com/1949855
Signed-off-by: Jakob Meng
Hi Jakob, this looks good, thanks!
Reviewed-by: Robin Jarry
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v6 2/2] ofproto-dpif-mirror: Add support for pre-selection filter.
On 10/8/23 06:21, Mike Pattrick wrote:
> Currently a bridge mirror will collect all packets and tools like
> ovs-tcpdump can apply additional filters after they have already been
> duplicated by vswitchd. This can result in inefficient collection.
>
> This patch adds support to apply pre-selection to bridge mirrors, which
> can limit which packets are mirrored based on flow metadata. This
> significantly improves overall vswitchd performance during mirroring if
> only a subset of traffic is required.
>
> I benchmarked this with two setups. A netlink based test with two veth
> pairs connected to a single bridge, and a netdev based test involving a
> mix of DPDK nics, and netdev-linux interfaces. Both tests involved
> saturating the link with iperf3 and then sending an icmp ping every
> second. I then measured the throughput on the link with no mirroring,
> icmp pre-selected mirroring, and full mirroring. The results, below,
> indicate a significant reduction to impact of mirroring when only a
> subset of the traffic on a port is selected for collection.
>
> Test No Mirror | No Filter | Filter | No Filter | Filter |
> +---+---+---+---+--+
> netlink | 39.0 Gbps | 36.1 Gbps | 38.2 Gbps | 7%|2%|
> netdev | 7.39 Gbps | 4.95 Gbps | 6.24 Gbps |33%| 15%|
>
> The ratios above are the percent reduction in total throughput when
> mirroring is used either with or without a filter.
>
> Signed-off-by: Mike Pattrick
>
Hi, Mike. Thanks for the patch!
Not a full review, but see some comments inline.
Best regards, Ilya Maximets.
> ---
> v3:
> - Added more tests
> - Refactored mirror wildcard modification out of mirror_get
> - Improved error handling
> v4:
> - Refactored mirror_set and mirror_get functions to reduce function
> parameters
> v5:
> - Fixed formating issues
> - Moved some code from patch 2 to patch 1 to fix compile
> v6:
> - Clarified filter format in documentation
> - Moved flow/mask operations to atomic set/get
> - Cleaned up and improved test
>
> ---
> Documentation/ref/ovs-tcpdump.8.rst | 8 +-
> NEWS| 4 +
> lib/flow.h | 9 +++
> ofproto/ofproto-dpif-mirror.c | 68 +++-
> ofproto/ofproto-dpif-mirror.h | 8 +-
> ofproto/ofproto-dpif-xlate.c| 16 +++-
> ofproto/ofproto-dpif.c | 9 +--
> ofproto/ofproto-dpif.h | 6 ++
> ofproto/ofproto.h | 3 +
> tests/ofproto-dpif.at | 116
> utilities/ovs-tcpdump.in| 13 +++-
> vswitchd/bridge.c | 13 +++-
> vswitchd/vswitch.ovsschema | 5 +-
> vswitchd/vswitch.xml| 13
> 14 files changed, 273 insertions(+), 18 deletions(-)
>
> diff --git a/Documentation/ref/ovs-tcpdump.8.rst
> b/Documentation/ref/ovs-tcpdump.8.rst
> index b9f8cdf6f..e21e61211 100644
> --- a/Documentation/ref/ovs-tcpdump.8.rst
> +++ b/Documentation/ref/ovs-tcpdump.8.rst
> @@ -61,8 +61,14 @@ Options
>
>If specified, mirror all ports (optional).
>
> +* ``--filter ``
> +
> + If specified, only mirror flows that match the provided OpenFlow filter.
> + The available fields are documented in ``ovs-fields(7)``.
> +
> See Also
>
>
> ``ovs-appctl(8)``, ``ovs-vswitchd(8)``, ``ovs-pcap(1)``,
> -``ovs-tcpundump(1)``, ``tcpdump(8)``, ``wireshark(8)``.
> +``ovs-fields(7)``, ``ovs-tcpundump(1)``, ``tcpdump(8)``,
> +``wireshark(8)``.
> diff --git a/NEWS b/NEWS
> index 6b45492f1..e95033b50 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -6,6 +6,10 @@ Post-v3.2.0
> from older version is supported but it may trigger more leader
> elections
> during the process, and error logs complaining unrecognized fields may
> be observed on old nodes.
> + * Added a new filter column in the Mirror table which can be used to
> + apply filters to mirror ports.
> + - ovs-tcpdump:
> + * Added new --filter parameter to apply pre-selection on mirrored flows.
>
>
> v3.2.0 - 17 Aug 2023
> diff --git a/lib/flow.h b/lib/flow.h
> index a9d026e1c..f77d3553a 100644
> --- a/lib/flow.h
> +++ b/lib/flow.h
> @@ -939,6 +939,15 @@ flow_union_with_miniflow(struct flow *dst, const struct
> miniflow *src)
> flow_union_with_miniflow_subset(dst, src, src->map);
> }
>
> +/* Perform a bitwise OR of minimask 'src' mask data with the equivalent
> + * fields in 'dst', storing the result in 'dst'. */
> +static inline void
> +flow_wildcards_union_with_minimask(struct flow_wildcards *dst,
> + const struct minimask *src)
> +{
> +flow_union_with_miniflow_subset(>masks, >masks,
> src->masks.map);
> +}
> +
> static inline bool is_ct_valid(const struct flow *flow,
> const struct flow_wildcards *mask,
> struct flow_wildcards *wc)
> diff --git
Re: [ovs-dev] [PATCH] ofproto-dpif-upcall: Pause revalidators when purging.
Hello Simon,
On Tue, Oct 10, 2023 at 5:10 PM Simon Horman wrote:
>
> On Mon, Oct 09, 2023 at 05:06:51PM +0200, David Marchand wrote:
> > A main thread executing the 'revalidator/purge' command could race with
> > revalidator threads that can be dumping/sweeping the purged flows at the
> > same time.
> >
> > This race can be reproduced (with dpif debug logs) by running the
> > conntrack - ICMP related unit tests with the userspace datapath:
> >
> > 2023-10-09T14:11:55.242Z|00177|unixctl|DBG|received request
> > revalidator/purge[], id=0
> > 2023-10-09T14:11:55.242Z|00044|dpif(revalidator17)|DBG|netdev@ovs-netdev:
> > flow_dump ufid:68ff6817-fb3b-4b30-8412-9cf175318294 ,
> > packets:0, bytes:0, used:never
> > 2023-10-09T14:11:55.242Z|00178|dpif|DBG|netdev@ovs-netdev: flow_del
> > ufid:07046e91-30a6-4862-9048-1a76b5a88a5b
> > recirc_id(0),dp_hash(0),skb_priority(0),in_port(2),skb_mark(0),
> > ct_state(0),ct_zone(0),ct_mark(0),ct_label(0),
> > packet_type(ns=0,id=0),
> > eth(src=a6:0a:bf:e2:f3:f2,dst=62:23:0f:f6:2c:75),
> > eth_type(0x0800),ipv4(src=10.1.1.1,dst=10.1.1.2,proto=17,tos=0,
> > ttl=64,frag=no),udp(src=37380,dst=1), packets:0, bytes:0,
> > used:never
> > ...
> > 2023-10-09T14:11:55.242Z|00049|dpif(revalidator17)|WARN|netdev@ovs-netdev:
> > failed to flow_get (No such file or directory)
> > ufid:07046e91-30a6-4862-9048-1a76b5a88a5b , packets:0,
> > bytes:0, used:never
> > 2023-10-09T14:11:55.242Z|00050|ofproto_dpif_upcall(revalidator17)|WARN|
> > Failed to acquire udpif_key corresponding to unexpected flow
> > (No such file or directory):
> > ufid:07046e91-30a6-4862-9048-1a76b5a88a5b
> > ...
> > 2023-10-09T14:11:55.242Z|00183|unixctl|DBG|replying with success, id=0: ""
> >
> > To avoid this race, a first part of the fix is to pause (if not already
> > paused) the revalidators while the main thread is purging the datapath
> > flows.
> >
> > Then a second issue is observed by running the same unit test with the
> > kernel datapath. Its dpif implementation dumps flows via a netlink request
> > (see dpif_flow_dump_create(), dpif_netlink_flow_dump_create(),
> > nl_dump_start(), nl_sock_send__()) in the leader revalidator thread,
> > before pausing revalidators:
> >
> > 2023-10-09T14:44:28.742Z|00122|unixctl|DBG|received request
> > revalidator/purge[], id=0
> > ...
> > 2023-10-09T14:44:28.742Z|00125|dpif|DBG|system@ovs-system: flow_del
> > ufid:70102d81-30a1-44b9-aa76-3d02a9ffd2c9 recirc_id(0),dp_hash(0),
> > skb_priority(0),in_port(2),skb_mark(0),ct_state(0),ct_zone(0),
> > ct_mark(0),ct_label(0),eth(src=a6:0a:bf:e2:f3:f2,
> > dst=ff:ff:ff:ff:ff:ff),eth_type(0x0806),arp(sip=10.1.1.1,
> > tip=10.1.1.2,op=1,sha=a6:0a:bf:e2:f3:f2,tha=00:00:00:00:00:00),
> > packets:0, bytes:0, used:never
> > ...
> > 2023-10-09T14:44:28.742Z|00129|unixctl|DBG|replying with success, id=0: ""
> > ...
> > 2023-10-09T14:44:28.742Z|6|dpif(revalidator21)|DBG|system@ovs-system:
> > flow_dump ufid:70102d81-30a1-44b9-aa76-3d02a9ffd2c9 ,
> > packets:0, bytes:0, used:never
> > ...
> > 2023-10-09T14:44:28.742Z|00012|dpif(revalidator21)|WARN|system@ovs-system:
> > failed to flow_get (No such file or directory)
> > ufid:70102d81-30a1-44b9-aa76-3d02a9ffd2c9 , packets:0,
> > bytes:0, used:never
> > 2023-10-09T14:44:28.742Z|00013|ofproto_dpif_upcall(revalidator21)|WARN|
> > Failed to acquire udpif_key corresponding to unexpected flow
> > (No such file or directory):
> > ufid:70102d81-30a1-44b9-aa76-3d02a9ffd2c9
> >
> > To avoid evaluating already deleted flows, the second part of the fix is
> > to ensure that dumping from the leader revalidator thread is done out of
> > any pause request.
> >
> > Fixes: 98bb4286970d ("tests: Add command to purge revalidators of flows.")
> > Signed-off-by: David Marchand
> > ---
>
> Hi David,
>
> I'm not having much luck reproducing the above.
> Any guidance would be much appreciated.
Sorry, my fault.
I wrote and tested this series against my system-dpdk series (patch 7
pulls the system-traffic.at tests in check-dpdk).
https://patchwork.ozlabs.org/project/openvswitch/list/?series=371864=%2A=both
I can't reproduce the issue with check-system-userspace on the current master.
But I do reproduce a failure on this test with check-kernel (rarely)
on the current master, on my fedora 37 system.
If I enable dpif:dbg logs in the test, it is more frequent:
$ git diff
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 418cd32fec..0844ada0fd 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -3654,6 +3654,7 @@ AT_SETUP([conntrack - ICMP related])
AT_SKIP_IF([test $HAVE_NC = no])
CHECK_CONNTRACK()
OVS_TRAFFIC_VSWITCHD_START()
+AT_CHECK([ovs-appctl vlog/set dpif:dbg])
ADD_NAMESPACES(at_ns0, at_ns1)
# while true; do make -C build check-kernel TESTSUITEFLAGS="-d 78" ||
break;
Re: [ovs-dev] [PATCH v6 1/2] ofproto-dpif-mirror: Reduce number of function parameters.
On 10/8/23 06:21, Mike Pattrick wrote:
> Previously the mirror_set() and mirror_get() functions took a large
> number of parameters, which was inefficient and difficult to read and
> extend. This patch moves most of the parameters into a struct.
>
> Signed-off-by: Mike Pattrick
> Acked-by: Simon Horman
> Acked-by: Eelco Chaudron
>
Hi, Mike. Thanks for the patch!
See some comments inline.
Best regards, Ilya Maximets.
> ---
>
> v6:
> - Changed a comment to be more grammatically correct
> ---
> ofproto/ofproto-dpif-mirror.c | 61 ++-
> ofproto/ofproto-dpif-mirror.h | 31 +-
> ofproto/ofproto-dpif-xlate.c | 25 ++
> ofproto/ofproto-dpif.c| 6 ++--
> 4 files changed, 67 insertions(+), 56 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-mirror.c b/ofproto/ofproto-dpif-mirror.c
> index 343b75f0e..17ba6f799 100644
> --- a/ofproto/ofproto-dpif-mirror.c
> +++ b/ofproto/ofproto-dpif-mirror.c
> @@ -207,19 +207,23 @@ mirror_bundle_dst(struct mbridge *mbridge, struct
> ofbundle *ofbundle)
> }
>
> int
> -mirror_set(struct mbridge *mbridge, void *aux, const char *name,
> - struct ofbundle **srcs, size_t n_srcs,
> - struct ofbundle **dsts, size_t n_dsts,
> - unsigned long *src_vlans, struct ofbundle *out_bundle,
> - uint16_t snaplen,
> - uint16_t out_vlan)
> +mirror_set(struct mbridge *mbridge, void *aux,
> + const struct ofproto_mirror_settings *ms, struct ofbundle **srcs,
> + struct ofbundle **dsts, struct ofbundle *bundle)
> +
> {
> struct mbundle *mbundle, *out;
> mirror_mask_t mirror_bit;
> struct mirror *mirror;
> struct hmapx srcs_map; /* Contains "struct ofbundle *"s. */
> struct hmapx dsts_map; /* Contains "struct ofbundle *"s. */
> +uint16_t out_vlan;
> +
> +if (!ms || !mbridge) {
> +return EINVAL;
> +}
>
> +out_vlan = ms->out_vlan;
> mirror = mirror_lookup(mbridge, aux);
> if (!mirror) {
> int idx;
> @@ -227,7 +231,7 @@ mirror_set(struct mbridge *mbridge, void *aux, const char
> *name,
> idx = mirror_scan(mbridge);
> if (idx < 0) {
> VLOG_WARN("maximum of %d port mirrors reached, cannot create %s",
> - MAX_MIRRORS, name);
> + MAX_MIRRORS, ms->name);
> return EFBIG;
> }
>
> @@ -242,8 +246,8 @@ mirror_set(struct mbridge *mbridge, void *aux, const char
> *name,
> unsigned long *vlans = ovsrcu_get(unsigned long *, >vlans);
>
> /* Get the new configuration. */
> -if (out_bundle) {
> -out = mbundle_lookup(mbridge, out_bundle);
> +if (bundle) {
> +out = mbundle_lookup(mbridge, bundle);
> if (!out) {
> mirror_destroy(mbridge, mirror->aux);
> return EINVAL;
> @@ -252,16 +256,16 @@ mirror_set(struct mbridge *mbridge, void *aux, const
> char *name,
> } else {
> out = NULL;
> }
> -mbundle_lookup_multiple(mbridge, srcs, n_srcs, _map);
> -mbundle_lookup_multiple(mbridge, dsts, n_dsts, _map);
> +mbundle_lookup_multiple(mbridge, srcs, ms->n_srcs, _map);
> +mbundle_lookup_multiple(mbridge, dsts, ms->n_dsts, _map);
This doesn't look right. ms->n_srcs is the number of entries
in ms->srcs. mirror_set__() function knows that the sizes of these
arrays are the same, but we do not know that here in this function.
Looks very prone to errors.
>
> /* If the configuration has not changed, do nothing. */
> if (hmapx_equals(_map, >srcs)
> && hmapx_equals(_map, >dsts)
> -&& vlan_bitmap_equal(vlans, src_vlans)
> +&& vlan_bitmap_equal(vlans, ms->src_vlans)
> && mirror->out == out
> && mirror->out_vlan == out_vlan
> -&& mirror->snaplen == snaplen)
> +&& mirror->snaplen == ms->snaplen)
> {
> hmapx_destroy(_map);
> hmapx_destroy(_map);
> @@ -275,15 +279,15 @@ mirror_set(struct mbridge *mbridge, void *aux, const
> char *name,
> hmapx_swap(_map, >dsts);
> hmapx_destroy(_map);
>
> -if (vlans || src_vlans) {
> +if (vlans || ms->src_vlans) {
> ovsrcu_postpone(free, vlans);
> -vlans = vlan_bitmap_clone(src_vlans);
> +vlans = vlan_bitmap_clone(ms->src_vlans);
> ovsrcu_set(>vlans, vlans);
> }
>
> mirror->out = out;
> mirror->out_vlan = out_vlan;
> -mirror->snaplen = snaplen;
> +mirror->snaplen = ms->snaplen;
>
> /* Update mbundles. */
> mirror_bit = MIRROR_MASK_C(1) << mirror->idx;
> @@ -406,23 +410,22 @@ mirror_update_stats(struct mbridge *mbridge,
> mirror_mask_t mirrors,
> /* Retrieves the mirror numbered 'index' in 'mbridge'. Returns true if such
> a
> * mirror exists, false otherwise.
> *
> - * If successful, '*vlans' receives the mirror's VLAN membership information,
> + * If successful 'mc->vlans' receives the
Re: [ovs-dev] [PATCH v3 0/3] Fix redundant mirror with tunnel options.
On 9 Oct 2023, at 14:05, Roi Dayan wrote: > Hi, > > The first commit removing the redundant mirror when using tunnel options. > The second is a test to verify it stays like this and doesn't break again. > The third commit is just updating prefixes of tests to match the file their > in. > > Thanks, > Roi I’ve applied the series to master. Once again, thanks for the patch. Cheers, Eelco ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v5] tests: Add some tests for byteq module.
On 13 Oct 2023, at 8:49, Eelco Chaudron wrote: > On 7 Oct 2023, at 10:37, James Raphael Tiovalen wrote: > >> This commit adds a non-exhaustive list of tests for some of the >> functions declared in `lib/byteq`. >> >> These unit tests have been executed via `make check` and they >> successfully passed. >> >> Acked-by: Simon Horman >> Signed-off-by: James Raphael Tiovalen > > Hi James, > > Thanks for working trough all the revisions and fixing my comments! I think > this patch looks good now. > > Acked-by: Eelco Chaudron I’ve applied the patch to master. Once again thanks for the patch. Cheers, Eelco ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v6 1/2] netdev-tc-offload: Add csum offload of IGMP/UDPLITE/SCTP in ip rewrite.
On 13 Oct 2023, at 9:27, Eelco Chaudron wrote: > On 7 Oct 2023, at 5:49, Faicker Mo wrote: > >> When the IP header is modified, for example, by NAT or a ToS/TTL change, >> the IP header checksum needs recalculation. In addition to the IP header >> checksum, for UDPLITE, its checksum also needs recalculation when any >> of the addresses change. >> >> This patch adds support for TC offloading of IGMP, UDPLITE, and SCTP >> packets by adding the correct csum action. >> >> Signed-off-by: Faicker Mo > > Thanks for making the changes! The patch looks good to me now! > > Acked-by: Eelco Chaudron I’ve committed the series to master. Once again thanks for submitting it! Cheers, Eelco ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 1/2] vswitch.xml: Add dpdkvhostuser group status.
On 13/10/2023 10:45, Ilya Maximets wrote:
On 10/12/23 14:15, Kevin Traynor wrote:
Add group for dpdkvhostuser(/client) netdev.
Adding as a single group as they display the same status,
one of which is 'mode' to indicate if it's client or server.
Fixes: b2e8b12f8a82 ("netdev-dpdk: add vhost-user get_status.")
Signed-off-by: Kevin Traynor
---
vswitchd/vswitch.xml | 28
1 file changed, 28 insertions(+)
diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
index 1e2a1267d..403b954c2 100644
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -3822,4 +3822,32 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch
options:peer=p1 \
+
+
+
+ dpdkvhostuser and dpdkvhostuserclient
+ netdev specific interface status options.
The 'options' might be not a good word here as these are not options,
i.e. users can't choose or set them. I see that 'dpdk' group uses
the same word, but we might need to change that as well. Some other
places use 'status information', for example.
I agree 'status information' sounds better. I'll update here and check
with Jaokb for dpdk as I don't want to cause unnecessary conflicts with
his patches.
+
+
+Client or server vhost mode.
+
+
+vhost features bitmap.
+
+
+Number of vrings.
+
+
+Vhost numa association.
+
+
+Vhost socket path.
+
+
+Status of connection.
+
+
+Size of vring n.
This is a very confusing description. Size of 'n'? :)
Most of other descriptions are also not very descriptive. I'm not
sure what is the value of adding these. I'd say we should either
document them better, so the average user can understand what they
mean, or just not describe at all.
What do you think?
Sure, I can expand them to be a bit more descriptive.
vring size is reported for a variable number of vrings, so there's
always going to be some correlation between the key and actual output
needed in that case, and I think with the output beside it, it makes
more sense. I can add some lines to state about there being variable of
vrings and what n is etc. to make it more clear standalone.
Thanks!
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2/2] Use hexify in `tx packet steering` test case.
On 10/11/23 15:03, Ihar Hrachyshka wrote:
> On Tue, Oct 10, 2023 at 4:44 PM Ihar Hrachyshka wrote:
>
>> This is a canary test case transformation to validate hexify usefulness.
>> The test case was chosen as one that relies on hardcoded frames.
>>
>> Signed-off-by: Ihar Hrachyshka
>> ---
>> tests/dpif-netdev.at | 42 ++
>> 1 file changed, 18 insertions(+), 24 deletions(-)
>>
>> diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at
>> index 85119fb81..509c192fe 100644
>> --- a/tests/dpif-netdev.at
>> +++ b/tests/dpif-netdev.at
>> @@ -746,19 +746,24 @@ OVS_VSWITCHD_START(
>> # Modify the ip_dst addr to force changing the IP csum.
>> AT_CHECK([ovs-ofctl add-flow br1
>> in_port=p1,actions=mod_nw_dst:192.168.1.1,output:p2])
>>
>> +flow_s="\
>> +eth(src=8a:bf:7e:2f:05:84,dst=0a:8f:39:4f:e0:73),eth_type(0x0800),\
>> +ipv4(src=192.168.123.2,dst=192.168.123.1,proto=6,tos=0,ttl=64,frag=no),\
>> +tcp(src=54392,dst=5201),tcp_flags(ack)"
>> +
>> +frame=$(ovs-appctl ofproto/hexify ${flow_s})
>> +
>> # Check if no offload remains ok.
>> AT_CHECK([ovs-vsctl set Interface p2 options:tx_pcap=p2.pcap])
>> AT_CHECK([ovs-vsctl set Interface p1 options:ol_ip_csum=false])
>> AT_CHECK([ovs-vsctl set Interface p1 options:ol_ip_csum_set_good=false])
>> -AT_CHECK([ovs-appctl netdev-dummy/receive p1 \
>>
>> -0a8f394fe0738abf7e2f05840800453433e0400040068f8fc0a87b02c0a87b01d4781451a962ad5417ed297b801000e547fd0101080a2524d2345c7fe1c4
>> -])
>> +AT_CHECK([ovs-appctl netdev-dummy/receive p1 ${frame}])
>>
>> # Checksum should change to 0x990 with ip_dst changed to 192.168.1.1
>> # by the datapath while processing the packet.
>> +expected=$(ovs-appctl ofproto/hexify ${flow_s/192.168.123.1/192.168.1.1})
>> AT_CHECK([ovs-pcap p2.pcap > p2.pcap.txt 2>&1])
>> -AT_CHECK([tail -n 1 p2.pcap.txt], [0], [dnl
>>
>> -0a8f394fe0738abf7e2f05840800453433e0400040060990c0a87b02c0a80101d4781451a962ad5417ed297b801000e5c1fd0101080a2524d2345c7fe1c4
>> +AT_CHECK_UNQUOTED([tail -n 1 p2.pcap.txt], [0], [${expected}
>> ])
>>
>> # Check if packets entering the datapath with csum offloading
>> @@ -766,12 +771,9 @@ AT_CHECK([tail -n 1 p2.pcap.txt], [0], [dnl
>> # in the datapath and not by the netdev.
>> AT_CHECK([ovs-vsctl set Interface p1 options:ol_ip_csum=false])
>> AT_CHECK([ovs-vsctl set Interface p1 options:ol_ip_csum_set_good=true])
>> -AT_CHECK([ovs-appctl netdev-dummy/receive p1 \
>>
>> -0a8f394fe0738abf7e2f05840800453433e0400040068f8fc0a87b02c0a87b01d4781451a962ad5417ed297b801000e547fd0101080a2524d2345c7fe1c4
>> -])
>> +AT_CHECK([ovs-appctl netdev-dummy/receive p1 ${frame}])
>> AT_CHECK([ovs-pcap p2.pcap > p2.pcap.txt 2>&1])
>> -AT_CHECK([tail -n 1 p2.pcap.txt], [0], [dnl
>>
>> -0a8f394fe0738abf7e2f05840800453433e0400040060990c0a87b02c0a80101d4781451a962ad5417ed297b801000e5c1fd0101080a2524d2345c7fe1c4
>> +AT_CHECK_UNQUOTED([tail -n 1 p2.pcap.txt], [0], [${expected}
>> ])
>>
>> # Check if packets entering the datapath with csum offloading
>> @@ -779,36 +781,28 @@ AT_CHECK([tail -n 1 p2.pcap.txt], [0], [dnl
>> # by the datapath.
>> AT_CHECK([ovs-vsctl set Interface p1 options:ol_ip_csum=true])
>> AT_CHECK([ovs-vsctl set Interface p1 options:ol_ip_csum_set_good=true])
>> -AT_CHECK([ovs-appctl netdev-dummy/receive p1 \
>>
>> -0a8f394fe0738abf7e2f05840800453433e0400040068f8fc0a87b02c0a87b01d4781451a962ad5417ed297b801000e547fd0101080a2524d2345c7fe1c4
>> +AT_CHECK([ovs-appctl netdev-dummy/receive p1 ${frame}
>> ])
>> AT_CHECK([ovs-pcap p2.pcap > p2.pcap.txt 2>&1])
>> -AT_CHECK([tail -n 1 p2.pcap.txt], [0], [dnl
>>
>> -0a8f394fe0738abf7e2f05840800453433e0400040060990c0a87b02c0a80101d4781451a962ad5417ed297b801000e5c1fd0101080a2524d2345c7fe1c4
>> +AT_CHECK_UNQUOTED([tail -n 1 p2.pcap.txt], [0], [${expected}
>> ])
>>
>> # Push a packet with bad csum and offloading disabled to check
>> # if the datapath updates the csum, but does not fix the issue.
>> AT_CHECK([ovs-vsctl set Interface p1 options:ol_ip_csum=false])
>> AT_CHECK([ovs-vsctl set Interface p1 options:ol_ip_csum_set_good=false])
>> -AT_CHECK([ovs-appctl netdev-dummy/receive p1 \
>>
>> -0a8f394fe0738abf7e2f05840800453433e0400040068f03c0a87b02c0a87b01d4781451a962ad5417ed297b801000e547fd0101080a2524d2345c7fe1c4
>> -])
>> +AT_CHECK([ovs-appctl netdev-dummy/receive p1 ${frame/c9b6/c9b0}])
>>
>
> A note to myself: this is rather brittle - there's no intrinsic guarantee
> that the checksum of the final string will be c9b6 and not something else.
> It would be better to introduce some function that would flip checksum
> whatever it is, then use it here and below.
The substitution syntax you're using is also not portable, it doesn't
work in some shells. For example, it breaks tests on FreeBSD.
If we had a separate test binary for the hexification, we could add
various special flags for packet manipulations, like as the program
to corrupt the data or set the incorrect checksum.
What
Re: [ovs-dev] [PATCH 1/2] Implement ofproto/hexify command.
On 10/10/23 22:44, Ihar Hrachyshka wrote:
> The command receives a flow string and an optional additional payload
> and produces a hex-string representation of the corresponding frame.
>
> The command is datapath-independent and may be useful in tests, where we
> may need to produce hex frame payloads to compare observed frames
> against.
>
> Signed-off-by: Ihar Hrachyshka
Hi, Ihar. Thanks for the patch!
This is an interesting and fairly useful functionality for testing.
But I'm not sure why it is part of ofproto-dpif-trace and why it
is an appctl command. It should be simple to just create a separate
test binary or add it into ovstest instead. This way we will avoid
unnecessary connection to the ovs-vswitchd daemon to perform the
operation that doesn't seem to belong there.
If it were a separate binary we could also add more interesting
options for packet construction that may only make sense for testing
purposes. More on that in the reply to the next patch (incoming).
Separate binary may be better than ovstest integration if we want
to use it for OVN as well, because OVN has its own copy of ovstest
that will overshadow OVS'es, IIRC.
We have a few such small programs like test-stream, for example.
I didn't review the code in full, but see a few comments inline.
Best regards, Ilya Maximets.
> ---
> ofproto/ofproto-dpif-trace.c | 70
> ofproto/ofproto-unixctl.man | 36 ++-
> tests/ofproto-dpif.at| 45 +++
> 3 files changed, 150 insertions(+), 1 deletion(-)
>
> diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c
> index 527e2f17e..2e1931a1f 100644
> --- a/ofproto/ofproto-dpif-trace.c
> +++ b/ofproto/ofproto-dpif-trace.c
> @@ -469,6 +469,73 @@ free_ct_states(struct ovs_list *ct_states)
> }
> }
>
> +static void
> +ofproto_unixctl_hexify(struct unixctl_conn *conn, int argc,
> + const char *argv[], void *aux OVS_UNUSED)
> +{
> +ovs_assert(argc == 2 || argc == 3);
> +
> +char *error = NULL;
> +struct dp_packet *packet = NULL;
> +
> +uint8_t *l7 = NULL;
> +size_t l7_len = 0;
> +
> +struct ofpbuf odp_key = { 0 };
> +struct ofpbuf odp_mask = { 0 };
> +ofpbuf_init(_key, 0);
> +ofpbuf_init(_mask, 0);
> +
> +/* Extract additional payload, if passed */
Period at the end of a comment.
> +if (argc == 3) {
> +struct dp_packet payload;
An empty line.
> +memset(, 0, sizeof payload);
> +dp_packet_init(, 0);
> +if (dp_packet_put_hex(, argv[2], NULL)[0] != '\0') {
> +dp_packet_uninit();
> +error = xstrdup("Trailing garbage in payload data");
> +goto out;
> +}
> +l7_len = dp_packet_size();
> +l7 = dp_packet_steal_data();
> +}
> +
> +/* Flow string to flow. */
> +/* `hexify` command is backer agnostic, hence port_names=NULL */
Period.
> +if (odp_flow_from_string(argv[1], NULL, _key, _mask, )) {
> +goto out;
> +}
> +struct flow flow;
Some blank lines would be nice. Might make sense to just move to the top.
> +if (odp_flow_key_to_flow(odp_key.data, odp_key.size, , )
> +== ODP_FIT_ERROR) {
> +goto out;
> +}
> +
> +/* Flow to binary. */
> +packet = dp_packet_new(0);
> +flow_compose(packet, , l7, l7_len);
> +
> +/* Binary to hex string. */
> +struct ds result;
> +ds_init();
May use DS_EMPTY_INITIALIZER.
> +for (int i = 0; i < dp_packet_size(packet); i++) {
> +uint8_t val = ((uint8_t *) dp_packet_data(packet))[i];
> +ds_put_format(, "%02"PRIx8, val);
> +}
There is a ds_put_hex.
> +
> +unixctl_command_reply(conn, result.string);
> +ds_destroy();
> +out:
> +if (error) {
> +unixctl_command_reply_error(conn, error);
> +free(error);
> +}
> +dp_packet_delete(packet);
> +free(l7);
> +ofpbuf_uninit(_mask);
> +ofpbuf_uninit(_key);
> +}
> +
> static void
> ofproto_unixctl_trace(struct unixctl_conn *conn, int argc, const char
> *argv[],
>void *aux OVS_UNUSED)
> @@ -876,6 +943,9 @@ ofproto_dpif_trace_init(void)
> "[-consistent] {[dp_name] odp_flow | bridge br_flow} [OPTIONS...] "
> "[-generate|packet] actions",
> 2, INT_MAX, ofproto_unixctl_trace_actions, NULL);
> +unixctl_command_register(
> +"ofproto/hexify", "odp_flow [payload]",
> +1, 2, ofproto_unixctl_hexify, NULL);
> }
>
> void
> diff --git a/ofproto/ofproto-unixctl.man b/ofproto/ofproto-unixctl.man
> index 095afd57c..8dc68a38d 100644
> --- a/ofproto/ofproto-unixctl.man
> +++ b/ofproto/ofproto-unixctl.man
> @@ -4,7 +4,7 @@ These commands manage the core OpenFlow switch implementation
> (called
> .
> .IP "\fBofproto/list\fR"
> Lists the names of the running ofproto instances. These are the names
> -that may be used on \fBofproto/trace\fR.
> +that may be used on
Re: [ovs-dev] [PATCH] ofproto-dpif-upcall: Pause revalidators when purging.
On 9 Oct 2023, at 17:06, David Marchand wrote:
> A main thread executing the 'revalidator/purge' command could race with
> revalidator threads that can be dumping/sweeping the purged flows at the
> same time.
>
> This race can be reproduced (with dpif debug logs) by running the
> conntrack - ICMP related unit tests with the userspace datapath:
>
> 2023-10-09T14:11:55.242Z|00177|unixctl|DBG|received request
> revalidator/purge[], id=0
> 2023-10-09T14:11:55.242Z|00044|dpif(revalidator17)|DBG|netdev@ovs-netdev:
> flow_dump ufid:68ff6817-fb3b-4b30-8412-9cf175318294 ,
> packets:0, bytes:0, used:never
> 2023-10-09T14:11:55.242Z|00178|dpif|DBG|netdev@ovs-netdev: flow_del
> ufid:07046e91-30a6-4862-9048-1a76b5a88a5b
> recirc_id(0),dp_hash(0),skb_priority(0),in_port(2),skb_mark(0),
> ct_state(0),ct_zone(0),ct_mark(0),ct_label(0),
> packet_type(ns=0,id=0),
> eth(src=a6:0a:bf:e2:f3:f2,dst=62:23:0f:f6:2c:75),
> eth_type(0x0800),ipv4(src=10.1.1.1,dst=10.1.1.2,proto=17,tos=0,
> ttl=64,frag=no),udp(src=37380,dst=1), packets:0, bytes:0,
> used:never
> ...
> 2023-10-09T14:11:55.242Z|00049|dpif(revalidator17)|WARN|netdev@ovs-netdev:
> failed to flow_get (No such file or directory)
> ufid:07046e91-30a6-4862-9048-1a76b5a88a5b , packets:0,
> bytes:0, used:never
> 2023-10-09T14:11:55.242Z|00050|ofproto_dpif_upcall(revalidator17)|WARN|
> Failed to acquire udpif_key corresponding to unexpected flow
> (No such file or directory):
> ufid:07046e91-30a6-4862-9048-1a76b5a88a5b
> ...
> 2023-10-09T14:11:55.242Z|00183|unixctl|DBG|replying with success, id=0: ""
>
> To avoid this race, a first part of the fix is to pause (if not already
> paused) the revalidators while the main thread is purging the datapath
> flows.
>
> Then a second issue is observed by running the same unit test with the
> kernel datapath. Its dpif implementation dumps flows via a netlink request
> (see dpif_flow_dump_create(), dpif_netlink_flow_dump_create(),
> nl_dump_start(), nl_sock_send__()) in the leader revalidator thread,
> before pausing revalidators:
>
> 2023-10-09T14:44:28.742Z|00122|unixctl|DBG|received request
> revalidator/purge[], id=0
> ...
> 2023-10-09T14:44:28.742Z|00125|dpif|DBG|system@ovs-system: flow_del
> ufid:70102d81-30a1-44b9-aa76-3d02a9ffd2c9 recirc_id(0),dp_hash(0),
> skb_priority(0),in_port(2),skb_mark(0),ct_state(0),ct_zone(0),
> ct_mark(0),ct_label(0),eth(src=a6:0a:bf:e2:f3:f2,
> dst=ff:ff:ff:ff:ff:ff),eth_type(0x0806),arp(sip=10.1.1.1,
> tip=10.1.1.2,op=1,sha=a6:0a:bf:e2:f3:f2,tha=00:00:00:00:00:00),
> packets:0, bytes:0, used:never
> ...
> 2023-10-09T14:44:28.742Z|00129|unixctl|DBG|replying with success, id=0: ""
> ...
> 2023-10-09T14:44:28.742Z|6|dpif(revalidator21)|DBG|system@ovs-system:
> flow_dump ufid:70102d81-30a1-44b9-aa76-3d02a9ffd2c9 ,
> packets:0, bytes:0, used:never
> ...
> 2023-10-09T14:44:28.742Z|00012|dpif(revalidator21)|WARN|system@ovs-system:
> failed to flow_get (No such file or directory)
> ufid:70102d81-30a1-44b9-aa76-3d02a9ffd2c9 , packets:0,
> bytes:0, used:never
> 2023-10-09T14:44:28.742Z|00013|ofproto_dpif_upcall(revalidator21)|WARN|
> Failed to acquire udpif_key corresponding to unexpected flow
> (No such file or directory):
> ufid:70102d81-30a1-44b9-aa76-3d02a9ffd2c9
>
> To avoid evaluating already deleted flows, the second part of the fix is
> to ensure that dumping from the leader revalidator thread is done out of
> any pause request.
>
> Fixes: 98bb4286970d ("tests: Add command to purge revalidators of flows.")
> Signed-off-by: David Marchand
> ---
> Note: with this fix, I can make most of system-traffic tests run fine
> under make check-dpdk check-kernel check-offloads.
> "Most", because a unit test "datapath - basic truncate action" fails for
> me though it predates this patch, and I did not understand why it fails
> yet.
I’m not getting this error in my test environment. I tested all datapaths and
did not get any errors.
> The check-offloads target can run fine if removing the exception on
> "failed to flow_get" and "failed to acquire ukey" warning logs.
Would it be good to add another patch in the series, to remove these?
This patch looks good to me!
Acked-by: Eelco Chaudron
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 1/2] vswitch.xml: Add dpdkvhostuser group status.
On 10/12/23 14:15, Kevin Traynor wrote:
> Add group for dpdkvhostuser(/client) netdev.
>
> Adding as a single group as they display the same status,
> one of which is 'mode' to indicate if it's client or server.
>
> Fixes: b2e8b12f8a82 ("netdev-dpdk: add vhost-user get_status.")
> Signed-off-by: Kevin Traynor
> ---
> vswitchd/vswitch.xml | 28
> 1 file changed, 28 insertions(+)
>
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index 1e2a1267d..403b954c2 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -3822,4 +3822,32 @@ ovs-vsctl add-port br0 p0 -- set Interface p0
> type=patch options:peer=p1 \
>
>
> +
> +
> +
> + dpdkvhostuser and dpdkvhostuserclient
> + netdev specific interface status options.
The 'options' might be not a good word here as these are not options,
i.e. users can't choose or set them. I see that 'dpdk' group uses
the same word, but we might need to change that as well. Some other
places use 'status information', for example.
> +
> +
> +Client or server vhost mode.
> +
> +
> +vhost features bitmap.
> +
> +
> +Number of vrings.
> +
> +
> +Vhost numa association.
> +
> +
> +Vhost socket path.
> +
> +
> +Status of connection.
> +
> +
> +Size of vring n.
This is a very confusing description. Size of 'n'? :)
Most of other descriptions are also not very descriptive. I'm not
sure what is the value of adding these. I'd say we should either
document them better, so the average user can understand what they
mean, or just not describe at all.
What do you think?
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 4/5] conntrack: support default timeout policy get/set cmd for netdev datapath
On Fri, Jan 28, 2022 at 11:14:46AM -0500, Aaron Conole wrote: > From: wenxu > > Now, the default timeout policy for netdev datapath is hard codeing. In > some case show or modify is needed. > Add command for get/set default timeout policy. Using like this: > > ovs-appctl dpctl/ct-get-default-tp [dp] > ovs-appctl dpctl/ct-set-default-tp [dp] policies > > Signed-off-by: wenxu > Signed-off-by: Aaron Conole Hi Aaron, This patch appears to have gone stale in patchwork, for one reason or another. If it is still relevant then I think it needs to be revisited, by being reposted after appropriate preparation. As such I'm marking this patch as "Deferred" in patchwork. No action is required unless there is a desire to revisit this patch. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v5] netdev: Sync'ed and cleaned {get, set}_config().
On 12.10.23 17:39, Robin Jarry wrote: > Kevin Traynor, Oct 12, 2023 at 17:34: >> ok, 'rss' is documented as default, so maybe we don't need to display if it >> is in use by default, selected by user or as fallback. >> >> That would make things a bit easier as 'rx-steering:' is free to use to >> indicate the unsupported case. >> >> So maybe status could just have: >> 'rx-steering:rss+lacp' for user enabled rss+lacp and NIC supports >> 'rx-steering:unsupported' for user enabled rss+lacp and NIC does not support. >> >> If rx-steering is not reported, then it is using the default 'rss'. >> >> Robin, what do you think ? > > It may be surprising to users not to see any mention in get_config() when > explicitly setting rx-steering=rss but I don't see that as a common/standard > use case. So I think it should be OK. > Thank you Kevin and Robin ☺️ Your change requests have been incorporated in v6: https://patchwork.ozlabs.org/project/openvswitch/list/?series=377463 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v6 3/3] netdev-afxdp: Sync and clean {get, set}_config() callbacks.
From: Jakob Meng
For better usability, the function pairs get_config() and
set_config() for netdevs should be symmetric: Options which are
accepted by set_config() should be returned by get_config() and the
latter should output valid options for set_config() only. This patch
also moves key-value pairs which are no valid options from get_config()
to the get_status() callback.
The documentation in vswitchd/vswitch.xml for status columns has been
updated accordingly.
Reported-at: https://bugzilla.redhat.com/1949855
Signed-off-by: Jakob Meng
---
Documentation/intro/install/afxdp.rst | 12
lib/netdev-afxdp.c| 21 +++--
lib/netdev-afxdp.h| 1 +
lib/netdev-linux-private.h| 1 +
lib/netdev-linux.c| 4 ++--
vswitchd/vswitch.xml | 11 +++
6 files changed, 38 insertions(+), 12 deletions(-)
diff --git a/Documentation/intro/install/afxdp.rst
b/Documentation/intro/install/afxdp.rst
index 51c24bf5b..5776614c8 100644
--- a/Documentation/intro/install/afxdp.rst
+++ b/Documentation/intro/install/afxdp.rst
@@ -219,14 +219,10 @@ Otherwise, enable debugging by::
ovs-appctl vlog/set netdev_afxdp::dbg
To check which XDP mode was chosen by ``best-effort``, you can look for
-``xdp-mode-in-use`` in the output of ``ovs-appctl dpctl/show``::
-
- # ovs-appctl dpctl/show
- netdev@ovs-netdev:
-<...>
-port 2: ens802f0 (afxdp: n_rxq=1, use-need-wakeup=true,
- xdp-mode=best-effort,
- xdp-mode-in-use=native-with-zerocopy)
+``xdp-mode`` in the output of ``ovs-vsctl get interface INT status:xdp-mode``::
+
+ # ovs-vsctl get interface ens802f0 status:xdp-mode
+ "native-with-zerocopy"
References
--
diff --git a/lib/netdev-afxdp.c b/lib/netdev-afxdp.c
index 16f26bc30..8519b5a2b 100644
--- a/lib/netdev-afxdp.c
+++ b/lib/netdev-afxdp.c
@@ -672,8 +672,6 @@ netdev_afxdp_get_config(const struct netdev *netdev, struct
smap *args)
ovs_mutex_lock(>mutex);
smap_add_format(args, "n_rxq", "%d", netdev->n_rxq);
smap_add_format(args, "xdp-mode", "%s", xdp_modes[dev->xdp_mode].name);
-smap_add_format(args, "xdp-mode-in-use", "%s",
-xdp_modes[dev->xdp_mode_in_use].name);
smap_add_format(args, "use-need-wakeup", "%s",
dev->use_need_wakeup ? "true" : "false");
ovs_mutex_unlock(>mutex);
@@ -1367,3 +1365,22 @@ netdev_afxdp_get_stats(const struct netdev *netdev,
return error;
}
+
+int
+netdev_afxdp_get_status(const struct netdev *netdev,
+struct smap *args)
+{
+int error = netdev_linux_get_status(netdev, args);
+if (error) {
+return error;
+}
+
+struct netdev_linux *dev = netdev_linux_cast(netdev);
+
+ovs_mutex_lock(>mutex);
+smap_add_format(args, "xdp-mode", "%s",
+xdp_modes[dev->xdp_mode_in_use].name);
+ovs_mutex_unlock(>mutex);
+
+return error;
+}
diff --git a/lib/netdev-afxdp.h b/lib/netdev-afxdp.h
index e91cd102d..bd3b9dfbe 100644
--- a/lib/netdev-afxdp.h
+++ b/lib/netdev-afxdp.h
@@ -63,6 +63,7 @@ int netdev_afxdp_set_config(struct netdev *netdev, const
struct smap *args,
int netdev_afxdp_get_config(const struct netdev *netdev, struct smap *args);
int netdev_afxdp_get_stats(const struct netdev *netdev_,
struct netdev_stats *stats);
+int netdev_afxdp_get_status(const struct netdev *netdev, struct smap *args);
int netdev_afxdp_get_custom_stats(const struct netdev *netdev,
struct netdev_custom_stats *custom_stats);
diff --git a/lib/netdev-linux-private.h b/lib/netdev-linux-private.h
index 0ecf0f748..188e8438a 100644
--- a/lib/netdev-linux-private.h
+++ b/lib/netdev-linux-private.h
@@ -50,6 +50,7 @@ struct netdev_rxq_linux {
};
int netdev_linux_construct(struct netdev *);
+int netdev_linux_get_status(const struct netdev *, struct smap *);
void netdev_linux_run(const struct netdev_class *);
int get_stats_via_netlink(const struct netdev *netdev_,
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index cca340879..70521e3c7 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -3493,7 +3493,7 @@ netdev_linux_get_next_hop(const struct in_addr *host,
struct in_addr *next_hop,
return ENXIO;
}
-static int
+int
netdev_linux_get_status(const struct netdev *netdev_, struct smap *smap)
{
struct netdev_linux *netdev = netdev_linux_cast(netdev_);
@@ -3759,7 +3759,7 @@ const struct netdev_class netdev_internal_class = {
.destruct = netdev_afxdp_destruct, \
.get_stats = netdev_afxdp_get_stats,\
.get_custom_stats = netdev_afxdp_get_custom_stats, \
-.get_status = netdev_linux_get_status, \
+.get_status = netdev_afxdp_get_status, \
.set_config = netdev_afxdp_set_config, \
[ovs-dev] [PATCH v6 1/3] netdev-dpdk: Sync and clean {get, set}_config() callbacks.
From: Jakob Meng
For better usability, the function pairs get_config() and
set_config() for netdevs should be symmetric: Options which are
accepted by set_config() should be returned by get_config() and the
latter should output valid options for set_config() only.
This patch moves key-value pairs which are no valid options from
get_config() to the get_status() callback. For example, get_config()
in lib/netdev-dpdk.c returned {configured,requested}_{rx,tx}_queues
previously. For requested rx queues the proper option name is n_rxq,
so requested_rx_queues has been renamed respectively. Tx queues
cannot be changed by the user, hence requested_tx_queues has been
dropped. Both configured_{rx,tx}_queues will be returned as
n_{r,t}xq in the get_status() callback.
The documentation in vswitchd/vswitch.xml for status columns as well
as tests have been updated accordingly.
Reported-at: https://bugzilla.redhat.com/1949855
Signed-off-by: Jakob Meng
---
Documentation/topics/dpdk/phy.rst | 4 +-
lib/netdev-dpdk.c | 104 +-
tests/system-dpdk.at | 64 +++---
vswitchd/vswitch.xml | 14 +++-
4 files changed, 127 insertions(+), 59 deletions(-)
diff --git a/Documentation/topics/dpdk/phy.rst
b/Documentation/topics/dpdk/phy.rst
index f66b106c4..41cc3588a 100644
--- a/Documentation/topics/dpdk/phy.rst
+++ b/Documentation/topics/dpdk/phy.rst
@@ -198,7 +198,7 @@ Example::
a dedicated queue, it will be explicit::
$ ovs-vsctl get interface dpdk-p0 status
- {..., rx_steering=unsupported}
+ {..., rx-steering=unsupported}
More details can often be found in ``ovs-vswitchd.log``::
@@ -499,7 +499,7 @@ its options::
$ ovs-appctl dpctl/show
[...]
- port 3: dpdk-rep0 (dpdk: configured_rx_queues=1, ...,
dpdk-vf-mac=00:11:22:33:44:55, ...)
+ port 3: dpdk-rep0 (dpdk: ..., dpdk-vf-mac=00:11:22:33:44:55, ...)
$ ovs-vsctl show
[...]
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 55700250d..56588f92a 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -1905,31 +1905,32 @@ netdev_dpdk_get_config(const struct netdev *netdev,
struct smap *args)
ovs_mutex_lock(>mutex);
-smap_add_format(args, "requested_rx_queues", "%d", dev->user_n_rxq);
-smap_add_format(args, "configured_rx_queues", "%d", netdev->n_rxq);
-smap_add_format(args, "requested_tx_queues", "%d", dev->requested_n_txq);
-smap_add_format(args, "configured_tx_queues", "%d", netdev->n_txq);
-smap_add_format(args, "mtu", "%d", dev->mtu);
+smap_add_format(args, "dpdk-devargs", "%s", dev->devargs);
+smap_add_format(args, "n_rxq", "%d", dev->user_n_rxq);
+smap_add_format(args, "rx-flow-ctrl", "%s",
+dev->fc_conf.mode == RTE_ETH_FC_TX_PAUSE ||
+dev->fc_conf.mode == RTE_ETH_FC_FULL ? "true" : "false");
+smap_add_format(args, "tx-flow-ctrl", "%s",
+dev->fc_conf.mode == RTE_ETH_FC_RX_PAUSE ||
+dev->fc_conf.mode == RTE_ETH_FC_FULL ? "true" : "false");
+smap_add_format(args, "flow-ctrl-autoneg", "%s",
+dev->fc_conf.autoneg ? "true" : "false");
-if (dev->type == DPDK_DEV_ETH) {
-smap_add_format(args, "n_rxq_desc", "%d", dev->rxq_size);
-smap_add_format(args, "n_txq_desc", "%d", dev->txq_size);
-if (dev->hw_ol_features & NETDEV_RX_CHECKSUM_OFFLOAD) {
-smap_add(args, "rx_csum_offload", "true");
-} else {
-smap_add(args, "rx_csum_offload", "false");
-}
-if (dev->rx_steer_flags == DPDK_RX_STEER_LACP) {
-smap_add(args, "rx-steering", "rss+lacp");
-}
-smap_add(args, "lsc_interrupt_mode",
- dev->lsc_interrupt_mode ? "true" : "false");
+smap_add_format(args, "n_rxq_desc", "%d", dev->rxq_size);
+smap_add_format(args, "n_txq_desc", "%d", dev->txq_size);
-if (dpdk_port_is_representor(dev)) {
-smap_add_format(args, "dpdk-vf-mac", ETH_ADDR_FMT,
-ETH_ADDR_ARGS(dev->requested_hwaddr));
-}
+if (dev->rx_steer_flags == DPDK_RX_STEER_LACP) {
+smap_add(args, "rx-steering", "rss+lacp");
+}
+
+smap_add(args, "dpdk-lsc-interrupt",
+dev->lsc_interrupt_mode ? "true" : "false");
+
+if (dpdk_port_is_representor(dev)) {
+smap_add_format(args, "dpdk-vf-mac", ETH_ADDR_FMT,
+ETH_ADDR_ARGS(dev->requested_hwaddr));
}
+
ovs_mutex_unlock(>mutex);
return 0;
@@ -2324,6 +2325,29 @@ out:
return err;
}
+static int
+netdev_dpdk_vhost_client_get_config(const struct netdev *netdev,
+struct smap *args)
+{
+struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
+
+ovs_mutex_lock(>mutex);
+
+if (dev->vhost_id) {
+smap_add(args, "vhost-server-path", dev->vhost_id);
+}
+
+int tx_retries_max;
+
[ovs-dev] [PATCH v6 2/3] netdev-dummy: Sync and clean {get, set}_config() callbacks.
From: Jakob Meng
For better usability, the function pairs get_config() and
set_config() for netdevs should be symmetric: Options which are
accepted by set_config() should be returned by get_config() and the
latter should output valid options for set_config() only. This patch
also moves key-value pairs which are no valid options from get_config()
to the get_status() callback. The tests have been updated accordingly.
Reported-at: https://bugzilla.redhat.com/1949855
Signed-off-by: Jakob Meng
---
lib/netdev-dummy.c | 19 +++
tests/pmd.at | 26 +-
2 files changed, 28 insertions(+), 17 deletions(-)
diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
index 1a54add87..fe82317d7 100644
--- a/lib/netdev-dummy.c
+++ b/lib/netdev-dummy.c
@@ -795,14 +795,25 @@ netdev_dummy_get_config(const struct netdev *dev, struct
smap *args)
dummy_packet_conn_get_config(>conn, args);
+/* pcap, rxq_pcap and tx_pcap cannot be recovered because filenames have
+ * been discarded after opening file descriptors */
+
+if (netdev->ol_ip_csum) {
+smap_add_format(args, "ol_ip_csum", "%s", "true");
+}
+
+if (netdev->ol_ip_csum_set_good) {
+smap_add_format(args, "ol_ip_csum_set_good", "%s", "true");
+}
+
/* 'dummy-pmd' specific config. */
if (!netdev_is_pmd(dev)) {
goto exit;
}
-smap_add_format(args, "requested_rx_queues", "%d",
netdev->requested_n_rxq);
-smap_add_format(args, "configured_rx_queues", "%d", dev->n_rxq);
-smap_add_format(args, "requested_tx_queues", "%d",
netdev->requested_n_txq);
-smap_add_format(args, "configured_tx_queues", "%d", dev->n_txq);
+
+smap_add_format(args, "n_rxq", "%d", netdev->requested_n_rxq);
+smap_add_format(args, "n_txq", "%d", netdev->requested_n_txq);
+smap_add_format(args, "numa_id", "%d", netdev->requested_numa_id);
exit:
ovs_mutex_unlock(>mutex);
diff --git a/tests/pmd.at b/tests/pmd.at
index 7bdaca9e7..df6adde6c 100644
--- a/tests/pmd.at
+++ b/tests/pmd.at
@@ -93,11 +93,11 @@ pmd thread numa_id core_id :
overhead: NOT AVAIL
])
-AT_CHECK([ovs-appctl dpif/show | sed
's/\(tx_queues=\)[[0-9]]*/\1/g'], [0], [dnl
+AT_CHECK([ovs-appctl dpif/show], [0], [dnl
dummy@ovs-dummy: hit:0 missed:0
br0:
br0 65534/100: (dummy-internal)
-p0 1/1: (dummy-pmd: configured_rx_queues=1,
configured_tx_queues=, requested_rx_queues=1,
requested_tx_queues=)
+p0 1/1: (dummy-pmd: n_rxq=1, n_txq=1, numa_id=0)
])
OVS_VSWITCHD_STOP
@@ -111,11 +111,11 @@ CHECK_PMD_THREADS_CREATED()
AT_CHECK([ovs-vsctl set interface p0 options:n_rxq=8])
-AT_CHECK([ovs-appctl dpif/show | sed
's/\(tx_queues=\)[[0-9]]*/\1/g'], [0], [dnl
+AT_CHECK([ovs-appctl dpif/show], [0], [dnl
dummy@ovs-dummy: hit:0 missed:0
br0:
br0 65534/100: (dummy-internal)
-p0 1/1: (dummy-pmd: configured_rx_queues=8,
configured_tx_queues=, requested_rx_queues=8,
requested_tx_queues=)
+p0 1/1: (dummy-pmd: n_rxq=8, n_txq=1, numa_id=0)
])
AT_CHECK([ovs-appctl dpif-netdev/pmd-rxq-show | sed SED_NUMA_CORE_PATTERN],
[0], [dnl
@@ -144,11 +144,11 @@ OVS_VSWITCHD_START([add-port br0 p0 -- set Interface p0
type=dummy-pmd options:n
CHECK_CPU_DISCOVERED(2)
CHECK_PMD_THREADS_CREATED()
-AT_CHECK([ovs-appctl dpif/show | sed
's/\(tx_queues=\)[[0-9]]*/\1/g'], [0], [dnl
+AT_CHECK([ovs-appctl dpif/show], [0], [dnl
dummy@ovs-dummy: hit:0 missed:0
br0:
br0 65534/100: (dummy-internal)
-p0 1/1: (dummy-pmd: configured_rx_queues=8,
configured_tx_queues=, requested_rx_queues=8,
requested_tx_queues=)
+p0 1/1: (dummy-pmd: n_rxq=8, n_txq=1, numa_id=0)
])
AT_CHECK([ovs-appctl dpif-netdev/pmd-rxq-show | sed SED_NUMA_CORE_PATTERN],
[0], [dnl
@@ -227,11 +227,11 @@ TMP=$(($(cat ovs-vswitchd.log | wc -l | tr -d
[[:blank:]])+1))
CHECK_CPU_DISCOVERED(4)
CHECK_PMD_THREADS_CREATED()
-AT_CHECK([ovs-appctl dpif/show | sed
's/\(tx_queues=\)[[0-9]]*/\1/g'], [0], [dnl
+AT_CHECK([ovs-appctl dpif/show | sed 's/\(numa_id=\)[[0-9]]*/\1/g'],
[0], [dnl
dummy@ovs-dummy: hit:0 missed:0
br0:
br0 65534/100: (dummy-internal)
-p0 1/1: (dummy-pmd: configured_rx_queues=8,
configured_tx_queues=, requested_rx_queues=8,
requested_tx_queues=)
+p0 1/1: (dummy-pmd: n_rxq=8, n_txq=1, numa_id=)
])
AT_CHECK([ovs-appctl dpif-netdev/pmd-rxq-show | sed SED_NUMA_CORE_PATTERN],
[0], [dnl
@@ -436,11 +436,11 @@ AT_CHECK([ovs-vsctl set Open_vSwitch .
other_config:smc-enable=true])
sleep 1
-AT_CHECK([ovs-appctl dpif/show | sed
's/\(tx_queues=\)[[0-9]]*/\1/g'], [0], [dnl
+AT_CHECK([ovs-appctl dpif/show], [0], [dnl
dummy@ovs-dummy: hit:0 missed:0
br0:
br0 65534/100: (dummy-internal)
-p0 7/1: (dummy-pmd: configured_rx_queues=4,
configured_tx_queues=, requested_rx_queues=4,
requested_tx_queues=)
+p0 7/1: (dummy-pmd: n_rxq=4, n_txq=1, numa_id=0)
])
AT_CHECK([ovs-appctl dpif-netdev/pmd-stats-show | sed SED_NUMA_CORE_PATTERN |
sed '/cycles/d' | grep pmd
[ovs-dev] [PATCH v6 0/3] netdev: Sync and clean {get, set}_config() callbacks.
From: Jakob Meng
For better usability, the function pairs get_config() and
set_config() for netdevs should be symmetric: Options which are
accepted by set_config() should be returned by get_config() and the
latter should output valid options for set_config() only.
This patch series moves key-value pairs which are no valid options
from get_config() to the get_status() callback. The documentation in
vswitchd/vswitch.xml for status columns as well as tests have been
updated accordingly.
Compared to v4, the patch has been split into a series. Change requests
from Kevin Traynor have been incorporated, e.g. n_{r,t}xq will be
reported in dpkvhostclient status and tx-steering in the dpdk status
will be "unsupported" if the hw does not support steering traffic to
additional rxq.
The netdev dpdk classes no longer share a common get_config callback,
instead both the dpdk_class and the dpdk_vhost_client_class defines
their own callbacks. For dpdk_vhost_client_class both config options
vhost-server-path and tx-retries-max are returned which were missed in
the previous patch version.
Jakob Meng (3):
netdev-dpdk: Sync and clean {get,set}_config() callbacks.
netdev-dummy: Sync and clean {get,set}_config() callbacks.
netdev-afxdp: Sync and clean {get,set}_config() callbacks.
Documentation/intro/install/afxdp.rst | 12 +--
Documentation/topics/dpdk/phy.rst | 4 +-
lib/netdev-afxdp.c| 21 +-
lib/netdev-afxdp.h| 1 +
lib/netdev-dpdk.c | 104 ++
lib/netdev-dummy.c| 19 -
lib/netdev-linux-private.h| 1 +
lib/netdev-linux.c| 4 +-
tests/pmd.at | 26 +++
tests/system-dpdk.at | 64 ++--
vswitchd/vswitch.xml | 25 ++-
11 files changed, 193 insertions(+), 88 deletions(-)
--
2.39.2
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] relay:probe interval of relay server
On Mon, Jan 10, 2022 at 10:42:16AM +0800, Wentao Jia wrote: > probe interval of relay server can be configured from > "sb_global/nb_global options:relay_probe_interval" > minimum and default value is 5s > > > Signed-off-by: Wentao Jia Hi Wentao Jia, This patch appears to have gone stale in patchwork, for one reason or another. If it is still relevant then I think it needs to be revisited, by being reposted after appropriate preparation. As such I'm marking this patch as "Deferred" in patchwork. No action is required unless there is a desire to revisit this patch. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ovsdb-server:Add parameter "relay-leader-only-mode"
On Fri, Jan 07, 2022 at 11:48:01AM +0800, Wentao Jia wrote: > a lot of transactions maybe fail in cluster ovsdb because of > prerequisite check mechanism, relay server connect to leader can > avoid this issue > > > Signed-off-by: Wentao Jia Hi Wentao Jia, This patch appears to have gone stale in patchwork, for one reason or another. If it is still relevant then I think it needs to be revisited, by being reposted after appropriate preparation. As such I'm marking this patch as "Deferred" in patchwork. No action is required unless there is a desire to revisit this patch. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4 1/3] upcall: prevent from installing flows when inconsistence
On Sun, Nov 21, 2021 at 11:20:55PM +0800, lic121 wrote: > In ovs kernel datapath upcall, the *key* and packet are passed to > userspace. The key contains the fields/meta extracted from packet. > Once the ovs-vswitchd receives the upcall, the packet is extracted > again into *flow*. Next, the flow is used to match openflow rules to > generate the wildcard(wc). At last, vswitchd installs a mega_flow in > datapath(mega_flow = key/wc,action) > > We can see that vswitchd generate wc from flow while it installs dp > flow with key. If the key is not consistent with the flow [1], we get > bad mega_flow. > > Let's assume we have the flowing rules, means to block tcp port 0-0xf, > but allow other ports. > > "table=0,priority=100,tcp,tp_dst=0x0/0xfff0 actions=drop" > "table=0,priority=90,tcp actions=p1" > > good case: > If a packet has tcp dst=0x10, generated `mega_flow=0x10/0xfff0,out:p1`, > this is expected. > > bad case: > If a packet has tcp dst=0x10 but not pass tcphdr_ok [1], generated wc > and action are `0xfff0,out:p1`. The mega_flow will be > `0x0/0xfff0,out:p1`, bacause mega_flow=key/wc,action. This allows > packets with tcp port 0-0xf pass by mistake. > > The following scapy3 script triggers the issue: > ```py > eth=Ether(src="fa:16:3e:5e:e3:57",dst="be:95:df:40:fb:57") > ip=IP(src="10.10.10.10",dst="20.20.20.20") > tcp=TCP(sport=100,dport=16,dataofs=1) > sendp(eth/ip/tcp) > ``` > > This patch is to prevent from installing datapath flow if the key is > not consistant with the flow. > > [1] https://github.com/openvswitch/ovs/blob/v2.16.1/datapath/flow.c#L601 > > Signed-off-by: lic121 Hi, This patch appears to have gone stale in patchwork, for one reason or another. If it is still relevant then I think it needs to be revisited, by being reposted after appropriate preparation. As such I'm marking this patch as "Deferred" in patchwork. No action is required unless there is a desire to revisit this patch. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 0/7] net: openvswitch: Reduce stack usage
From: Nicholas Piggin > Sent: 12 October 2023 02:19 ... > It is a kernel crash, so we need something for stable. But In a case > like this there's not one single problem. Linux kernel stack use has > always been pretty dumb - "don't use too much", for some values of > too much, and just cross fingers config and compiler and worlkoad > doesn't hit some overflow case. I think it ought to be possible to use a FINE_IBT (I think that is what it is called) compile to get indirect calls grouped and change objtool to output the stack offset of every call. It is then a simple (for some definition of simple) process to work out the static maximum stack usage. Any recursion causes grief (the stack depth for each loop can be reported). Also I suspect the compiler will need an enhancement to add a constant into the hash to disambiguate indirect calls with the same argument list. My suspicion (from doing this exercise on an embedded system a long time ago) is that the stack will be blown somewhere inside printk() in some error path. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v6 2/2] netdev-tc-offload: Add IPIP/GRE protocols to offload in ip rewrite.
On Sat, Oct 07, 2023 at 11:49:46AM +0800, Faicker Mo via dev wrote:
> Currently checksum recalculation is not supported with TC offload for
> IPIP and GRE packets. This patch adds support for TC offloading of
> IPIP and GRE packets by adding the correct csum action.
>
> Without this patch the following warning can be seen in the logging:
> Can't offload rewrite of IP/IPV6 with ip_proto: X.
>
> Fixes: d6118e628988 ("netdev-tc-offloads: Verify csum flags on dump from tc")
> Signed-off-by: Faicker Mo
Acked-by: Simon Horman
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v6 1/2] netdev-tc-offload: Add csum offload of IGMP/UDPLITE/SCTP in ip rewrite.
On Sat, Oct 07, 2023 at 11:49:42AM +0800, Faicker Mo via dev wrote: > When the IP header is modified, for example, by NAT or a ToS/TTL change, > the IP header checksum needs recalculation. In addition to the IP header > checksum, for UDPLITE, its checksum also needs recalculation when any > of the addresses change. > > This patch adds support for TC offloading of IGMP, UDPLITE, and SCTP > packets by adding the correct csum action. > > Signed-off-by: Faicker Mo Acked-by: Simon Horman ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3] bond: Always revalidate unbalanced bonds when active member changes
On 8 Oct 2023, at 7:26, Mike Pattrick wrote:
> Currently a bond will not always revalidate when an active member
> changes. This can result in counter-intuitive behaviors like the fact
> that using ovs-appctl bond/set-active-member will cause the bond to
> revalidate but changing other_config:bond-primary will not trigger a
> revalidate in the bond.
>
> When revalidation is not set but the active member changes in an
> unbalanced bond, OVS may send traffic out of previously active member
> instead of the new active member.
>
> This change will always mark unbalanced bonds for revalidation if the
> active member changes.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2214979
> Signed-off-by: Mike Pattrick
Hi Mike,
Thanks for the patch. Some comments below, and the subject needs to end with a
dot.
//Eelco
> ---
> v2: Added a test
> v3: Made the test more reliable
> ---
> ofproto/bond.c | 8 +--
> tests/system-traffic.at | 50 +
> 2 files changed, 56 insertions(+), 2 deletions(-)
>
> diff --git a/ofproto/bond.c b/ofproto/bond.c
> index cfdf44f85..fb108d30a 100644
> --- a/ofproto/bond.c
> +++ b/ofproto/bond.c
> @@ -193,6 +193,7 @@ static void bond_update_post_recirc_rules__(struct bond
> *, bool force)
> static bool bond_is_falling_back_to_ab(const struct bond *);
> static void bond_add_lb_output_buckets(const struct bond *);
> static void bond_del_lb_output_buckets(const struct bond *);
> +static bool bond_is_balanced(const struct bond *bond) OVS_REQ_RDLOCK(rwlock);
>
>
> /* Attempts to parse 's' as the name of a bond balancing mode. If
> successful,
> @@ -552,11 +553,15 @@ bond_find_member_by_mac(const struct bond *bond, const
> struct eth_addr mac)
>
> static void
> bond_active_member_changed(struct bond *bond)
> +OVS_REQ_WRLOCK(rwlock)
> {
> if (bond->active_member) {
> struct eth_addr mac;
> netdev_get_etheraddr(bond->active_member->netdev, );
> bond->active_member_mac = mac;
> +if (!bond_is_balanced(bond)) {
> +bond->bond_revalidate = true;
> +}
> } else {
> bond->active_member_mac = eth_addr_zero;
> }
> @@ -1124,7 +1129,7 @@ bond_get_recirc_id_and_hash_basis(struct bond *bond,
> uint32_t *recirc_id,
> /* Rebalancing. */
>
> static bool
> -bond_is_balanced(const struct bond *bond) OVS_REQ_RDLOCK(rwlock)
Any reason why the annotation was removed? clang would be smart enough to see
that a write lock is fine for a readlock. It seems to compile fine here. Or
were you trying to clean this up and move it to the next line :)
> +bond_is_balanced(const struct bond *bond)
> {
> return bond->rebalance_interval
> && (bond->balance == BM_SLB || bond->balance == BM_TCP)
> @@ -1728,7 +1733,6 @@ bond_unixctl_set_active_member(struct unixctl_conn
> *conn,
> }
>
> if (bond->active_member != member) {
> -bond->bond_revalidate = true;
> bond->active_member = member;
> VLOG_INFO("bond %s: active member is now %s",
>bond->name, member->name);
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index 945037ec0..7075c35ea 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -291,6 +291,56 @@ NS_CHECK_EXEC([at_ns0], [ping -s 3200 -q -c 3 -i 0.3 -w
> 2 10.1.1.2 | FORMAT_PING
> OVS_TRAFFIC_VSWITCHD_STOP
> AT_CLEANUP
>
> +AT_SETUP([datapath - ping over active-backup bond])
This does not really represent the test case. What about ‘datapath - bond
active-backup failover’
> +OVS_TRAFFIC_VSWITCHD_START([_ADD_BR([br1])])
> +
> +AT_CHECK([ovs-ofctl add-flow br0 "priority=1 actions=drop"])
> +AT_CHECK([ovs-ofctl add-flow br1 "priority=1 actions=drop"])
> +AT_CHECK([ovs-ofctl add-flow br0 "ip,icmp actions=normal"])
> +AT_CHECK([ovs-ofctl add-flow br1 "ip,icmp actions=normal"])
> +AT_CHECK([ovs-ofctl add-flow br0 "arp actions=normal"])
> +AT_CHECK([ovs-ofctl add-flow br1 "arp actions=normal"])
Do we need this ‘complex’ set of rules, will the following not work?
AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"])
AT_CHECK([ovs-ofctl add-flow br1 "actions=normal"])
> +ADD_NAMESPACES(at_ns0, at_ns1)
> +
> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
> +ADD_VETH(p1, at_ns1, br1, "10.1.1.2/24")
> +on_exit 'ip link del link0a'
> +on_exit 'ip link del link0b'
> +AT_CHECK([ip link add link0a type veth peer name link1a])
> +AT_CHECK([ip link add link0b type veth peer name link1b])
> +
> +AT_CHECK([ip link set dev link0a up])
> +AT_CHECK([ip link set dev link1a up])
> +AT_CHECK([ip link set dev link0b up])
> +AT_CHECK([ip link set dev link1b up])
> +
> +AT_CHECK([ovs-vsctl add-bond br0 bond0 link0a link0b
> bond_mode=active-backup])
> +AT_CHECK([ovs-vsctl add-bond br1 bond1 link1a link1b
> bond_mode=active-backup])
> +
> +dnl Set primary
dnl Set the primary bond member.
> +AT_CHECK([ovs-vsctl set port bond0 other_config:bond-primary=link0a -- \
> +
Re: [ovs-dev] [PATCH v6 2/2] netdev-tc-offload: Add IPIP/GRE protocols to offload in ip rewrite.
On 7 Oct 2023, at 5:49, Faicker Mo wrote:
> Currently checksum recalculation is not supported with TC offload for
> IPIP and GRE packets. This patch adds support for TC offloading of
> IPIP and GRE packets by adding the correct csum action.
>
> Without this patch the following warning can be seen in the logging:
> Can't offload rewrite of IP/IPV6 with ip_proto: X.
>
> Fixes: d6118e628988 ("netdev-tc-offloads: Verify csum flags on dump from tc")
> Signed-off-by: Faicker Mo
Thanks for making the changes! The patch looks good to me now!
Acked-by: Eelco Chaudron
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v6 1/2] netdev-tc-offload: Add csum offload of IGMP/UDPLITE/SCTP in ip rewrite.
On 7 Oct 2023, at 5:49, Faicker Mo wrote: > When the IP header is modified, for example, by NAT or a ToS/TTL change, > the IP header checksum needs recalculation. In addition to the IP header > checksum, for UDPLITE, its checksum also needs recalculation when any > of the addresses change. > > This patch adds support for TC offloading of IGMP, UDPLITE, and SCTP > packets by adding the correct csum action. > > Signed-off-by: Faicker Mo Thanks for making the changes! The patch looks good to me now! Acked-by: Eelco Chaudron ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 2/3] tests/tunnel.at: Add geneve options mirror test.
On 9 Oct 2023, at 14:05, Roi Dayan wrote: > Test geneve options mirror flow doesn't add redundant mirror. > > Signed-off-by: Roi Dayan Thanks Roi for fixing the unit test. With this change ACK for the series! Acked-by: Eelco Chaudron ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v5] tests: Add some tests for byteq module.
On 7 Oct 2023, at 10:37, James Raphael Tiovalen wrote: > This commit adds a non-exhaustive list of tests for some of the > functions declared in `lib/byteq`. > > These unit tests have been executed via `make check` and they > successfully passed. > > Acked-by: Simon Horman > Signed-off-by: James Raphael Tiovalen Hi James, Thanks for working trough all the revisions and fixing my comments! I think this patch looks good now. Acked-by: Eelco Chaudron ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
