Re: [ovs-dev] [PATCH ovn] tests: Fix ssl-ciphers RO sb test with old openssl.
On Sat, Jul 6, 2024 at 4:21 PM Vladislav Odintsov wrote:
> The test "read-only sb db:pssl access with ssl-ciphers and ssl-protocols"
> fails when running with openssl which doesn't support some of passed
> values.
> For instance, on openssl 1.0.2 there is no support for 'SECLEVEL' and
> test fails due to extra string in stderr, which is asserted as a part of
> test:
>
> ./ovn.at:37851: ovn-sbctl --db=ssl:127.0.0.1:$TCP_PORT \
> --private-key=$PKIDIR/testpki-test-privkey.pem \
> --certificate=$PKIDIR/testpki-test-cert.pem \
> --ca-cert=$PKIDIR/testpki-cacert.pem \
> --ssl-ciphers='HIGH:!aNULL:!MD5:@SECLEVEL=1' \
> --ssl-protocols='TLSv1,TLSv1.1,TLSv1.2' \
> chassis-add ch vxlan 1.2.4.8
> --- - 2024-07-05 13:48:11.697647047 +0300
> +++
> /builddir/build/BUILD/ovn-24.03.90/tests/testsuite.dir/at-groups/520/stderr
> 2024-07-05 13:48:11.694353357 +0300
> @@ -1,2 +1,3 @@
> +2024-07-05T10:48:11Z|1|stream_ssl|ERR|SSL_CTX_set_cipher_list:
> error:140E6118:SSL routines:SSL_CIPHER_PROCESS_RULESTR:invalid command
>ovn-sbctl: transaction error: {"details":"insert operation not allowed
> when database server is in read only mode","error":"not allowed"}
>
> This patch fixes the test adding grep of expected transaction error.
>
> CC: Aliasgar Ginwala
> Fixes: 620203f9f0d9 ("Fix segfault due to ssl-ciphers.")
> Signed-off-by: Vladislav Odintsov
> ---
> tests/ovn.at | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 87a64499f..2341f52d5 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -37854,9 +37854,9 @@ AT_CHECK([ovn-sbctl --db=ssl:127.0.0.1:$TCP_PORT \
> --ca-cert=$PKIDIR/testpki-cacert.pem \
> --ssl-ciphers='HIGH:!aNULL:!MD5:@SECLEVEL=1' \
> --ssl-protocols='TLSv1,TLSv1.1,TLSv1.2' \
> -chassis-add ch vxlan 1.2.4.8], [1], [ignore],
> -[ovn-sbctl: transaction error: {"details":"insert operation not allowed
> when database server is in read only mode","error":"not allowed"}
> -])
> +chassis-add ch vxlan 1.2.4.8 2>&1 | grep 'transaction
> error]', [0], [dnl
> +ovn-sbctl: transaction error: {"details":"insert operation not allowed
> when database server is in read only mode","error":"not allowed"}
> +], [ignore])
>
> OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> AT_CLEANUP
> --
> 2.45.2
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Looks good to me, thanks.
Acked-by: Ales Musil
--
Ales Musil
Senior Software Engineer - OVN Core
Red Hat EMEA
amu...@redhat.com
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4 2/3] Userspace: Software fallback for UDP encapsulated TCP segmentation.
On Tue, Jul 9, 2024 at 5:57 PM Ilya Maximets wrote: > > On 7/5/24 22:44, Mike Pattrick wrote: > > When sending packets that are flagged as requiring segmentation to an > > interface that does not support this feature, send the packet to the TSO > > software fallback instead of dropping it. > > > > Signed-off-by: Mike Pattrick > > --- > > v2: > > - Fixed udp tunnel length > > - Added test that UDP headers are correct > > - Split inner and outer ip_id into different counters > > - Set tunnel flags in reset_tcp_seg > > > > v3: > > - Changed logic of netdev_send() to account for NICs that support > > tunnel offload but not checksum offload > > - Adjusted udp tunnel header length during software tso > > > > v4: > > - Moved a bugfix into its own patch > > - Fixed an indentation issue > > - Changed the scope of ip_hdr > > Signed-off-by: Mike Pattrick > > --- > > lib/dp-packet-gso.c | 90 - > > lib/dp-packet.h | 34 > > lib/netdev.c| 44 ++-- > > tests/system-traffic.at | 87 +++ > > 4 files changed, 214 insertions(+), 41 deletions(-) > > Hi, Mike. Not a full review, but see a few comments below. > > > diff --git a/tests/system-traffic.at b/tests/system-traffic.at > > index 3f1a15445..dd30b6bf5 100644 > > --- a/tests/system-traffic.at > > +++ b/tests/system-traffic.at > > @@ -351,6 +351,93 @@ OVS_WAIT_UNTIL([diff -q payload.bin udp_data]) > > OVS_TRAFFIC_VSWITCHD_STOP > > AT_CLEANUP > > > > +AT_SETUP([datapath - tcp over vxlan tunnel with software fallback]) > > +AT_SKIP_IF([test $HAVE_NC = no]) > > +AT_SKIP_IF([test $HAVE_TCPDUMP = no]) > > +OVS_CHECK_VXLAN() > > + > > +dnl This test is only valid with tso. If the kernel segments the packets, > > the > > +dnl packet lengths in the final test will be different. > > +m4_ifndef([CHECK_SYSTEM_TSO], [AT_SKIP_IF(:)]) > > + > > +OVS_TRAFFIC_VSWITCHD_START() > > +ADD_BR([br-underlay]) > > + > > +AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"]) > > +AT_CHECK([ovs-ofctl add-flow br-underlay "actions=normal"]) > > + > > +ADD_NAMESPACES(at_ns0) > > + > > +dnl Set up underlay link from host into the namespace using veth pair. > > +ADD_VETH(p0, at_ns0, br-underlay, "172.31.1.1/24") > > +AT_CHECK([ip addr add dev br-underlay "172.31.1.100/24"]) > > +AT_CHECK([ip link set dev br-underlay up]) > > + > > +dnl Test the case where only one side has all checksum and tso offload > > disabled. > > +AT_CHECK([ethtool -K ovs-p0 tso off], [0], [ignore], [ignore]) > > +AT_CHECK([ethtool -K ovs-p0 sg off], [0], [ignore], [ignore]) > > + > > +dnl Reinitialize. > > +AT_CHECK([ovs-vsctl del-port ovs-p0]) > > +AT_CHECK([ovs-vsctl add-port br-underlay ovs-p0]) > > + > > +dnl Set up tunnel endpoints on OVS outside the namespace and with a native > > +dnl linux device inside the namespace. > > +ADD_OVS_TUNNEL([vxlan], [br0], [at_vxlan0], [172.31.1.1], [10.1.1.100/24]) > > +ADD_NATIVE_TUNNEL([vxlan], [at_vxlan1], [at_ns0], [172.31.1.100], > > [10.1.1.1/24], > > + [id 0 dstport 4789]) > > + > > +dnl First, check the underlay. > > +NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -W 2 172.31.1.100 | > > FORMAT_PING], [0], [dnl > > +3 packets transmitted, 3 received, 0% packet loss, time 0ms > > +]) > > + > > +dnl Check that the tunnel is up. > > +NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -W 2 10.1.1.100 | > > FORMAT_PING], [0], [dnl > > +3 packets transmitted, 3 received, 0% packet loss, time 0ms > > +]) > > + > > +dnl Start tcpdump to capture the encapsulated packets. > > +OVS_DAEMONIZE([tcpdump -i ovs-p0 -w p0.pcap], [tcpdump.pid]) > > +sleep 1 > > Don't sleep - wait. The sleep here is to make sure that tcpdump has actually started listening for packets. I found sometimes in github ci the pcap would be missing the start of the transfer causing a failure. It's also used in the conntrack tests. > > + > > +dnl Initialize the listener before it is needed. > > +NETNS_DAEMONIZE([at_ns0], [nc -l 10.1.1.1 1234 > data2], [nc.pid]) > > + > > +dnl Verify that ncat is ready. > > +OVS_WAIT_UNTIL([NS_EXEC([at_ns0], [netstat -ln | grep :1234])]) > > + > > +dnl Large TCP transfer aimed towards ovs-p0, which has TSO disabled. > > +AT_CHECK([dd if=/dev/urandom of=payload.bin bs=6 count=1 2> /dev/null]) > > +AT_CHECK([nc $NC_EOF_OPT 10.1.1.1 1234 < payload.bin]) > > + > > +dnl Wait until transfer completes before checking. > > +OVS_WAIT_WHILE([kill -0 $(cat nc.pid)]) > > +AT_CHECK([diff -q payload.bin data2], [0]) > > + > > +dnl Stop OVS and tcpdump and verify the results. > > +OVS_TRAFFIC_VSWITCHD_STOP > > + > > +AT_CHECK([kill -15 $(cat tcpdump.pid)]) > > + > > +OVS_WAIT_WHILE([kill -0 $(cat tcpdump.pid)]) > > + > > +ovs-pcap p0.pcap > > + > > +dnl The exact number of packets sent will vary, but we check that the > > largest segments have the correct > > +dnl lengths and certain other fields. > > +AT_CHECK([test $(ovs-pcap p0.pcap | grep
Re: [ovs-dev] [PATCH ovn v4 1/1] Add support for overlay provider networks.
On Mon, Jul 8, 2024 at 5:32 PM Numan Siddique wrote:
>
> On Mon, Jul 8, 2024 at 2:54 AM Han Zhou wrote:
> >
> > On Thu, Jun 6, 2024 at 3:38 PM wrote:
> > >
> > > From: Numan Siddique
> > >
> > > It is expected that a provider network logical switch has a localnet
> > > logical
> > > switch port in order to bridge the overlay traffic to the underlay
> > > traffic.
> > > There can be some usecases where a overlay logical switch (without
> > > a localnet port) can act as a provider network and presently NAT doesn't
> > > work as expected. This patch adds this support. A new config option
> > > "overlay_provider_network" is added to support this feature.
> > > This feature gets enabled for a logical switch 'P' if:
> > > - The above option is set to true in the Logical_Switch.other_config
> > > column.
> > > - The logical switch 'P' doesn't have any localnet ports.
> > > - The logical router port of a router 'R' connecting to 'P'
> > > is a gateway router port.
> > > - And the logical router 'R' has only one gateway router port.
> > >
> > > If all the above conditions are met, ovn-northd creates a chassisredirect
> > > port for the logical switch port (of type router) connecting to the
> > > router 'R'. For example, if the logical port is named as "P-R" and its
> > > peer router port is "R-P", then chassisredirect port cr-P-R is created
> > > along with cr-R-P. Gateway chassis binding the cr-R-P also binds cr-P-R.
> > > This ensures that the routing is centralized on this gateway chassis for
> > > the traffic coming from switch "P" towards the router or vice versa.
> > > This centralization is required in order to support NAT (both SNAT and
> > > DNAT). Distributed NAT (i.e if external_mac and router_port is set) is
> > > not supported and instead the router port mac is used for such traffic.
> > >
> > > Reported-at: https://issues.redhat.com/browse/FDP-364
> > > Signed-off-by: Numan Siddique
> > > ---
> > > NEWS | 2 +
> > > controller/physical.c | 4 +
> > > northd/northd.c | 239 ++
> > > northd/northd.h | 1 +
> > > ovn-nb.xml| 27 ++
> > > tests/multinode-macros.at | 2 +-
> > > tests/multinode.at| 177 +
> > > tests/ovn-northd.at | 520 +-
> > > tests/ovn.at | 8 +-
> > > 9 files changed, 928 insertions(+), 52 deletions(-)
> > >
> > > diff --git a/NEWS b/NEWS
> > > index 3bdc551728..51e6eeabc9 100644
> > > --- a/NEWS
> > > +++ b/NEWS
> > > @@ -31,6 +31,8 @@ Post v24.03.0
> > > has been renamed to "options:ic-route-denylist" in order to comply
> > > with
> > > inclusive language guidelines. The previous name is still recognized
> > > to
> > > aid with backwards compatibility.
> > > + - Added Overlay provider network support to a logical switch if
> > > +the config "overlay_provider_network" is set to true.
> > >
> > > OVN v24.03.0 - 01 Mar 2024
> > > --
> > > diff --git a/controller/physical.c b/controller/physical.c
> > > index 25da789f0b..69a617f341 100644
> > > --- a/controller/physical.c
> > > +++ b/controller/physical.c
> > > @@ -1607,6 +1607,10 @@ consider_port_binding(struct ovsdb_idl_index
> > > *sbrec_port_binding_by_name,
> > > ct_zones);
> > > put_zones_ofpacts(&zone_ids, ofpacts_p);
> > >
> > > +/* Clear the MFF_INPORT. Its possible that the same packet
> > > may
> > > + * go out from the same tunnel inport. */
> > > +put_load(ofp_to_u16(OFPP_NONE), MFF_IN_PORT, 0, 16,
> > > ofpacts_p);
> > > +
> > > /* Resubmit to table 41. */
> > > put_resubmit(OFTABLE_CHECK_LOOPBACK, ofpacts_p);
> > > }
> > > diff --git a/northd/northd.c b/northd/northd.c
> > > index 9f81afccbb..2af9295a50 100644
> > > --- a/northd/northd.c
> > > +++ b/northd/northd.c
> > > @@ -2098,6 +2098,53 @@ parse_lsp_addrs(struct ovn_port *op)
> > > }
> > > }
> > >
> > > +static struct ovn_port *
> > > +create_cr_port(struct ovn_port *op, struct hmap *ports,
> > > + struct ovs_list *both_dbs, struct ovs_list *nb_only)
> > > +{
> > > +char *redirect_name = ovn_chassis_redirect_name(
> > > +op->nbsp ? op->nbsp->name : op->nbrp->name);
> > > +
> > > +struct ovn_port *crp = ovn_port_find(ports, redirect_name);
> > > +if (crp && crp->sb && crp->sb->datapath == op->od->sb) {
> > > +ovn_port_set_nb(crp, NULL, op->nbrp);
> > > +ovs_list_remove(&crp->list);
> > > +ovs_list_push_back(both_dbs, &crp->list);
> > > +} else {
> > > +crp = ovn_port_create(ports, redirect_name,
> > > + op->nbsp, op->nbrp, NULL);
> > > +ovs_list_push_back(nb_only, &crp->list);
> > > +}
> > > +
> > > +crp->primary_port = op;
> > > +op->cr_port = crp;
> > > +crp->od = op
[ovs-dev] [PATCH ovn v1] Add support for centralize routing for distributed gw ports.
From: Numan Siddique
Consider a deployment with the below logical resources:
1. A bridged logical switch 'public' with a port - P1 and a localnet
port ln-public.
2. A logical router 'R'
3. Logical switch 'public' connected to R via logical switch/router port
peers (public-R and R-public).
4. R-public is distributed gateway port with its network as 172.16.0.0/24
5. NATs (dnat_and_snat) configured in 'R'.
6. And a few overlay logical switches S1, S2 to R.
Any traffic from logical port - P1 of public logical switch destined to
S1 or S2's logical ports goes out of the source chassis
(where P1 resides) via the localnet port and reaches the gateway chassis
which handles the routing.
There are couple of traffic flow scenarios which doesn't work if the
logical switch 'public' doesn't have a localnet port.
1. Traffic from port - P1 destined to logical switches S1 or S2 gets
dropped in the source chassis. The packet enters the router R's
pipeline, but it gets dropped in the 'lr_in_admission' stage since
the logical flow to allow traffic destined to the distributed gateway
port MAC is installed only on the gateway chassis.
2. NAT doesn't work as expected.
In order to suppose this use case (of a logical switch not having a
localnet port, but has a distributed gateway port and NATs), this patch
supports the option 'centralize_routing', which can be configured on
the distributed gateway port (R-public in the example above).
If this option is set, then routing is centralized on the gateway
chassis for the traffic destined to the R-public's networks
(172.16.0.0/24 for the above example). Traffic from P1 will be
tunnelled to the gateway chassis.
ovn-northd creates a chassisresident port (cr-public-R) for the
logical switch port - public-R, along with cr-R-public inorder to
centralize the traffic.
This feature gets enabled for the distributed gateway port R-public if
- The above option is set to true in the R-public's options column.
- The logical switch 'public' doesn't have any localnet ports.
- And R-public is the only distributed gateway port of R.
Distributed NAT (i.e if external_mac and router_port is set) is
not supported and instead the router port mac is used for such traffic
and centralized on the gateway chassis.
Reported-at: https://issues.redhat.com/browse/FDP-364
Signed-off-by: Numan Siddique
---
NEWS | 2 +
controller/physical.c | 4 +
northd/northd.c | 257 +++
northd/northd.h | 1 +
ovn-nb.xml| 34 +++
tests/multinode-macros.at | 2 +-
tests/multinode.at| 177 +
tests/ovn-northd.at | 516 +-
tests/ovn.at | 8 +-
9 files changed, 951 insertions(+), 50 deletions(-)
diff --git a/NEWS b/NEWS
index 3e392ff08b..472445a188 100644
--- a/NEWS
+++ b/NEWS
@@ -38,6 +38,8 @@ Post v24.03.0
ability to disable "VXLAN mode" to extend available tunnel IDs space for
datapaths from 4095 to 16711680. For more details see man ovn-nb(5) for
mentioned option.
+ - Added Overlay provider network support to a logical switch if
+the config "overlay_provider_network" is set to true.
OVN v24.03.0 - 01 Mar 2024
--
diff --git a/controller/physical.c b/controller/physical.c
index 22756810fd..e3a316989a 100644
--- a/controller/physical.c
+++ b/controller/physical.c
@@ -1608,6 +1608,10 @@ consider_port_binding(struct ovsdb_idl_index
*sbrec_port_binding_by_name,
ct_zones);
put_zones_ofpacts(&zone_ids, ofpacts_p);
+/* Clear the MFF_INPORT. Its possible that the same packet may
+ * go out from the same tunnel inport. */
+put_load(ofp_to_u16(OFPP_NONE), MFF_IN_PORT, 0, 16, ofpacts_p);
+
/* Resubmit to table 41. */
put_resubmit(OFTABLE_CHECK_LOOPBACK, ofpacts_p);
}
diff --git a/northd/northd.c b/northd/northd.c
index 6898daa00d..9b52d5a3c0 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -2099,6 +2099,55 @@ parse_lsp_addrs(struct ovn_port *op)
}
}
+static struct ovn_port *
+create_cr_port(struct ovn_port *op, struct hmap *ports,
+ struct ovs_list *both_dbs, struct ovs_list *nb_only)
+{
+char *redirect_name = ovn_chassis_redirect_name(
+op->nbsp ? op->nbsp->name : op->nbrp->name);
+
+struct ovn_port *crp = ovn_port_find(ports, redirect_name);
+if (crp && crp->sb && crp->sb->datapath == op->od->sb) {
+ovn_port_set_nb(crp, NULL, op->nbrp);
+ovs_list_remove(&crp->list);
+ovs_list_push_back(both_dbs, &crp->list);
+} else {
+crp = ovn_port_create(ports, redirect_name,
+ op->nbsp, op->nbrp, NULL);
+ovs_list_push_back(nb_only, &crp->list);
+}
+
+crp->primary_port = op;
+op->cr_port = crp;
+crp->od = op->od;
+free(redirect_name);
+
Re: [ovs-dev] [PATCH v1 02/13] odp-util: Add support OVS_ACTION_ATTR_PSAMPLE.
On 7/9/24 21:07, Adrián Moreno wrote:
> On Tue, Jul 09, 2024 at 04:15:12PM GMT, Eelco Chaudron wrote:
>>
>>
>> On 9 Jul 2024, at 15:30, Adrián Moreno wrote:
>>
>>> On Tue, Jul 09, 2024 at 11:45:15AM GMT, Eelco Chaudron wrote:
On 4 Jul 2024, at 9:52, Adrian Moreno wrote:
> Add support for parsing and formatting the new action.
>
> Also, flag OVS_ACTION_ATTR_SAMPLE as requiring datapath assistance if it
> contains a nested OVS_ACTION_ATTR_PSAMPLE. The reason is that the
> sampling rate form the parent "sample" is made available to the nested
form -> from
> "psample" by the kernel.
Two small comments below, the rest looks good.
> Signed-off-by: Adrian Moreno
> ---
> include/linux/openvswitch.h | 28 +++
> lib/dpif-netdev.c| 1 +
> lib/dpif.c | 1 +
> lib/odp-execute.c| 25 +-
> lib/odp-util.c | 93
> lib/odp-util.h | 3 ++
> ofproto/ofproto-dpif-ipfix.c | 1 +
> ofproto/ofproto-dpif-sflow.c | 1 +
> python/ovs/flow/odp.py | 8
> tests/odp.at | 16 +++
> 10 files changed, 176 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
> index d9fb991ef..0023b65fb 100644
> --- a/include/linux/openvswitch.h
> +++ b/include/linux/openvswitch.h
> @@ -992,6 +992,31 @@ struct check_pkt_len_arg {
> };
> #endif
>
> +#define OVS_PSAMPLE_COOKIE_MAX_SIZE 16
> +/**
> + * enum ovs_pample_attr - Attributes for %OVS_ACTION_ATTR_PSAMPLE
> + * action.
> + *
> + * @OVS_PSAMPLE_ATTR_GROUP: 32-bit number to identify the source of the
> + * sample.
> + * @OVS_PSAMPLE_ATTR_COOKIE: An optional variable-length binary cookie
> that
> + * contains user-defined metadata. The maximum length is
> + * OVS_PSAMPLE_COOKIE_MAX_SIZE bytes.
> + *
> + * Sends the packet to the psample multicast group with the specified
> group and
> + * cookie. It is possible to combine this action with the
> + * %OVS_ACTION_ATTR_TRUNC action to limit the size of the sample.
> + */
> +enum ovs_psample_attr {
> +OVS_PSAMPLE_ATTR_GROUP = 1,/* u32 number. */
> +OVS_PSAMPLE_ATTR_COOKIE, /* Optional, user specified
> cookie. */
> +
> +/* private: */
None of the other definitions have this private marking, so I see no need
to
start adding it here.
>>>
>>> OK. The uAPI file has it (requested by netdev maintainers) but I guess
>>> it's kernel-specific and this file is not a blind copy of the uAPI
>>> anyways so I think we can remove it. I'll do on the next version.
I'd keep it. There is no point in having this header different from
the kernel one in places where it doesn't surve any purpose.
>>>
> +__OVS_PSAMPLE_ATTR_MAX
> +};
> +
> +#define OVS_PSAMPLE_ATTR_MAX (__OVS_PSAMPLE_ATTR_MAX - 1)
> +
> /**
> * enum ovs_action_attr - Action types.
> *
> @@ -1056,6 +1081,8 @@ struct check_pkt_len_arg {
> * of l3 tunnel flag in the tun_flags field of OVS_ACTION_ATTR_ADD_MPLS
> * argument.
> * @OVS_ACTION_ATTR_DROP: Explicit drop action.
> + * @OVS_ACTION_ATTR_PSAMPLE: Send a sample of the packet to external
> observers
> + * via psample.
> */
>
> enum ovs_action_attr {
> @@ -1087,6 +1114,7 @@ enum ovs_action_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. */
> + OVS_ACTION_ATTR_PSAMPLE, /* Nested OVS_PSAMPLE_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 c7f9e1490..f0594e5f5 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -9519,6 +9519,7 @@ dp_execute_cb(void *aux_, struct dp_packet_batch
> *packets_,
> case OVS_ACTION_ATTR_DROP:
> case OVS_ACTION_ATTR_ADD_MPLS:
> case OVS_ACTION_ATTR_DEC_TTL:
> +case OVS_ACTION_ATTR_PSAMPLE:
> case __OVS_ACTION_ATTR_MAX:
> OVS_NOT_REACHED();
> }
> diff --git a/lib/dpif.c b/lib/dpif.c
> index 23eb18495..71728badc 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -1192,6 +1192,7 @@ dpif_execute_helper_cb(void *aux_, struct
> dp_packet_batch *packets_,
> case OVS_ACTION_ATTR_TUNNEL_PUSH:
> case OVS_ACTION_ATTR_TUNNEL_POP:
> case OVS_ACTION_ATTR_USERSPACE:
> +case OVS_ACTION_ATTR_PSAMPLE:
> case OVS_ACTION_ATTR_RECIRC: {
> struct dpif_execute execute;
>
Re: [ovs-dev] [PATCH v1 01/13] ofproto-dpif: Allow forcing dp features.
On Wed, Jul 10, 2024 at 12:13:37AM GMT, Ilya Maximets wrote:
> On 7/7/24 22:08, Adrian Moreno wrote:
> > Datapath features can be set with dpif/set-dp-features unixctl command.
> > This command is not docummented and therefore not supported in
> > production but only useful for unit tests.
> >
> > A limitation was put in place originally to avoid enabling features at
> > runtime that were disabled at boot time to avoid breaking the datapath
> > in unexpected ways.
> >
> > But, considering users should not use this command and it should only be
> > used for testing, we can assume whoever runs it knows what they are
> > doing. Therefore, the limitation should be bypass-able.
> >
> > This patch adds a "--force" flag to the unixctl command to allow
> > bypassing the mentioned limitation.
> >
> > Signed-off-by: Adrian Moreno
> > ---
> > ofproto/ofproto-dpif.c | 37 +++--
> > 1 file changed, 27 insertions(+), 10 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index fcd7cd753..4712d10a8 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -6432,7 +6432,8 @@ display_support_field(const char *name,
> > static bool
> > dpif_set_support(struct dpif_backer_support *rt_support,
> > struct dpif_backer_support *bt_support,
> > - const char *name, const char *value, struct ds *ds)
> > + const char *name, const char *value, bool force,
> > + struct ds *ds)
> > {
> > struct shash all_fields = SHASH_INITIALIZER(&all_fields);
> > struct dpif_support_field *field;
> > @@ -6484,8 +6485,8 @@ dpif_set_support(struct dpif_backer_support
> > *rt_support,
> >
> > if (field->type == DPIF_SUPPORT_FIELD_bool) {
> > if (!strcasecmp(value, "true")) {
> > -if (*(bool *)field->bt_ptr) {
> > -*(bool *)field->rt_ptr = true;
> > +if (*(bool *) field->bt_ptr || force) {
> > +*(bool *) field->rt_ptr = true;
>
> We discussed having a scary warning message for the force option.
> I don't see it here.
>
You're right. I forgot. Thanks.
> > changed = true;
> > } else {
> > ds_put_cstr(ds, "Can not enable features not supported by
> > the datapth");
> > @@ -6720,21 +6721,36 @@ ofproto_unixctl_dpif_set_dp_features(struct
> > unixctl_conn *conn,
> > void *aux OVS_UNUSED)
> > {
> > struct ds ds = DS_EMPTY_INITIALIZER;
> > -const char *br = argv[1];
> > +struct ofproto_dpif *ofproto;
> > +bool changed, force = false;
> > const char *name, *value;
> > -struct ofproto_dpif *ofproto = ofproto_dpif_lookup_by_name(br);
> > -bool changed;
> > +const char *br = NULL;
> > +
> > +if (!strcmp(argv[1], "--force")) {
> > +force = true;
> > +if (argc > 2) {
> > +br = argv[2];
> > +name = argc > 3 ? argv[3] : NULL;
> > +value = argc > 4 ? argv[4] : NULL;
> > +} else {
> > +unixctl_command_reply_error(conn, "bridge not specified");
> > +return;
> > +}
> > +} else {
> > +br = argv[1];
> > +name = argc > 2 ? argv[2] : NULL;
> > +value = argc > 3 ? argv[3] : NULL;
> > +}
>
> It feels like this can be cleaner if we just eat the --force,
> argv++, argc-- and continue the processing instead of doing
> the same thing with diferent indexes in different branches.
>
Thanks for the suggestion. It does seem to yield cleaner code.
> >
> > +ofproto = ofproto_dpif_lookup_by_name(br);
> > if (!ofproto) {
> > unixctl_command_reply_error(conn, "no such bridge");
> > return;
> > }
> >
> > -name = argc > 2 ? argv[2] : NULL;
> > -value = argc > 3 ? argv[3] : NULL;
> > changed = dpif_set_support(&ofproto->backer->rt_support,
> > &ofproto->backer->bt_support,
> > - name, value, &ds);
> > + name, value, force, &ds);
> > if (changed) {
> > xlate_set_support(ofproto, &ofproto->backer->rt_support);
> > udpif_flush(ofproto->backer->udpif);
> > @@ -6777,7 +6793,8 @@ ofproto_unixctl_init(void)
> > unixctl_command_register("dpif/dump-flows",
> > "[-m] [--names | --no-names] bridge", 1,
> > INT_MAX,
> > ofproto_unixctl_dpif_dump_flows, NULL);
> > -unixctl_command_register("dpif/set-dp-features", "bridge", 1, 3 ,
> > +unixctl_command_register("dpif/set-dp-features",
> > + "[--force] bridge [feature] [value]", 1, 4 ,
>
> There is an extra space after '4'. It was there before, but it's
> a good time to remove it.
>
> Also, [feature [value]] .
Ack.
>
> > ofproto_unixctl_dpif_set_dp_features, NULL);
> > }
> >
>
__
Re: [ovs-dev] [PATCH v1 01/13] ofproto-dpif: Allow forcing dp features.
On 7/7/24 22:08, Adrian Moreno wrote:
> Datapath features can be set with dpif/set-dp-features unixctl command.
> This command is not docummented and therefore not supported in
> production but only useful for unit tests.
>
> A limitation was put in place originally to avoid enabling features at
> runtime that were disabled at boot time to avoid breaking the datapath
> in unexpected ways.
>
> But, considering users should not use this command and it should only be
> used for testing, we can assume whoever runs it knows what they are
> doing. Therefore, the limitation should be bypass-able.
>
> This patch adds a "--force" flag to the unixctl command to allow
> bypassing the mentioned limitation.
>
> Signed-off-by: Adrian Moreno
> ---
> ofproto/ofproto-dpif.c | 37 +++--
> 1 file changed, 27 insertions(+), 10 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index fcd7cd753..4712d10a8 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -6432,7 +6432,8 @@ display_support_field(const char *name,
> static bool
> dpif_set_support(struct dpif_backer_support *rt_support,
> struct dpif_backer_support *bt_support,
> - const char *name, const char *value, struct ds *ds)
> + const char *name, const char *value, bool force,
> + struct ds *ds)
> {
> struct shash all_fields = SHASH_INITIALIZER(&all_fields);
> struct dpif_support_field *field;
> @@ -6484,8 +6485,8 @@ dpif_set_support(struct dpif_backer_support *rt_support,
>
> if (field->type == DPIF_SUPPORT_FIELD_bool) {
> if (!strcasecmp(value, "true")) {
> -if (*(bool *)field->bt_ptr) {
> -*(bool *)field->rt_ptr = true;
> +if (*(bool *) field->bt_ptr || force) {
> +*(bool *) field->rt_ptr = true;
We discussed having a scary warning message for the force option.
I don't see it here.
> changed = true;
> } else {
> ds_put_cstr(ds, "Can not enable features not supported by
> the datapth");
> @@ -6720,21 +6721,36 @@ ofproto_unixctl_dpif_set_dp_features(struct
> unixctl_conn *conn,
> void *aux OVS_UNUSED)
> {
> struct ds ds = DS_EMPTY_INITIALIZER;
> -const char *br = argv[1];
> +struct ofproto_dpif *ofproto;
> +bool changed, force = false;
> const char *name, *value;
> -struct ofproto_dpif *ofproto = ofproto_dpif_lookup_by_name(br);
> -bool changed;
> +const char *br = NULL;
> +
> +if (!strcmp(argv[1], "--force")) {
> +force = true;
> +if (argc > 2) {
> +br = argv[2];
> +name = argc > 3 ? argv[3] : NULL;
> +value = argc > 4 ? argv[4] : NULL;
> +} else {
> +unixctl_command_reply_error(conn, "bridge not specified");
> +return;
> +}
> +} else {
> +br = argv[1];
> +name = argc > 2 ? argv[2] : NULL;
> +value = argc > 3 ? argv[3] : NULL;
> +}
It feels like this can be cleaner if we just eat the --force,
argv++, argc-- and continue the processing instead of doing
the same thing with diferent indexes in different branches.
>
> +ofproto = ofproto_dpif_lookup_by_name(br);
> if (!ofproto) {
> unixctl_command_reply_error(conn, "no such bridge");
> return;
> }
>
> -name = argc > 2 ? argv[2] : NULL;
> -value = argc > 3 ? argv[3] : NULL;
> changed = dpif_set_support(&ofproto->backer->rt_support,
> &ofproto->backer->bt_support,
> - name, value, &ds);
> + name, value, force, &ds);
> if (changed) {
> xlate_set_support(ofproto, &ofproto->backer->rt_support);
> udpif_flush(ofproto->backer->udpif);
> @@ -6777,7 +6793,8 @@ ofproto_unixctl_init(void)
> unixctl_command_register("dpif/dump-flows",
> "[-m] [--names | --no-names] bridge", 1,
> INT_MAX,
> ofproto_unixctl_dpif_dump_flows, NULL);
> -unixctl_command_register("dpif/set-dp-features", "bridge", 1, 3 ,
> +unixctl_command_register("dpif/set-dp-features",
> + "[--force] bridge [feature] [value]", 1, 4 ,
There is an extra space after '4'. It was there before, but it's
a good time to remove it.
Also, [feature [value]] .
> ofproto_unixctl_dpif_set_dp_features, NULL);
> }
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4 3/3] netdev-dpdk: Re-enable VXLAN/Geneve offload for Intel cards.
On 7/5/24 22:44, Mike Pattrick wrote: > Previously the OVS support for checksum/TSO offloading didn't work well > with some network cards that supported VXLAN/Geneve tunnel TSO but not > outer UDP checksums. Now support for this configuration is improved and Could you, please, point out how exactly it is improved or add a link to the changes you're referring to? Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4 2/3] Userspace: Software fallback for UDP encapsulated TCP segmentation.
On 7/5/24 22:44, Mike Pattrick wrote:
> When sending packets that are flagged as requiring segmentation to an
> interface that does not support this feature, send the packet to the TSO
> software fallback instead of dropping it.
>
> Signed-off-by: Mike Pattrick
> ---
> v2:
> - Fixed udp tunnel length
> - Added test that UDP headers are correct
> - Split inner and outer ip_id into different counters
> - Set tunnel flags in reset_tcp_seg
>
> v3:
> - Changed logic of netdev_send() to account for NICs that support
> tunnel offload but not checksum offload
> - Adjusted udp tunnel header length during software tso
>
> v4:
> - Moved a bugfix into its own patch
> - Fixed an indentation issue
> - Changed the scope of ip_hdr
> Signed-off-by: Mike Pattrick
> ---
> lib/dp-packet-gso.c | 90 -
> lib/dp-packet.h | 34
> lib/netdev.c| 44 ++--
> tests/system-traffic.at | 87 +++
> 4 files changed, 214 insertions(+), 41 deletions(-)
Hi, Mike. Not a full review, but see a few comments below.
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index 3f1a15445..dd30b6bf5 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -351,6 +351,93 @@ OVS_WAIT_UNTIL([diff -q payload.bin udp_data])
> OVS_TRAFFIC_VSWITCHD_STOP
> AT_CLEANUP
>
> +AT_SETUP([datapath - tcp over vxlan tunnel with software fallback])
> +AT_SKIP_IF([test $HAVE_NC = no])
> +AT_SKIP_IF([test $HAVE_TCPDUMP = no])
> +OVS_CHECK_VXLAN()
> +
> +dnl This test is only valid with tso. If the kernel segments the packets, the
> +dnl packet lengths in the final test will be different.
> +m4_ifndef([CHECK_SYSTEM_TSO], [AT_SKIP_IF(:)])
> +
> +OVS_TRAFFIC_VSWITCHD_START()
> +ADD_BR([br-underlay])
> +
> +AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"])
> +AT_CHECK([ovs-ofctl add-flow br-underlay "actions=normal"])
> +
> +ADD_NAMESPACES(at_ns0)
> +
> +dnl Set up underlay link from host into the namespace using veth pair.
> +ADD_VETH(p0, at_ns0, br-underlay, "172.31.1.1/24")
> +AT_CHECK([ip addr add dev br-underlay "172.31.1.100/24"])
> +AT_CHECK([ip link set dev br-underlay up])
> +
> +dnl Test the case where only one side has all checksum and tso offload
> disabled.
> +AT_CHECK([ethtool -K ovs-p0 tso off], [0], [ignore], [ignore])
> +AT_CHECK([ethtool -K ovs-p0 sg off], [0], [ignore], [ignore])
> +
> +dnl Reinitialize.
> +AT_CHECK([ovs-vsctl del-port ovs-p0])
> +AT_CHECK([ovs-vsctl add-port br-underlay ovs-p0])
> +
> +dnl Set up tunnel endpoints on OVS outside the namespace and with a native
> +dnl linux device inside the namespace.
> +ADD_OVS_TUNNEL([vxlan], [br0], [at_vxlan0], [172.31.1.1], [10.1.1.100/24])
> +ADD_NATIVE_TUNNEL([vxlan], [at_vxlan1], [at_ns0], [172.31.1.100],
> [10.1.1.1/24],
> + [id 0 dstport 4789])
> +
> +dnl First, check the underlay.
> +NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -W 2 172.31.1.100 |
> FORMAT_PING], [0], [dnl
> +3 packets transmitted, 3 received, 0% packet loss, time 0ms
> +])
> +
> +dnl Check that the tunnel is up.
> +NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -W 2 10.1.1.100 | FORMAT_PING],
> [0], [dnl
> +3 packets transmitted, 3 received, 0% packet loss, time 0ms
> +])
> +
> +dnl Start tcpdump to capture the encapsulated packets.
> +OVS_DAEMONIZE([tcpdump -i ovs-p0 -w p0.pcap], [tcpdump.pid])
> +sleep 1
Don't sleep - wait.
> +
> +dnl Initialize the listener before it is needed.
> +NETNS_DAEMONIZE([at_ns0], [nc -l 10.1.1.1 1234 > data2], [nc.pid])
> +
> +dnl Verify that ncat is ready.
> +OVS_WAIT_UNTIL([NS_EXEC([at_ns0], [netstat -ln | grep :1234])])
> +
> +dnl Large TCP transfer aimed towards ovs-p0, which has TSO disabled.
> +AT_CHECK([dd if=/dev/urandom of=payload.bin bs=6 count=1 2> /dev/null])
> +AT_CHECK([nc $NC_EOF_OPT 10.1.1.1 1234 < payload.bin])
> +
> +dnl Wait until transfer completes before checking.
> +OVS_WAIT_WHILE([kill -0 $(cat nc.pid)])
> +AT_CHECK([diff -q payload.bin data2], [0])
> +
> +dnl Stop OVS and tcpdump and verify the results.
> +OVS_TRAFFIC_VSWITCHD_STOP
> +
> +AT_CHECK([kill -15 $(cat tcpdump.pid)])
> +
> +OVS_WAIT_WHILE([kill -0 $(cat tcpdump.pid)])
> +
> +ovs-pcap p0.pcap
> +
> +dnl The exact number of packets sent will vary, but we check that the
> largest segments have the correct
> +dnl lengths and certain other fields.
> +AT_CHECK([test $(ovs-pcap p0.pcap | grep -Ec dnl
> +"^.{24}0800"dnl Ethernet
> +"450005aa4000..11ac1f0164ac1f0101"dnl IP(len=1450, DF, UDP,
> 172.31.1.100->172.31.1.1)
> +"12b50596"dnl UDP(len=1430, dport=4789)
> +"0800"dnl VXLAN(gpid=0, vni=0)
> +".{24}0800"dnl Ethernet
> +"450005784000..060a0101640a010101"dnl IP(len=1400, DF, TCP,
> 10.1.1.100->10.1.1.1)
> +"04d2"dnl TCP(dport=1234
> +) -ge 20])
> +
> +AT_CLEANUP
> +
> AT_SETUP([datapath - ping vlan over vxlan tunnel])
> OVS_CHECK_TUNN
Re: [ovs-dev] [PATCH v2 1/1] ofproto-dpif-upcall: Avoid stale ukeys leaks.
On 7/3/24 12:28, Roi Dayan via dev wrote: > > > On 03/07/2024 13:16, Eelco Chaudron wrote: >> >> >> On 3 Jul 2024, at 9:33, Roi Dayan wrote: >> >>> It is observed in some environments that there are much more ukeys than >>> actual DP flows. For example: >>> >>> $ ovs-appctl upcall/show >>> system@ovs-system: >>> flows : (current 7) (avg 6) (max 117) (limit 2125) >>> offloaded flows : 525 >>> dump duration : 1063ms >>> ufid enabled : true >>> >>> 23: (keys 3612) >>> 24: (keys 3625) >>> 25: (keys 3485) >>> >>> The revalidator threads are busy revalidating the stale ukeys leading to >>> high CPU and long dump duration. >>> >>> This patch adds checks in the sweep phase for such ukeys and move them >>> to DELETE so that they can be cleared eventually. >> >> Thank Roi for the patch update, one small issue below. Let’s also discuss a >> bit more a potential testcase before sending a v3. >> >> Cheers, >> >> Eelco >> >> > > I also replied in v0 but replying here so we can continue the discussion here > in v2. > > I did this for testing this case: > > - create bridge with 2 veth ports. configure ips. > - ping between the ports to have tc rules. > - repeated few times: clear the tc rules like this: tc filter del dev veth1 > ingress and also on the 2nd port. > - set max-idle to 1 and remove it to cause a flush of the rules. > - create another set of veth ports. add/del veth4 from the bridge a few times > to cause a sweep. > - before the fix: ovs-appctl upcall/show will show ukeys. > - after the fix upcall/show will show 0 ukeys. > > > what do you think? > I think if there is a cleaner way to do a sweep with purge=false as just > will just skip seq mismatch check and just set every ukey to delete. One problem here is that flow dumps are inherently racy. We can dump the same flow multiple times as well as not receive some flows that are actually in the datapath. So, we can't just remove ukeys if we missed a flow once or twice. If we didn't see the flow in the dumps for the max-idle interval, that might be a better indicator that something is wrong. One more problem here however is that ukey statistics can keep increasing if there is some traffic that matches. Since the datapath flow doesn't exist, all this traffic will go to userspace updating the ukey stats, but the flow will never be installed again. And we will not be able to fix that in this patch since the stats will grow. We need a way to tell that we haven't seen this flow in the dumps for a long time and stats are not a good indicator in this case. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v14 0/6] Add global option to output JSON from ovs-appctl cmds.
On 7/9/24 09:14, jm...@redhat.com wrote: > From: Jakob Meng > > v14 addresses all comments from Ilya Maximets for v13 [0]. Aside from > polishing, appctl.py's "--format" arg will be case-insensitive, allowing > 'json' and 'JSON' etc. > > Kudos to Ilya for holding up code quality in Open vSwitch ❤️ > > Best regards, > Jakob > > [0] > https://patchwork.ozlabs.org/project/openvswitch/list/?series=413880&state=%2A&archive=both > > Jakob Meng (6): > Add global option for JSON output to ovs-appctl. > python: Add option for JSON output to unixctl classes and appctl.py. > appctl: Add option '--pretty' for pretty-printing JSON output. > python: Add option for pretty-printing JSON output to appctl.py. > vswitchd: Add JSON output for 'list-commands' command. > ofproto: Add JSON output for 'dpif/show' command. > > Documentation/ref/ovs-appctl.8.rst | 20 +++ > NEWS | 7 + > lib/unixctl.c | 224 + > lib/unixctl.h | 17 ++- > lib/util.c | 6 +- > ofproto/ofproto-dpif.c | 126 ++-- > python/ovs/unixctl/__init__.py | 8 ++ > python/ovs/unixctl/client.py | 5 +- > python/ovs/unixctl/server.py | 56 ++-- > tests/appctl.py| 43 +- > tests/ofproto-dpif.at | 40 ++ > tests/ovs-vswitchd.at | 33 + > tests/unixctl-py.at| 13 ++ > utilities/ovs-appctl.c | 157 +--- > 14 files changed, 647 insertions(+), 108 deletions(-) > Thanks, Jakob! Applied. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [ovs-dev, v8] [PATCH]ipf: Fix ovs ipf crash.
On Tue, Jul 9, 2024 at 1:49 AM 赖香武 <15310488...@163.com> wrote: > > hi, llya and Michael > > Can you take the time to take a look at my patch? Thank you. Hello, I wasn't able to reproduce the crash using your method and unfortunately the stack trace appears to be missing debug symbols. Looking at the code, I can guess at some ways that would trigger the crash. Instead of changing the current behaviour on expiration I think it would be better to just check last_sent_idx before accessing frag_list[].pkt. If last_sent_idx != IPF_INVALID_IDX, then functions like ipf_post_execute_reass_pkts(), ipf_process_frag(), etc can just return immediately. Thanks, M > > At 2024-05-28 23:16:46, "Ilya Maximets" wrote: > >On 5/23/24 09:40, laixiangwu wrote: > >> Description: > >> > >> when 1) The fragment timeout is between 15 seconds and 25 seconds; 2) > >> The ipf_list currently has received more than 32 fragments, and there > >> are other fragments of same big packet that have not been received. > >> > >> When the above two scenario conditions are met, due to exceeding the > >> capacity of the packet batch(here is 32), ipf_dp_packet_batch_add > >> returns false, and ipf_list will not be cleared. However, the 32 > >> fragments packets added to the packet batch will be processed normally. > >> When receiving the subsequent fragments of the ipf_list, because the > >> first 32 fragments have been processed, when processing subsequent > >> fragment packets, relevant information about the processed fragment > >> packets will be read，therefore will occur carsh. > >> One solution is do not forward timeout fragment packets from the above > >> scenarios, that is, do not add them to the packet batch, and handle > >> other scenarios according to the original logic. > >> Signed-off-by: laixiangwu <15310488...@163.com> > >> --- > >> lib/ipf.c | 10 -- > >> 1 file changed, 4 insertions(+), 6 deletions(-) > > > >Hi, laixiangwu. This version of the patch looks the same as the > >previous one here: > > > > https://patchwork.ozlabs.org/project/openvswitch/patch/20240522021957.2292-1-15310488...@163.com/ > > > >And I see Mike asked a few questions for the approach there. > >Could you, please, answer those? > > > >For now, I'll mark this patch with 'Changes Requested'. > > > >If you plan to send a new version based on Mike's comments, please, add > >'v6' to the subject prefix, i.e. [PATCH v6], since it's technically a > >6th version of it. > > > >Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1 12/13] ofproto: xlate: Make bridge-sampled drops explicit.
On Tue, Jul 09, 2024 at 11:46:09AM GMT, Eelco Chaudron wrote:
> On 7 Jul 2024, at 22:09, Adrian Moreno wrote:
>
> > The decision to add or not the explicit drop action is currently based
> > on whether the resulting dp action list is empty or not.
> >
> > This is OK for most of the cases but if per-flow IPFIX/sFlow sampling
> > is enabled on the bridge, it doesn't work as expected.
> >
> > Fix this by taking into account the size of these sampling actions.
>
> Same comments as on the previous patch.
>
> > Fixes: a13a0209750c ("userspace: Improved packet drop statistics.")
> > Cc: anju.tho...@ericsson.com
> > Signed-off-by: Adrian Moreno
> > ---
> > ofproto/ofproto-dpif-xlate.c | 5 ++--
> > tests/drop-stats.at | 44
> > tests/ofproto-dpif.at| 4 ++--
> > 3 files changed, 49 insertions(+), 4 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> > index 094fe5d72..323a58cbf 100644
> > --- a/ofproto/ofproto-dpif-xlate.c
> > +++ b/ofproto/ofproto-dpif-xlate.c
> > @@ -8153,6 +8153,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out
> > *xout)
> > uint64_t action_set_stub[1024 / 8];
> > uint64_t frozen_actions_stub[1024 / 8];
> > uint64_t actions_stub[256 / 8];
> > +size_t sample_actions_len = 0;
> > struct ofpbuf scratch_actions = OFPBUF_STUB_INITIALIZER(actions_stub);
> > struct xlate_ctx ctx = {
> > .xin = xin,
> > @@ -8412,7 +8413,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out
> > *xout)
> > user_cookie_offset = compose_sflow_action(&ctx);
> > compose_ipfix_action(&ctx, ODPP_NONE);
> > }
> > -size_t sample_actions_len = ctx.odp_actions->size;
> > +sample_actions_len = ctx.odp_actions->size;
> > bool ecn_drop = !tnl_process_ecn(flow);
> >
> > if (!ecn_drop
> > @@ -8575,7 +8576,7 @@ exit:
> > }
> >
> > /* Install drop action if datapath supports explicit drop action. */
> > -if (xin->odp_actions && !xin->odp_actions->size &&
> > +if (xin->odp_actions && xin->odp_actions->size == sample_actions_len &&
> > ovs_explicit_drop_action_supported(ctx.xbridge->ofproto)) {
> > put_drop_action(xin->odp_actions, ctx.error);
>
> See other patch, do we need ctx.error here?
Here we *do* need it.
This is where it was originally placed to flag xlate errors. We are at
the end of xlate_actions main function and the one place where we have
gathered ctx.error.
The change here is not to modify how the drop is added to the action
list. What this change does is change the condition upon which it's
added. Before we were checking if the action list was empty under the
assumption that if do_xlate_actions does not fill in any action, it's a
drop.
However, if per-bridge IPFIX (or sFlow) is enabled, their corresponding
actions are added _before_ calling do_xlate_actions. That's why this
patch only makes the check compare the size of the actions with the one
it had before calling do_xlate_actions.
Calling do_xlate_actions can yield errors and we should definitely flag
them.
>
> > }
> > diff --git a/tests/drop-stats.at b/tests/drop-stats.at
> > index 44c5cc35b..31782ac52 100644
> > --- a/tests/drop-stats.at
> > +++ b/tests/drop-stats.at
> > @@ -192,6 +192,50 @@ ovs-appctl coverage/read-counter
> > drop_action_too_many_mpls_labels
> > OVS_VSWITCHD_STOP(["/|WARN|/d"])
> > AT_CLEANUP
> >
> > +AT_SETUP([drop-stats - sampling])
>
> Should this maybe be 'bridge sampling'?
>
Ack.
> > +
> > +OVS_VSWITCHD_START([dnl
> > +set bridge br0 datapath_type=dummy \
> > +protocols=OpenFlow10,OpenFlow13,OpenFlow14,OpenFlow15 -- \
> > +add-port br0 p1 -- set Interface p1 type=dummy ofport_request=1])
> > +
> > +AT_DATA([flows.txt], [dnl
> > +table=0,in_port=1,actions=drop
> > +])
> > +
> > +AT_CHECK([
> > +ovs-ofctl del-flows br0
> > +ovs-ofctl -Oopenflow13 add-flows br0 flows.txt
> > +ovs-ofctl -Oopenflow13 dump-flows br0 | ofctl_strip | sort | grep
> > actions ], [0], [dnl
> > + in_port=1 actions=drop
> > +])
> > +
> > +AT_CHECK([ovs-vsctl -- set bridge br0 ipfix=@fix -- \
> > +--id=@fix create ipfix targets=\"127.0.0.1:4739\" \
> > + sampling=1],
> > + [0], [ignore])
> > +
> > +AT_CHECK([
> > +ovs-appctl netdev-dummy/receive p1
> > 'in_port(1),packet_type(ns=0,id=0),eth(src=3a:6d:d2:09:9c:ab,dst=1e:2c:e9:2a:66:9e),ipv4(src=192.168.10.10,dst=192.168.10.30,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'
> > +ovs-appctl netdev-dummy/receive p1
> > 'in_port(1),packet_type(ns=0,id=0),eth(src=3a:6d:d2:09:9c:ab,dst=1e:2c:e9:2a:66:9e),ipv4(src=192.168.10.10,dst=192.168.10.30,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'
> > +ovs-appctl netdev-dummy/receive p1
> > 'in_port(1),packet_type(ns=0,id=0),eth(src=3a:6d:d2:09:9c:ab,dst=1e:2c:e9:2a:66:9e),ipv4(src=192.168.10.10,dst=192.168.10.30,
Re: [ovs-dev] [PATCH v1 02/13] odp-util: Add support OVS_ACTION_ATTR_PSAMPLE.
On Tue, Jul 09, 2024 at 04:15:12PM GMT, Eelco Chaudron wrote:
>
>
> On 9 Jul 2024, at 15:30, Adrián Moreno wrote:
>
> > On Tue, Jul 09, 2024 at 11:45:15AM GMT, Eelco Chaudron wrote:
> >> On 4 Jul 2024, at 9:52, Adrian Moreno wrote:
> >>
> >>> Add support for parsing and formatting the new action.
> >>>
> >>> Also, flag OVS_ACTION_ATTR_SAMPLE as requiring datapath assistance if it
> >>> contains a nested OVS_ACTION_ATTR_PSAMPLE. The reason is that the
> >>> sampling rate form the parent "sample" is made available to the nested
> >>
> >> form -> from
> >>
> >>> "psample" by the kernel.
> >>
> >> Two small comments below, the rest looks good.
> >>
> >>> Signed-off-by: Adrian Moreno
> >>> ---
> >>> include/linux/openvswitch.h | 28 +++
> >>> lib/dpif-netdev.c| 1 +
> >>> lib/dpif.c | 1 +
> >>> lib/odp-execute.c| 25 +-
> >>> lib/odp-util.c | 93
> >>> lib/odp-util.h | 3 ++
> >>> ofproto/ofproto-dpif-ipfix.c | 1 +
> >>> ofproto/ofproto-dpif-sflow.c | 1 +
> >>> python/ovs/flow/odp.py | 8
> >>> tests/odp.at | 16 +++
> >>> 10 files changed, 176 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
> >>> index d9fb991ef..0023b65fb 100644
> >>> --- a/include/linux/openvswitch.h
> >>> +++ b/include/linux/openvswitch.h
> >>> @@ -992,6 +992,31 @@ struct check_pkt_len_arg {
> >>> };
> >>> #endif
> >>>
> >>> +#define OVS_PSAMPLE_COOKIE_MAX_SIZE 16
> >>> +/**
> >>> + * enum ovs_pample_attr - Attributes for %OVS_ACTION_ATTR_PSAMPLE
> >>> + * action.
> >>> + *
> >>> + * @OVS_PSAMPLE_ATTR_GROUP: 32-bit number to identify the source of the
> >>> + * sample.
> >>> + * @OVS_PSAMPLE_ATTR_COOKIE: An optional variable-length binary cookie
> >>> that
> >>> + * contains user-defined metadata. The maximum length is
> >>> + * OVS_PSAMPLE_COOKIE_MAX_SIZE bytes.
> >>> + *
> >>> + * Sends the packet to the psample multicast group with the specified
> >>> group and
> >>> + * cookie. It is possible to combine this action with the
> >>> + * %OVS_ACTION_ATTR_TRUNC action to limit the size of the sample.
> >>> + */
> >>> +enum ovs_psample_attr {
> >>> +OVS_PSAMPLE_ATTR_GROUP = 1,/* u32 number. */
> >>> +OVS_PSAMPLE_ATTR_COOKIE, /* Optional, user specified
> >>> cookie. */
> >>> +
> >>> +/* private: */
> >>
> >> None of the other definitions have this private marking, so I see no need
> >> to
> >> start adding it here.
> >
> > OK. The uAPI file has it (requested by netdev maintainers) but I guess
> > it's kernel-specific and this file is not a blind copy of the uAPI
> > anyways so I think we can remove it. I'll do on the next version.
> >
> >>
> >>> +__OVS_PSAMPLE_ATTR_MAX
> >>> +};
> >>> +
> >>> +#define OVS_PSAMPLE_ATTR_MAX (__OVS_PSAMPLE_ATTR_MAX - 1)
> >>> +
> >>> /**
> >>> * enum ovs_action_attr - Action types.
> >>> *
> >>> @@ -1056,6 +1081,8 @@ struct check_pkt_len_arg {
> >>> * of l3 tunnel flag in the tun_flags field of OVS_ACTION_ATTR_ADD_MPLS
> >>> * argument.
> >>> * @OVS_ACTION_ATTR_DROP: Explicit drop action.
> >>> + * @OVS_ACTION_ATTR_PSAMPLE: Send a sample of the packet to external
> >>> observers
> >>> + * via psample.
> >>> */
> >>>
> >>> enum ovs_action_attr {
> >>> @@ -1087,6 +1114,7 @@ enum ovs_action_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. */
> >>> + OVS_ACTION_ATTR_PSAMPLE, /* Nested OVS_PSAMPLE_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 c7f9e1490..f0594e5f5 100644
> >>> --- a/lib/dpif-netdev.c
> >>> +++ b/lib/dpif-netdev.c
> >>> @@ -9519,6 +9519,7 @@ dp_execute_cb(void *aux_, struct dp_packet_batch
> >>> *packets_,
> >>> case OVS_ACTION_ATTR_DROP:
> >>> case OVS_ACTION_ATTR_ADD_MPLS:
> >>> case OVS_ACTION_ATTR_DEC_TTL:
> >>> +case OVS_ACTION_ATTR_PSAMPLE:
> >>> case __OVS_ACTION_ATTR_MAX:
> >>> OVS_NOT_REACHED();
> >>> }
> >>> diff --git a/lib/dpif.c b/lib/dpif.c
> >>> index 23eb18495..71728badc 100644
> >>> --- a/lib/dpif.c
> >>> +++ b/lib/dpif.c
> >>> @@ -1192,6 +1192,7 @@ dpif_execute_helper_cb(void *aux_, struct
> >>> dp_packet_batch *packets_,
> >>> case OVS_ACTION_ATTR_TUNNEL_PUSH:
> >>> case OVS_ACTION_ATTR_TUNNEL_POP:
> >>> case OVS_ACTION_ATTR_USERSPACE:
> >>> +case OVS_ACTION_ATTR_PSAMPLE:
> >>> case OVS_ACTION_ATTR_RECIRC: {
> >>> struct dpif_execute execute;
> >>> struct ofpbuf execute_actions;
> >>> diff --git a/lib/odp-execute.c b/lib/odp-execute.c
> >>> index 081e4d432..15577d539 100644
> >>> --- a/lib/odp-execute.c
> >>> +++ b/lib/o
Re: [ovs-dev] [PATCH v14 1/6] Add global option for JSON output to ovs-appctl.
On 7/9/24 09:14, 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-appctl. It gains a global option '-f,--format' which changes it to
> print a JSON document instead of plain-text for humans. For example, a
> later patch implements support for
> 'ovs-appctl --format json dpif/show'. By default, the output format
> is plain-text as before.
>
> A new 'set-options' command has been added to lib/unixctl.c which
> allows to change the output format of the commands executed afterwards
> on the same socket connection. It is supposed to be run by ovs-appctl
> 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 afterwards it will call 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.
>
> Two access functions unixctl_command_{get,set}_output_format() and a
> unixctl_command_reply_json function have been added to lib/unixctl.h:
> unixctl_command_get_output_format() is supposed to be used in commands
> like 'dpif/show' to query the requested output format. When JSON output
> has been selected, the unixctl_command_reply_json() function can be
> used to return JSON objects to the client (ovs-appctl) instead of
> plain-text with the unixctl_command_reply{,_error}() functions.
>
> When JSON has been requested but a command has not implemented JSON
> output the plain-text output will be wrapped in a provisional JSON
> document with the following structure:
>
> {"reply":"$PLAIN_TEXT_HERE","reply-format":"plain"}
>
> Thus commands which have been executed successfully will not fail when
> they try to render the output at a later stage.
>
> A test for the 'version' command has been implemented which shows how
> the provisional JSON document looks like in practice. For a cleaner
> JSON document, the trailing newline has been moved from the program
> version string to function ovs_print_version(). This way, the
> plain-text output of the 'version' command has not changed.
>
> Output formatting has been moved from unixctl_client_transact() in
> lib/unixctl.c to utilities/ovs-appctl.c. The former merely returns the
> JSON objects returned from the server and the latter is now responsible
> for printing it properly.
>
> 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 chosen name also better aligns with ovsdb-client
> where '-f,--format' controls output formatting.
>
> Reported-at: https://bugzilla.redhat.com/1824861
> Signed-off-by: Jakob Meng
> ---
> Documentation/ref/ovs-appctl.8.rst | 12 ++
> NEWS | 3 +
> lib/unixctl.c | 180 ++---
> lib/unixctl.h | 17 ++-
> lib/util.c | 6 +-
> python/ovs/unixctl/server.py | 3 -
> tests/appctl.py| 5 +
> tests/ovs-vswitchd.at | 12 ++
> utilities/ovs-appctl.c | 135 +++---
> 9 files changed, 305 insertions(+), 68 deletions(-)
Recheck-request: github-robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn] controller: Add the capability to specify a min/max value for ct_zone.
Introduce the capability to specify boundaries (max and min values) for
the ct_zones dynamically selected by ovn-controller.
Reported-at: https://issues.redhat.com/browse/FDP-383
Signed-off-by: Lorenzo Bianconi
---
NEWS| 2 ++
controller/ct-zone.c| 37 --
controller/ct-zone.h| 4 ++-
controller/ovn-controller.c | 19 ---
tests/ovn-controller.at | 63 +
5 files changed, 111 insertions(+), 14 deletions(-)
diff --git a/NEWS b/NEWS
index 3e392ff08..421f3cd0a 100644
--- a/NEWS
+++ b/NEWS
@@ -38,6 +38,8 @@ Post v24.03.0
ability to disable "VXLAN mode" to extend available tunnel IDs space for
datapaths from 4095 to 16711680. For more details see man ovn-nb(5) for
mentioned option.
+ - Added support to define boundaries (min and max values) for selected ct
+zones.
OVN v24.03.0 - 01 Mar 2024
--
diff --git a/controller/ct-zone.c b/controller/ct-zone.c
index e4f66a52a..5191c0fdf 100644
--- a/controller/ct-zone.c
+++ b/controller/ct-zone.c
@@ -29,7 +29,8 @@ static void ct_zone_add_pending(struct shash
*pending_ct_zones,
int zone, bool add, const char *name);
static int ct_zone_get_snat(const struct sbrec_datapath_binding *dp);
static bool ct_zone_assign_unused(struct ct_zone_ctx *ctx,
- const char *zone_name, int *scan_start);
+ const char *zone_name,
+ int *scan_start, int scan_stop);
static bool ct_zone_remove(struct ct_zone_ctx *ctx,
struct simap_node *ct_zone);
@@ -89,10 +90,11 @@ ct_zones_restore(struct ct_zone_ctx *ctx,
void
ct_zones_update(const struct sset *local_lports,
+const struct ovsrec_open_vswitch_table *ovs_table,
const struct hmap *local_datapaths, struct ct_zone_ctx *ctx)
{
+int min_ct_zone = 1, max_ct_zone = MAX_CT_ZONES + 1;
struct simap_node *ct_zone;
-int scan_start = 1;
const char *user;
struct sset all_users = SSET_INITIALIZER(&all_users);
struct simap req_snat_zones = SIMAP_INITIALIZER(&req_snat_zones);
@@ -131,9 +133,18 @@ ct_zones_update(const struct sset *local_lports,
free(snat);
}
+const struct ovsrec_open_vswitch *cfg =
+ovsrec_open_vswitch_table_first(ovs_table);
+if (cfg) {
+min_ct_zone = smap_get_int(&cfg->external_ids, "ct_zone_min_val", 1);
+max_ct_zone = smap_get_int(&cfg->external_ids, "ct_zone_max_val",
+ MAX_CT_ZONES + 1);
+}
+
/* Delete zones that do not exist in above sset. */
SIMAP_FOR_EACH_SAFE (ct_zone, &ctx->current) {
-if (!sset_contains(&all_users, ct_zone->name)) {
+if (!sset_contains(&all_users, ct_zone->name) ||
+ct_zone->data < min_ct_zone || ct_zone->data > max_ct_zone) {
ct_zone_remove(ctx, ct_zone);
} else if (!simap_find(&req_snat_zones, ct_zone->name)) {
bitmap_set1(unreq_snat_zones_map, ct_zone->data);
@@ -195,7 +206,7 @@ ct_zones_update(const struct sset *local_lports,
continue;
}
-ct_zone_assign_unused(ctx, user, &scan_start);
+ct_zone_assign_unused(ctx, user, &min_ct_zone, max_ct_zone);
}
simap_destroy(&req_snat_zones);
@@ -296,11 +307,19 @@ ct_zone_handle_dp_update(struct ct_zone_ctx *ctx,
/* Returns "true" if there was an update to the context. */
bool
ct_zone_handle_port_update(struct ct_zone_ctx *ctx, const char *name,
- bool updated, int *scan_start)
+ bool updated, int *scan_start,
+ int min_ct_zone, int max_ct_zone)
{
struct simap_node *ct_zone = simap_find(&ctx->current, name);
+
+if (ct_zone &&
+(ct_zone->data < min_ct_zone || ct_zone->data > max_ct_zone)) {
+ct_zone_remove(ctx, ct_zone);
+ct_zone = NULL;
+}
+
if (updated && !ct_zone) {
-ct_zone_assign_unused(ctx, name, scan_start);
+ct_zone_assign_unused(ctx, name, scan_start, max_ct_zone);
return true;
} else if (!updated && ct_zone_remove(ctx, ct_zone)) {
return true;
@@ -312,11 +331,11 @@ ct_zone_handle_port_update(struct ct_zone_ctx *ctx, const
char *name,
static bool
ct_zone_assign_unused(struct ct_zone_ctx *ctx, const char *zone_name,
- int *scan_start)
+ int *scan_start, int scan_stop)
{
/* We assume that there are 64K zones and that we own them all. */
-int zone = bitmap_scan(ctx->bitmap, 0, *scan_start, MAX_CT_ZONES + 1);
-if (zone == MAX_CT_ZONES + 1) {
+int zone = bitmap_scan(ctx->bitmap, 0, *scan_start, scan_stop);
+if (zone == scan_stop) {
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
VLOG_WARN_RL(&rl, "exhausted all ct zones");
Re: [ovs-dev] [PATCH v1 11/13] ofproto: xlate: Make flow-sampled drops explicit.
On Tue, Jul 09, 2024 at 04:22:04PM GMT, Eelco Chaudron wrote:
>
>
> On 9 Jul 2024, at 16:00, Adrián Moreno wrote:
>
> > On Tue, Jul 09, 2024 at 11:46:06AM GMT, Eelco Chaudron wrote:
> >> On 7 Jul 2024, at 22:09, Adrian Moreno wrote:
> >>
> >>> When an action set ends in a an OFP_SAMPLE action, it means the packet
> >>> will be dropped and sampled. Make the drop explicit in this case so that
> >>> drop statistics remain accurate.
> >>>
> >>> This could be done outside of the sample action, i.e: "sample(...),drop"
> >>> but datapaths optimize sample actions that are found in the last
> >>> position. So, taking into account that datapaths already report when the
> >>> last sample probability fails, it is safe to put the drop inside the
> >>> sample, i.e: "sample(...,drop)".
> >>
> >> Some style comments below, as this was discussed with Ilya, I let him
> >> comment
> >> on the feature side.
> >>
> >>> Signed-off-by: Adrian Moreno
> >>> ---
> >>> ofproto/ofproto-dpif-xlate.c | 16 +++--
> >>> tests/drop-stats.at | 65
> >>> tests/ofproto-dpif.at| 44
> >>> 3 files changed, 123 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> >>> index b9546dc5b..094fe5d72 100644
> >>> --- a/ofproto/ofproto-dpif-xlate.c
> >>> +++ b/ofproto/ofproto-dpif-xlate.c
> >>> @@ -715,6 +715,7 @@ static void xlate_xbundle_copy(struct xbridge *,
> >>> struct xbundle *);
> >>> static void xlate_xport_copy(struct xbridge *, struct xbundle *,
> >>> struct xport *);
> >>> static void xlate_xcfg_free(struct xlate_cfg *);
> >>> +static void put_drop_action(struct ofpbuf *, enum xlate_error);
> >>>
> >>> /* Tracing helpers. */
> >>>
> >>> @@ -3392,6 +3393,8 @@ struct sample_userspace_args {
> >>> struct compose_sample_args {
> >>> uint32_t probability; /* Number of packets out of
> >>> * UINT32_MAX to sample. */
> >>> +bool last;/* If it's the last action
> >>> and a
> >>> + * drop action must be
> >>> added. */
> >>
> >> Should describing this means this is the last action not be enough?
> >> Maybe also rename it to be last_action, so it's more clear what this means.
> >>
> >
> > Sure "last_action" is pretty much self-explanatory.
> >
> >>> struct sample_userspace_args *userspace; /* Optional,
> >>> * arguments for
> >>> userspace. */
> >>> struct sample_psample_args *psample; /* Optional,
> >>> @@ -3456,6 +3459,11 @@ compose_sample_action(struct xlate_ctx *ctx,
> >>> ovs_assert(res == 0);
> >>> }
> >>>
> >>> +if (args->last &&
> >>> +ovs_explicit_drop_action_supported(ctx->xbridge->ofproto)) {
> >>> +put_drop_action(ctx->odp_actions, ctx->error);
> >>
> >> Any reason why we use ctx->error here? Should we just not use XLATE_OK?
> >>
> >
> > I was trying to thing in a case where ctx->error would not be XLATE_OK
> > but I couldn't. Nevertheless I thought if there is such case, we should
> > definitely report the right error code, right?
>
> Well, in case of an error all actions are deleted, and only an explicit drop
> (or no) actions are added.
>
> For the sample() action, the drop reason should always be ok, or else the
> action should not be there. So I would prefer to make this clear by using
> XLATE_OK.
OK. I'll change that.
>
> >>> +}
> >>> +
> >>> if (is_sample) {
> >>> nl_msg_end_nested(ctx->odp_actions, actions_offset);
> >>> nl_msg_end_nested(ctx->odp_actions, sample_offset);
> >>> @@ -3490,6 +3498,7 @@ compose_sflow_action(struct xlate_ctx *ctx)
> >>>
> >>> args.probability = dpif_sflow_get_probability(sflow);
> >>> args.userspace = &userspace;
> >>> +args.last = false;
> >>>
> >>> return compose_sample_action(ctx, &args);
> >>> }
> >>> @@ -3542,6 +3551,7 @@ compose_ipfix_action(struct xlate_ctx *ctx,
> >>> odp_port_t output_odp_port)
> >>>
> >>> args.probability = dpif_ipfix_get_bridge_exporter_probability(ipfix);
> >>> args.userspace = &userspace;
> >>> +args.last = false;
> >>>
> >>> compose_sample_action(ctx, &args);
> >>> }
> >>> @@ -5974,7 +5984,8 @@ xlate_fill_ipfix_sample(struct xlate_ctx *ctx,
> >>>
> >>> static void
> >>> xlate_sample_action(struct xlate_ctx *ctx,
> >>> -const struct ofpact_sample *os)
> >>> +const struct ofpact_sample *os,
> >>> +bool last)
> >>> {
> >>> uint8_t cookie_buf[sizeof(os->obs_domain_id) +
> >>> sizeof(os->obs_point_id)];
> >>> struct dpif_lsample *lsample = ctx->xbridge->lsample;
> >>> @@ -5991,6 +6002,7 @@ xlate_sample_action(struct xlate_ctx *ctx,
> >>> * the same percentage. */
> >>> compose_args.prob
Re: [ovs-dev] [PATCH v1 13/13] ofp-actions: Load data from fields in sample action.
On 9 Jul 2024, at 16:17, Adrián Moreno wrote:
> On Tue, Jul 09, 2024 at 11:46:12AM GMT, Eelco Chaudron wrote:
>> On 7 Jul 2024, at 22:09, Adrian Moreno wrote:
>>
>>> When sample action gets used as a way of sampling traffic with
>>> controller-generated metadata (i.e: obs_domain_id and obs_point_id),
>>> the controller will have to increase the number of flows to ensure each
>>> part of the pipeline contains the right metadata.
>>>
>>> As an example, if the controller decides to sample stateful traffic, it
>>> could store the computed metadata for each connection in the conntrack
>>> label. However, for established connections, a flow must be created for
>>> each different ct_label value with a sample action that contains a
>>> different hardcoded obs_domain and obs_point id.
>>>
>>> This patch adds a new version of the NXAST_RAW_SAMPLE* action (number 4)
>>> that supports specifying the observation point and domain using an
>>> OpenFlow field reference, so now the controller can express:
>>>
>>> sample(...
>>> obs_domain_id=NXM_NX_CT_LABEL[0..31],
>>> obs_point_id=NXM_NX_CT_LABEL[32..63]
>>> ...
>>>)
>>>
>>> Signed-off-by: Adrian Moreno
>>
>> See some comments inline. I’m missing the documentation update.
>>
>
> Yes. I noticed I missed both the NEWS and documentation update.
>
>> Cheers,
>>
>> Eelco
>>
>>> ---
>>> include/openvswitch/ofp-actions.h | 8 +-
>>> lib/ofp-actions.c | 249 +++---
>>> ofproto/ofproto-dpif-xlate.c | 55 ++-
>>> python/ovs/flow/ofp.py| 8 +-
>>> python/ovs/flow/ofp_act.py| 4 +-
>>> tests/ofp-actions.at | 5 +
>>> tests/ofproto-dpif.at | 41 +
>>> tests/system-traffic.at | 74 +
>>> 8 files changed, 405 insertions(+), 39 deletions(-)
>>>
>>> diff --git a/include/openvswitch/ofp-actions.h
>>> b/include/openvswitch/ofp-actions.h
>>> index 7b57e49ad..56dc2c147 100644
>>> --- a/include/openvswitch/ofp-actions.h
>>> +++ b/include/openvswitch/ofp-actions.h
>>> @@ -1015,14 +1015,16 @@ enum nx_action_sample_direction {
>>>
>>> /* OFPACT_SAMPLE.
>>> *
>>> - * Used for NXAST_SAMPLE, NXAST_SAMPLE2, and NXAST_SAMPLE3. */
>>> + * Used for NXAST_SAMPLE, NXAST_SAMPLE2, NXAST_SAMPLE3 and NXAST_SAMPLE4.
>>> */
>>> struct ofpact_sample {
>>> OFPACT_PADDED_MEMBERS(
>>> struct ofpact ofpact;
>>> uint16_t probability; /* Always positive. */
>>> uint32_t collector_set_id;
>>> -uint32_t obs_domain_id;
>>> -uint32_t obs_point_id;
>>> +uint32_t obs_domain_imm;
>>> +struct mf_subfield obs_domain_src;
>>> +uint32_t obs_point_imm;
>>> +struct mf_subfield obs_point_src;
>>> ofp_port_t sampling_port;
>>> enum nx_action_sample_direction direction;
>>> );
>>> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
>>> index da7b1dd31..e329a7e3f 100644
>>> --- a/lib/ofp-actions.c
>>> +++ b/lib/ofp-actions.c
>>> @@ -330,6 +330,8 @@ enum ofp_raw_action_type {
>>> NXAST_RAW_SAMPLE2,
>>> /* NX1.0+(41): struct nx_action_sample2. */
>>> NXAST_RAW_SAMPLE3,
>>> +/* NX1.0+(51): struct nx_action_sample4, ... VLMFF */
>>> +NXAST_RAW_SAMPLE4,
>>>
>>> /* NX1.0+(34): struct nx_action_conjunction. */
>>> NXAST_RAW_CONJUNCTION,
>>> @@ -6188,6 +6190,34 @@ struct nx_action_sample2 {
>>> };
>>> OFP_ASSERT(sizeof(struct nx_action_sample2) == 32);
>>>
>>> +/* Action structure for NXAST_SAMPLE4
>>> + *
>>> + * NXAST_SAMPLE4 was added in Open vSwitch 3.4.0. Compared to
>>> NXAST_SAMPLE3,
>>> + * it adds support for using field specifiers for observation_domain_id and
>>> + * observation_point_id. */
>>> +struct nx_action_sample4 {
>>> +ovs_be16 type; /* OFPAT_VENDOR. */
>>> +ovs_be16 len; /* Length is 32. */
>>> +ovs_be32 vendor;/* NX_VENDOR_ID. */
>>> +ovs_be16 subtype; /* NXAST_SAMPLE. */
>>> +ovs_be16 probability; /* Fraction of packets to sample. */
>>> +ovs_be32 collector_set_id; /* ID of collector set in OVSDB. */
>>> +ovs_be32 obs_domain_src;/* Source of the
>>> observation_domain_id. */
>>> +union {
>>> +ovs_be16 obs_domain_ofs_nbits; /* Range to use from source field.
>>> */
>>> +ovs_be32 obs_domain_imm;/* Immediate value for domain id.
>>> */
>>> +};
>>> +ovs_be32 obs_point_src; /* Source of the observation_point_id
>>> */
>>> +union {
>>> +ovs_be16 obs_point_ofs_nbits; /* Range to use from source field.
>>> */
>>> +ovs_be32 obs_point_imm;/* Immediate value for point id. */
>>> +};
>>> +ovs_be16 sampling_port; /* Sampling port. */
>>> +uint8_t direction; /* Sampling direction. */
>>> +uint8_t zeros[5]; /* Pad to a multiple of 8 bytes */
>>> + };
>>
>> Maybe align all comments, and
Re: [ovs-dev] [PATCH v1 11/13] ofproto: xlate: Make flow-sampled drops explicit.
On 9 Jul 2024, at 16:00, Adrián Moreno wrote:
> On Tue, Jul 09, 2024 at 11:46:06AM GMT, Eelco Chaudron wrote:
>> On 7 Jul 2024, at 22:09, Adrian Moreno wrote:
>>
>>> When an action set ends in a an OFP_SAMPLE action, it means the packet
>>> will be dropped and sampled. Make the drop explicit in this case so that
>>> drop statistics remain accurate.
>>>
>>> This could be done outside of the sample action, i.e: "sample(...),drop"
>>> but datapaths optimize sample actions that are found in the last
>>> position. So, taking into account that datapaths already report when the
>>> last sample probability fails, it is safe to put the drop inside the
>>> sample, i.e: "sample(...,drop)".
>>
>> Some style comments below, as this was discussed with Ilya, I let him comment
>> on the feature side.
>>
>>> Signed-off-by: Adrian Moreno
>>> ---
>>> ofproto/ofproto-dpif-xlate.c | 16 +++--
>>> tests/drop-stats.at | 65
>>> tests/ofproto-dpif.at| 44
>>> 3 files changed, 123 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>>> index b9546dc5b..094fe5d72 100644
>>> --- a/ofproto/ofproto-dpif-xlate.c
>>> +++ b/ofproto/ofproto-dpif-xlate.c
>>> @@ -715,6 +715,7 @@ static void xlate_xbundle_copy(struct xbridge *, struct
>>> xbundle *);
>>> static void xlate_xport_copy(struct xbridge *, struct xbundle *,
>>> struct xport *);
>>> static void xlate_xcfg_free(struct xlate_cfg *);
>>> +static void put_drop_action(struct ofpbuf *, enum xlate_error);
>>>
>>> /* Tracing helpers. */
>>>
>>> @@ -3392,6 +3393,8 @@ struct sample_userspace_args {
>>> struct compose_sample_args {
>>> uint32_t probability; /* Number of packets out of
>>> * UINT32_MAX to sample. */
>>> +bool last;/* If it's the last action
>>> and a
>>> + * drop action must be
>>> added. */
>>
>> Should describing this means this is the last action not be enough?
>> Maybe also rename it to be last_action, so it's more clear what this means.
>>
>
> Sure "last_action" is pretty much self-explanatory.
>
>>> struct sample_userspace_args *userspace; /* Optional,
>>> * arguments for userspace.
>>> */
>>> struct sample_psample_args *psample; /* Optional,
>>> @@ -3456,6 +3459,11 @@ compose_sample_action(struct xlate_ctx *ctx,
>>> ovs_assert(res == 0);
>>> }
>>>
>>> +if (args->last &&
>>> +ovs_explicit_drop_action_supported(ctx->xbridge->ofproto)) {
>>> +put_drop_action(ctx->odp_actions, ctx->error);
>>
>> Any reason why we use ctx->error here? Should we just not use XLATE_OK?
>>
>
> I was trying to thing in a case where ctx->error would not be XLATE_OK
> but I couldn't. Nevertheless I thought if there is such case, we should
> definitely report the right error code, right?
Well, in case of an error all actions are deleted, and only an explicit drop
(or no) actions are added.
For the sample() action, the drop reason should always be ok, or else the
action should not be there. So I would prefer to make this clear by using
XLATE_OK.
>>> +}
>>> +
>>> if (is_sample) {
>>> nl_msg_end_nested(ctx->odp_actions, actions_offset);
>>> nl_msg_end_nested(ctx->odp_actions, sample_offset);
>>> @@ -3490,6 +3498,7 @@ compose_sflow_action(struct xlate_ctx *ctx)
>>>
>>> args.probability = dpif_sflow_get_probability(sflow);
>>> args.userspace = &userspace;
>>> +args.last = false;
>>>
>>> return compose_sample_action(ctx, &args);
>>> }
>>> @@ -3542,6 +3551,7 @@ compose_ipfix_action(struct xlate_ctx *ctx,
>>> odp_port_t output_odp_port)
>>>
>>> args.probability = dpif_ipfix_get_bridge_exporter_probability(ipfix);
>>> args.userspace = &userspace;
>>> +args.last = false;
>>>
>>> compose_sample_action(ctx, &args);
>>> }
>>> @@ -5974,7 +5984,8 @@ xlate_fill_ipfix_sample(struct xlate_ctx *ctx,
>>>
>>> static void
>>> xlate_sample_action(struct xlate_ctx *ctx,
>>> -const struct ofpact_sample *os)
>>> +const struct ofpact_sample *os,
>>> +bool last)
>>> {
>>> uint8_t cookie_buf[sizeof(os->obs_domain_id) +
>>> sizeof(os->obs_point_id)];
>>> struct dpif_lsample *lsample = ctx->xbridge->lsample;
>>> @@ -5991,6 +6002,7 @@ xlate_sample_action(struct xlate_ctx *ctx,
>>> * the same percentage. */
>>> compose_args.probability =
>>> ((uint32_t) os->probability << 16) | os->probability;
>>> +compose_args.last = last;
>>>
>>> if (ipfix) {
>>> xlate_fill_ipfix_sample(ctx, os, ipfix, &userspace);
>>> @@ -7726,7 +7738,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t
>>> ofpacts_len,
>>> b
Re: [ovs-dev] [PATCH v1 13/13] ofp-actions: Load data from fields in sample action.
On Tue, Jul 09, 2024 at 11:46:12AM GMT, Eelco Chaudron wrote:
> On 7 Jul 2024, at 22:09, Adrian Moreno wrote:
>
> > When sample action gets used as a way of sampling traffic with
> > controller-generated metadata (i.e: obs_domain_id and obs_point_id),
> > the controller will have to increase the number of flows to ensure each
> > part of the pipeline contains the right metadata.
> >
> > As an example, if the controller decides to sample stateful traffic, it
> > could store the computed metadata for each connection in the conntrack
> > label. However, for established connections, a flow must be created for
> > each different ct_label value with a sample action that contains a
> > different hardcoded obs_domain and obs_point id.
> >
> > This patch adds a new version of the NXAST_RAW_SAMPLE* action (number 4)
> > that supports specifying the observation point and domain using an
> > OpenFlow field reference, so now the controller can express:
> >
> > sample(...
> > obs_domain_id=NXM_NX_CT_LABEL[0..31],
> > obs_point_id=NXM_NX_CT_LABEL[32..63]
> > ...
> >)
> >
> > Signed-off-by: Adrian Moreno
>
> See some comments inline. I’m missing the documentation update.
>
Yes. I noticed I missed both the NEWS and documentation update.
> Cheers,
>
> Eelco
>
> > ---
> > include/openvswitch/ofp-actions.h | 8 +-
> > lib/ofp-actions.c | 249 +++---
> > ofproto/ofproto-dpif-xlate.c | 55 ++-
> > python/ovs/flow/ofp.py| 8 +-
> > python/ovs/flow/ofp_act.py| 4 +-
> > tests/ofp-actions.at | 5 +
> > tests/ofproto-dpif.at | 41 +
> > tests/system-traffic.at | 74 +
> > 8 files changed, 405 insertions(+), 39 deletions(-)
> >
> > diff --git a/include/openvswitch/ofp-actions.h
> > b/include/openvswitch/ofp-actions.h
> > index 7b57e49ad..56dc2c147 100644
> > --- a/include/openvswitch/ofp-actions.h
> > +++ b/include/openvswitch/ofp-actions.h
> > @@ -1015,14 +1015,16 @@ enum nx_action_sample_direction {
> >
> > /* OFPACT_SAMPLE.
> > *
> > - * Used for NXAST_SAMPLE, NXAST_SAMPLE2, and NXAST_SAMPLE3. */
> > + * Used for NXAST_SAMPLE, NXAST_SAMPLE2, NXAST_SAMPLE3 and NXAST_SAMPLE4.
> > */
> > struct ofpact_sample {
> > OFPACT_PADDED_MEMBERS(
> > struct ofpact ofpact;
> > uint16_t probability; /* Always positive. */
> > uint32_t collector_set_id;
> > -uint32_t obs_domain_id;
> > -uint32_t obs_point_id;
> > +uint32_t obs_domain_imm;
> > +struct mf_subfield obs_domain_src;
> > +uint32_t obs_point_imm;
> > +struct mf_subfield obs_point_src;
> > ofp_port_t sampling_port;
> > enum nx_action_sample_direction direction;
> > );
> > diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> > index da7b1dd31..e329a7e3f 100644
> > --- a/lib/ofp-actions.c
> > +++ b/lib/ofp-actions.c
> > @@ -330,6 +330,8 @@ enum ofp_raw_action_type {
> > NXAST_RAW_SAMPLE2,
> > /* NX1.0+(41): struct nx_action_sample2. */
> > NXAST_RAW_SAMPLE3,
> > +/* NX1.0+(51): struct nx_action_sample4, ... VLMFF */
> > +NXAST_RAW_SAMPLE4,
> >
> > /* NX1.0+(34): struct nx_action_conjunction. */
> > NXAST_RAW_CONJUNCTION,
> > @@ -6188,6 +6190,34 @@ struct nx_action_sample2 {
> > };
> > OFP_ASSERT(sizeof(struct nx_action_sample2) == 32);
> >
> > +/* Action structure for NXAST_SAMPLE4
> > + *
> > + * NXAST_SAMPLE4 was added in Open vSwitch 3.4.0. Compared to
> > NXAST_SAMPLE3,
> > + * it adds support for using field specifiers for observation_domain_id and
> > + * observation_point_id. */
> > +struct nx_action_sample4 {
> > +ovs_be16 type; /* OFPAT_VENDOR. */
> > +ovs_be16 len; /* Length is 32. */
> > +ovs_be32 vendor;/* NX_VENDOR_ID. */
> > +ovs_be16 subtype; /* NXAST_SAMPLE. */
> > +ovs_be16 probability; /* Fraction of packets to sample. */
> > +ovs_be32 collector_set_id; /* ID of collector set in OVSDB. */
> > +ovs_be32 obs_domain_src;/* Source of the
> > observation_domain_id. */
> > +union {
> > +ovs_be16 obs_domain_ofs_nbits; /* Range to use from source field.
> > */
> > +ovs_be32 obs_domain_imm;/* Immediate value for domain id.
> > */
> > +};
> > +ovs_be32 obs_point_src; /* Source of the observation_point_id
> > */
> > +union {
> > +ovs_be16 obs_point_ofs_nbits; /* Range to use from source field.
> > */
> > +ovs_be32 obs_point_imm;/* Immediate value for point id. */
> > +};
> > +ovs_be16 sampling_port; /* Sampling port. */
> > +uint8_t direction; /* Sampling direction. */
> > +uint8_t zeros[5]; /* Pad to a multiple of 8 bytes */
> > + };
>
> Maybe align all comments, and make sure all end with a dot.
>
> struct nx_action_sample4 {
>
Re: [ovs-dev] [PATCH v1 02/13] odp-util: Add support OVS_ACTION_ATTR_PSAMPLE.
On 9 Jul 2024, at 15:30, Adrián Moreno wrote:
> On Tue, Jul 09, 2024 at 11:45:15AM GMT, Eelco Chaudron wrote:
>> On 4 Jul 2024, at 9:52, Adrian Moreno wrote:
>>
>>> Add support for parsing and formatting the new action.
>>>
>>> Also, flag OVS_ACTION_ATTR_SAMPLE as requiring datapath assistance if it
>>> contains a nested OVS_ACTION_ATTR_PSAMPLE. The reason is that the
>>> sampling rate form the parent "sample" is made available to the nested
>>
>> form -> from
>>
>>> "psample" by the kernel.
>>
>> Two small comments below, the rest looks good.
>>
>>> Signed-off-by: Adrian Moreno
>>> ---
>>> include/linux/openvswitch.h | 28 +++
>>> lib/dpif-netdev.c| 1 +
>>> lib/dpif.c | 1 +
>>> lib/odp-execute.c| 25 +-
>>> lib/odp-util.c | 93
>>> lib/odp-util.h | 3 ++
>>> ofproto/ofproto-dpif-ipfix.c | 1 +
>>> ofproto/ofproto-dpif-sflow.c | 1 +
>>> python/ovs/flow/odp.py | 8
>>> tests/odp.at | 16 +++
>>> 10 files changed, 176 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
>>> index d9fb991ef..0023b65fb 100644
>>> --- a/include/linux/openvswitch.h
>>> +++ b/include/linux/openvswitch.h
>>> @@ -992,6 +992,31 @@ struct check_pkt_len_arg {
>>> };
>>> #endif
>>>
>>> +#define OVS_PSAMPLE_COOKIE_MAX_SIZE 16
>>> +/**
>>> + * enum ovs_pample_attr - Attributes for %OVS_ACTION_ATTR_PSAMPLE
>>> + * action.
>>> + *
>>> + * @OVS_PSAMPLE_ATTR_GROUP: 32-bit number to identify the source of the
>>> + * sample.
>>> + * @OVS_PSAMPLE_ATTR_COOKIE: An optional variable-length binary cookie that
>>> + * contains user-defined metadata. The maximum length is
>>> + * OVS_PSAMPLE_COOKIE_MAX_SIZE bytes.
>>> + *
>>> + * Sends the packet to the psample multicast group with the specified
>>> group and
>>> + * cookie. It is possible to combine this action with the
>>> + * %OVS_ACTION_ATTR_TRUNC action to limit the size of the sample.
>>> + */
>>> +enum ovs_psample_attr {
>>> +OVS_PSAMPLE_ATTR_GROUP = 1,/* u32 number. */
>>> +OVS_PSAMPLE_ATTR_COOKIE, /* Optional, user specified cookie.
>>> */
>>> +
>>> +/* private: */
>>
>> None of the other definitions have this private marking, so I see no need to
>> start adding it here.
>
> OK. The uAPI file has it (requested by netdev maintainers) but I guess
> it's kernel-specific and this file is not a blind copy of the uAPI
> anyways so I think we can remove it. I'll do on the next version.
>
>>
>>> +__OVS_PSAMPLE_ATTR_MAX
>>> +};
>>> +
>>> +#define OVS_PSAMPLE_ATTR_MAX (__OVS_PSAMPLE_ATTR_MAX - 1)
>>> +
>>> /**
>>> * enum ovs_action_attr - Action types.
>>> *
>>> @@ -1056,6 +1081,8 @@ struct check_pkt_len_arg {
>>> * of l3 tunnel flag in the tun_flags field of OVS_ACTION_ATTR_ADD_MPLS
>>> * argument.
>>> * @OVS_ACTION_ATTR_DROP: Explicit drop action.
>>> + * @OVS_ACTION_ATTR_PSAMPLE: Send a sample of the packet to external
>>> observers
>>> + * via psample.
>>> */
>>>
>>> enum ovs_action_attr {
>>> @@ -1087,6 +1114,7 @@ enum ovs_action_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. */
>>> + OVS_ACTION_ATTR_PSAMPLE, /* Nested OVS_PSAMPLE_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 c7f9e1490..f0594e5f5 100644
>>> --- a/lib/dpif-netdev.c
>>> +++ b/lib/dpif-netdev.c
>>> @@ -9519,6 +9519,7 @@ dp_execute_cb(void *aux_, struct dp_packet_batch
>>> *packets_,
>>> case OVS_ACTION_ATTR_DROP:
>>> case OVS_ACTION_ATTR_ADD_MPLS:
>>> case OVS_ACTION_ATTR_DEC_TTL:
>>> +case OVS_ACTION_ATTR_PSAMPLE:
>>> case __OVS_ACTION_ATTR_MAX:
>>> OVS_NOT_REACHED();
>>> }
>>> diff --git a/lib/dpif.c b/lib/dpif.c
>>> index 23eb18495..71728badc 100644
>>> --- a/lib/dpif.c
>>> +++ b/lib/dpif.c
>>> @@ -1192,6 +1192,7 @@ dpif_execute_helper_cb(void *aux_, struct
>>> dp_packet_batch *packets_,
>>> case OVS_ACTION_ATTR_TUNNEL_PUSH:
>>> case OVS_ACTION_ATTR_TUNNEL_POP:
>>> case OVS_ACTION_ATTR_USERSPACE:
>>> +case OVS_ACTION_ATTR_PSAMPLE:
>>> case OVS_ACTION_ATTR_RECIRC: {
>>> struct dpif_execute execute;
>>> struct ofpbuf execute_actions;
>>> diff --git a/lib/odp-execute.c b/lib/odp-execute.c
>>> index 081e4d432..15577d539 100644
>>> --- a/lib/odp-execute.c
>>> +++ b/lib/odp-execute.c
>>> @@ -818,13 +818,13 @@ requires_datapath_assistance(const struct nlattr *a)
>>> case OVS_ACTION_ATTR_RECIRC:
>>> case OVS_ACTION_ATTR_CT:
>>> case OVS_ACTION_ATTR_METER:
>>> +case OVS_ACTION_ATTR_PSAMPLE:
>>> return true;
>>>
>>> case OVS_ACTION_ATTR_SET:
>>> case
Re: [ovs-dev] [PATCH v1 06/13] ofproto-dpif-xlate: Use psample for local sample.
On 9 Jul 2024, at 15:52, Adrián Moreno wrote:
> On Tue, Jul 09, 2024 at 11:45:51AM GMT, Eelco Chaudron wrote:
>> On 7 Jul 2024, at 22:08, Adrian Moreno wrote:
>>
>>> Use the newly added psample action to implement OpenFlow sample() actions
>>> with local sampling configuration if possible.
>>>
>>> A bit of refactoring in compose_sample_actions arguments helps make it a
>>> bit more readable.
>>
>> Some comments below.
>>
>> Cheers,
>>
>> Eelco
>>
>>> Signed-off-by: Adrian Moreno
>>> ---
>>> ofproto/ofproto-dpif-lsample.c | 16 +++
>>> ofproto/ofproto-dpif-lsample.h | 5 +
>>> ofproto/ofproto-dpif-xlate.c | 251 +++--
>>> ofproto/ofproto-dpif-xlate.h | 5 +-
>>> ofproto/ofproto-dpif.c | 2 +-
>>> tests/ofproto-dpif.at | 146 +++
>>> 6 files changed, 345 insertions(+), 80 deletions(-)
>>>
>>> diff --git a/ofproto/ofproto-dpif-lsample.c b/ofproto/ofproto-dpif-lsample.c
>>> index d675a116f..534ad96f0 100644
>>> --- a/ofproto/ofproto-dpif-lsample.c
>>> +++ b/ofproto/ofproto-dpif-lsample.c
>>> @@ -140,6 +140,22 @@ dpif_lsample_set_options(struct dpif_lsample *lsample,
>>> return changed;
>>> }
>>>
>>> +/* Returns the group_id for a given collector_set_id, if it exists. */
>>> +bool
>>> +dpif_lsample_get_group_id(struct dpif_lsample *ps, uint32_t
>>> collector_set_id,
>>> + uint32_t *group_id)
>>> +{
>>> +struct lsample_exporter_node *node;
>>> +bool found = false;
>>> +
>>> +node = dpif_lsample_find_exporter_node(ps, collector_set_id);
>>> +if (node) {
>>> +found = true;
>>> +*group_id = node->exporter.options.group_id;
>>> +}
>>> +return found;
>>> +}
>>> +
>>> struct dpif_lsample *
>>> dpif_lsample_create(void)
>>> {
>>> diff --git a/ofproto/ofproto-dpif-lsample.h b/ofproto/ofproto-dpif-lsample.h
>>> index bee36c9c5..9c1026551 100644
>>> --- a/ofproto/ofproto-dpif-lsample.h
>>> +++ b/ofproto/ofproto-dpif-lsample.h
>>> @@ -18,6 +18,7 @@
>>> #define OFPROTO_DPIF_LSAMPLE_H 1
>>>
>>> #include
>>> +#include
>>> #include
>>>
>>> struct dpif_lsample;
>>> @@ -33,4 +34,8 @@ bool dpif_lsample_set_options(struct dpif_lsample *,
>>>const struct ofproto_lsample_options *,
>>>size_t n_opts);
>>>
>>> +bool dpif_lsample_get_group_id(struct dpif_lsample *,
>>> + uint32_t collector_set_id,
>>> + uint32_t *group_id);
>>> +
>>> #endif /* OFPROTO_DPIF_LSAMPLE_H */
>>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>>> index 7c4950895..5e8113d5e 100644
>>> --- a/ofproto/ofproto-dpif-xlate.c
>>> +++ b/ofproto/ofproto-dpif-xlate.c
>>> @@ -47,6 +47,7 @@
>>> #include "ofproto/ofproto-dpif-ipfix.h"
>>> #include "ofproto/ofproto-dpif-mirror.h"
>>> #include "ofproto/ofproto-dpif-monitor.h"
>>> +#include "ofproto/ofproto-dpif-lsample.h"
>>
>> Add in alphabetical order?
>
> Ack.
>
>>
>>> #include "ofproto/ofproto-dpif-sflow.h"
>>> #include "ofproto/ofproto-dpif-trace.h"
>>> #include "ofproto/ofproto-dpif-xlate-cache.h"
>>> @@ -117,6 +118,7 @@ struct xbridge {
>>> struct dpif_sflow *sflow; /* SFlow handle, or null. */
>>> struct dpif_ipfix *ipfix; /* Ipfix handle, or null. */
>>> struct netflow *netflow; /* Netflow handle, or null. */
>>> +struct dpif_lsample *lsample; /* Local sample handle, or null. */
>>
>> I would move above netflow, as you also do the actual init/unref before
>> netflow.
>
>
> Ack.
>
>
>>
>>> struct stp *stp; /* STP or null if disabled. */
>>> struct rstp *rstp;/* RSTP or null if disabled. */
>>>
>>> @@ -686,6 +688,7 @@ static void xlate_xbridge_set(struct xbridge *, struct
>>> dpif *,
>>>const struct mbridge *,
>>>const struct dpif_sflow *,
>>>const struct dpif_ipfix *,
>>> + const struct dpif_lsample *,
>>>const struct netflow *,
>>>bool forward_bpdu, bool has_in_band,
>>>const struct dpif_backer_support *,
>>> @@ -1069,6 +1072,7 @@ xlate_xbridge_set(struct xbridge *xbridge,
>>>const struct mbridge *mbridge,
>>>const struct dpif_sflow *sflow,
>>>const struct dpif_ipfix *ipfix,
>>> + const struct dpif_lsample *lsample,
>>>const struct netflow *netflow,
>>>bool forward_bpdu, bool has_in_band,
>>>const struct dpif_backer_support *support,
>>> @@ -1099,6 +1103,11 @@ xlate_xbridge_set(struct xbridge *xbridge,
>>> xbridge->ipfix = dpif_ipfix_ref(ipfix);
>>> }
>>>
>>> +if (xbridge->lsample != lsample) {
>>> +dpif_lsample_unref(xbridge->lsample);
>>> +xbridge->lsa
Re: [ovs-dev] [PATCH v1 11/13] ofproto: xlate: Make flow-sampled drops explicit.
On Tue, Jul 09, 2024 at 11:46:06AM GMT, Eelco Chaudron wrote:
> On 7 Jul 2024, at 22:09, Adrian Moreno wrote:
>
> > When an action set ends in a an OFP_SAMPLE action, it means the packet
> > will be dropped and sampled. Make the drop explicit in this case so that
> > drop statistics remain accurate.
> >
> > This could be done outside of the sample action, i.e: "sample(...),drop"
> > but datapaths optimize sample actions that are found in the last
> > position. So, taking into account that datapaths already report when the
> > last sample probability fails, it is safe to put the drop inside the
> > sample, i.e: "sample(...,drop)".
>
> Some style comments below, as this was discussed with Ilya, I let him comment
> on the feature side.
>
> > Signed-off-by: Adrian Moreno
> > ---
> > ofproto/ofproto-dpif-xlate.c | 16 +++--
> > tests/drop-stats.at | 65
> > tests/ofproto-dpif.at| 44
> > 3 files changed, 123 insertions(+), 2 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> > index b9546dc5b..094fe5d72 100644
> > --- a/ofproto/ofproto-dpif-xlate.c
> > +++ b/ofproto/ofproto-dpif-xlate.c
> > @@ -715,6 +715,7 @@ static void xlate_xbundle_copy(struct xbridge *, struct
> > xbundle *);
> > static void xlate_xport_copy(struct xbridge *, struct xbundle *,
> > struct xport *);
> > static void xlate_xcfg_free(struct xlate_cfg *);
> > +static void put_drop_action(struct ofpbuf *, enum xlate_error);
> >
> > /* Tracing helpers. */
> >
> > @@ -3392,6 +3393,8 @@ struct sample_userspace_args {
> > struct compose_sample_args {
> > uint32_t probability; /* Number of packets out of
> > * UINT32_MAX to sample. */
> > +bool last;/* If it's the last action
> > and a
> > + * drop action must be
> > added. */
>
> Should describing this means this is the last action not be enough?
> Maybe also rename it to be last_action, so it's more clear what this means.
>
Sure "last_action" is pretty much self-explanatory.
> > struct sample_userspace_args *userspace; /* Optional,
> > * arguments for userspace.
> > */
> > struct sample_psample_args *psample; /* Optional,
> > @@ -3456,6 +3459,11 @@ compose_sample_action(struct xlate_ctx *ctx,
> > ovs_assert(res == 0);
> > }
> >
> > +if (args->last &&
> > +ovs_explicit_drop_action_supported(ctx->xbridge->ofproto)) {
> > +put_drop_action(ctx->odp_actions, ctx->error);
>
> Any reason why we use ctx->error here? Should we just not use XLATE_OK?
>
I was trying to thing in a case where ctx->error would not be XLATE_OK
but I couldn't. Nevertheless I thought if there is such case, we should
definitely report the right error code, right?
> > +}
> > +
> > if (is_sample) {
> > nl_msg_end_nested(ctx->odp_actions, actions_offset);
> > nl_msg_end_nested(ctx->odp_actions, sample_offset);
> > @@ -3490,6 +3498,7 @@ compose_sflow_action(struct xlate_ctx *ctx)
> >
> > args.probability = dpif_sflow_get_probability(sflow);
> > args.userspace = &userspace;
> > +args.last = false;
> >
> > return compose_sample_action(ctx, &args);
> > }
> > @@ -3542,6 +3551,7 @@ compose_ipfix_action(struct xlate_ctx *ctx,
> > odp_port_t output_odp_port)
> >
> > args.probability = dpif_ipfix_get_bridge_exporter_probability(ipfix);
> > args.userspace = &userspace;
> > +args.last = false;
> >
> > compose_sample_action(ctx, &args);
> > }
> > @@ -5974,7 +5984,8 @@ xlate_fill_ipfix_sample(struct xlate_ctx *ctx,
> >
> > static void
> > xlate_sample_action(struct xlate_ctx *ctx,
> > -const struct ofpact_sample *os)
> > +const struct ofpact_sample *os,
> > +bool last)
> > {
> > uint8_t cookie_buf[sizeof(os->obs_domain_id) +
> > sizeof(os->obs_point_id)];
> > struct dpif_lsample *lsample = ctx->xbridge->lsample;
> > @@ -5991,6 +6002,7 @@ xlate_sample_action(struct xlate_ctx *ctx,
> > * the same percentage. */
> > compose_args.probability =
> > ((uint32_t) os->probability << 16) | os->probability;
> > +compose_args.last = last;
> >
> > if (ipfix) {
> > xlate_fill_ipfix_sample(ctx, os, ipfix, &userspace);
> > @@ -7726,7 +7738,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t
> > ofpacts_len,
> > break;
> >
> > case OFPACT_SAMPLE:
> > -xlate_sample_action(ctx, ofpact_get_SAMPLE(a));
> > +xlate_sample_action(ctx, ofpact_get_SAMPLE(a), last);
> > break;
> >
> > case OFPACT_CLONE:
> > diff --git a/tests/drop-stats.at b/tests/drop-stats.at
> > index 1d3af98da..44c5cc35b 100644
> > --
Re: [ovs-dev] [PATCH v1 10/13] tests: Test local sampling.
On Tue, Jul 09, 2024 at 11:46:03AM GMT, Eelco Chaudron wrote: > On 7 Jul 2024, at 22:09, Adrian Moreno wrote: > > > Test simultaneous IPFIX and local sampling including slow-path. > > See comments below. > > Cheers, > > Eelco > > > Signed-off-by: Adrian Moreno > > --- > > tests/system-common-macros.at | 4 + > > tests/system-traffic.at | 285 ++ > > 2 files changed, 289 insertions(+) > > > > diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at > > index 2a68cd664..860d6a8c9 100644 > > --- a/tests/system-common-macros.at > > +++ b/tests/system-common-macros.at > > @@ -378,3 +378,7 @@ m4_define([OVS_CHECK_GITHUB_ACTION], > > # OVS_CHECK_DROP_ACTION() > > m4_define([OVS_CHECK_DROP_ACTION], > > [AT_SKIP_IF([! grep -q "Datapath supports explicit drop action" > > ovs-vswitchd.log])]) > > + > > +# OVS_CHECK_PSAMPLE() > > +m4_define([OVS_CHECK_PSAMPLE], > > +[AT_SKIP_IF([! grep -q "Datapath supports psample" ovs-vswitchd.log])]) > > diff --git a/tests/system-traffic.at b/tests/system-traffic.at > > index 3f1a15445..ddab4ece3 100644 > > --- a/tests/system-traffic.at > > +++ b/tests/system-traffic.at > > @@ -9103,3 +9103,288 @@ OVS_WAIT_UNTIL([ovs-pcap p2.pcap | grep -q > > "m4_join([], [^], > > > > OVS_TRAFFIC_VSWITCHD_STOP > > AT_CLEANUP > > + > > +AT_BANNER([local-sampling]) > > + > > +m4_define([SAMPLE_ACTION], > > + > > [sample(probability=65535,collector_set_id=$1,obs_domain_id=$2,obs_point_id=$3)]dnl > > +) > > + > > +AT_SETUP([psample - sanity check]) > > Maybe call it '- ipv4' as it's equal to ipv6, or rename the IPv6 one to > 'sanity check IPv6' Ack. > > > +OVS_TRAFFIC_VSWITCHD_START() > > +OVS_CHECK_PSAMPLE() > > + > > +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") > > + > > +AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \ > > +-- create Flow_Sample_Collector_Set id=1 bridge=@br0 > > local_group_id=10 \ > > +-- create Flow_Sample_Collector_Set id=2 bridge=@br0 > > local_group_id=12], > > + [0], [ignore]) > > Maybe wrap to stay <80 chars? > > AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \ > -- create Flow_Sample_Collector_Set id=1 bridge=@br0 \ >local_group_id=10 \ > -- create Flow_Sample_Collector_Set id=2 bridge=@br0 \ >local_group_id=12], > [0], [ignore]) > Ack. > > + > > +AT_DATA([flows.txt], [dnl > > +arp actions=NORMAL > > +in_port=ovs-p0,ip actions=SAMPLE_ACTION(1, 2853183536, 2856341600),ovs-p1 > > +in_port=ovs-p1,ip actions=SAMPLE_ACTION(2, 3138396208, 3141554272),ovs-p0 > > +]) > > + > > +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) > > + > > +OVS_DAEMONIZE([ovstest test-psample > psample.out], [psample.pid]) > > +OVS_WAIT_UNTIL([grep -q "Listening for psample events" psample.out]) > > + > > +NS_CHECK_EXEC([at_ns0], [ping -q -c 1 10.1.1.2 | FORMAT_PING], [0], [dnl > > +1 packets transmitted, 1 received, 0% packet loss, time 0ms > > +]) > > + > > +AT_CHECK([ovs-appctl dpctl/dump-flows -m --names], [0], [stdout]) > > +AT_CHECK([grep -q > > 'actions:psample(group=10,cookie=0xaa102030aa405060),ovs-p1' stdout]) > > +AT_CHECK([grep -q > > 'actions:psample(group=12,cookie=0xbb102030bb405060),ovs-p0' stdout]) > > + > > +m4_define([SAMPLE1], [m4_join([ ], > > +[group_id=0xa], > > +[obs_domain=0xaa102030,obs_point=0xaa405060], > > +[.*icmp.*nw_src=10.1.1.1,nw_dst=10.1.1.2])]) > > + > > +m4_define([SAMPLE2], [m4_join([ ], > > +[group_id=0xc], > > +[obs_domain=0xbb102030,obs_point=0xbb405060], > > +[.*icmp.*nw_src=10.1.1.2,nw_dst=10.1.1.1])]) > > + > > +OVS_WAIT_UNTIL([grep -qE 'SAMPLE1' psample.out]) > > +OVS_WAIT_UNTIL([grep -qE 'SAMPLE2' psample.out]) > > + > > +AT_CHECK([ovs-appctl lsample/show br0], [0], [dnl > > +Local sample statistics for bridge "br0": > > +Collector Set ID: 1: > > + Group ID : 10 > > + Total packets: 1 > > + Total bytes : 98 > > + > > +Collector Set ID: 2: > > + Group ID : 12 > > + Total packets: 1 > > + Total bytes : 98 > > +]) > > + > > We miss OVS_TRAFFIC_VSWITCHD_STOP() here. > Yes! > > +AT_CLEANUP > > + > > +AT_SETUP([psample - ipv6]) > > See comment above, if you decide to keep it ipv6, please change it to IPv6. > > > +OVS_TRAFFIC_VSWITCHD_START() > > +OVS_CHECK_PSAMPLE() > > + > > +ADD_NAMESPACES(at_ns0, at_ns1) > > + > > +ADD_VETH(p0, at_ns0, br0, "fc00::1/96") > > +ADD_VETH(p1, at_ns1, br0, "fc00::2/96") > > + > > +AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \ > > +-- create Flow_Sample_Collector_Set id=1 bridge=@br0 > > local_group_id=10 \ > > +-- create Flow_Sample_Collector_Set id=2 bridge=@br0 > > local_group_id=12], > > + [0], [ignore]) > > + > > +AT_DATA([flows.txt], [dnl > > +priority=100,in_port=ovs-p0,ip6,icmp6,icmpv6_type=128 > > actions=SAMPLE_ACTIO
Re: [ovs-dev] [PATCH v1 08/13] ofproto-dpif-lsample: Show stats via unixctl.
On Tue, Jul 09, 2024 at 11:45:58AM GMT, Eelco Chaudron wrote:
> On 7 Jul 2024, at 22:09, Adrian Moreno wrote:
>
> > Add a command to dump statistics per exporter.
>
> Small nit below, rest looks good to me. If this is the only change in the
> next series you can add my ACK.
>
> //Eelco
>
> > Signed-off-by: Adrian Moreno
> > ---
> > NEWS | 2 +
> > ofproto/ofproto-dpif-lsample.c | 111 +
> > ofproto/ofproto-dpif-lsample.h | 1 +
> > ofproto/ofproto-dpif.c | 1 +
> > 4 files changed, 115 insertions(+)
> >
> > diff --git a/NEWS b/NEWS
> > index 15faf9fc3..1c53badea 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -20,6 +20,8 @@ Post-v3.3.0
> > allows samples to be emitted locally (instead of via IPFIX) in a
> > datapath-specific manner. The Linux kernel datapath is the first to
> > support this feature by using the new datapath "psample" action.
> > + A new unixctl command 'lsample/show' shows packet and bytes statistics
> > + per local sample exporter.
> >
> >
> > v3.3.0 - 16 Feb 2024
> > diff --git a/ofproto/ofproto-dpif-lsample.c b/ofproto/ofproto-dpif-lsample.c
> > index 171129d5b..82a87c27d 100644
> > --- a/ofproto/ofproto-dpif-lsample.c
> > +++ b/ofproto/ofproto-dpif-lsample.c
> > @@ -21,7 +21,10 @@
> > #include "dpif.h"
> > #include "hash.h"
> > #include "ofproto.h"
> > +#include "ofproto-dpif.h"
> > +#include "openvswitch/dynamic-string.h"
> > #include "openvswitch/thread.h"
> > +#include "unixctl.h"
> >
> > /* Dpif local sampling.
> > *
> > @@ -219,3 +222,111 @@ dpif_lsample_unref(struct dpif_lsample *lsample)
> > dpif_lsample_destroy(lsample);
> > }
> > }
> > +
> > +static int
> > +comp_exporter_collector_id(const void *a_, const void *b_)
> > +{
> > +const struct lsample_exporter_node *a, *b;
> > +
> > +a = *(struct lsample_exporter_node **) a_;
> > +b = *(struct lsample_exporter_node **) b_;
> > +
> > +if (a->exporter.options.collector_set_id >
> > +b->exporter.options.collector_set_id) {
> > +return 1;
> > +}
> > +if (a->exporter.options.collector_set_id <
> > +b->exporter.options.collector_set_id) {
> > +return -1;
> > +}
> > +return 0;
> > +}
> > +
> > +static void
> > +lsample_exporter_list(struct dpif_lsample *lsample,
> > + struct lsample_exporter_node ***list,
> > + size_t *num_exporters)
> > +{
> > +struct lsample_exporter_node **exporter_list;
> > +struct lsample_exporter_node *node;
> > +size_t k = 0, n;
> > +
> > +n = cmap_count(&lsample->exporters);
> > +
> > +exporter_list = xcalloc(n, sizeof *exporter_list);
> > +
> > +CMAP_FOR_EACH (node, node, &lsample->exporters) {
> > +if (k >= n) {
> > +break;
> > +}
> > +exporter_list[k++] = node;
> > +}
> > +
> > +qsort(exporter_list, k, sizeof *exporter_list,
> > comp_exporter_collector_id);
> > +
> > +*list = exporter_list;
> > +*num_exporters = k;
> > +}
> > +
> > +static void
> > +lsample_unixctl_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
> > + const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
> > +{
> > +struct lsample_exporter_node **node_list = NULL;
> > +struct ds ds = DS_EMPTY_INITIALIZER;
> > +const struct ofproto_dpif *ofproto;
> > +size_t i, num;
> > +
> > +ofproto = ofproto_dpif_lookup_by_name(argv[1]);
> > +if (!ofproto) {
> > +unixctl_command_reply_error(conn, "no such bridge");
> > +return;
> > +}
> > +
> > +if (!ofproto->lsample) {
> > +unixctl_command_reply_error(conn,
> > +"no local sampling exporters
> > configured");
> > +return;
> > +}
> > +
> > +ds_put_format(&ds, "Local sample statistics for bridge \"%s\":\n",
> > + argv[1]);
> > +
> > +lsample_exporter_list(ofproto->lsample, &node_list, &num);
> > +
> > +for (i = 0; i < num; i++) {
> > +uint64_t n_bytes;
> > +uint64_t n_packets;
> > +
> > +struct lsample_exporter_node *node = node_list[i];
> > +
> > +atomic_read_relaxed(&node->exporter.n_packets, &n_packets);
> > +atomic_read_relaxed(&node->exporter.n_bytes, &n_bytes);
> > +
> > +if (i) {
> > +ds_put_cstr(&ds, "\n");
> > +}
> > +
> > +ds_put_format(&ds, "Collector Set ID: %"PRIu32":\n",
> > +node->exporter.options.collector_set_id);
> > +ds_put_format(&ds, " Group ID : %"PRIu32"\n",
> > +node->exporter.options.group_id);
> > +ds_put_format(&ds, " Total packets: %"PRIu64"\n", n_packets);
> > +ds_put_format(&ds, " Total bytes : %"PRIu64"\n", n_bytes);
> > +}
> > +
> > +free(node_list);
> > +unixctl_command_reply(conn, ds_cstr(&ds));
> > +ds_destroy(&ds);
> > +}
> > +
> > +void dpif_lsample_init(void)
Re: [ovs-dev] [PATCH v1 07/13] ofproto-dpif-xlate-cache: Add lsample to xcache.
On Tue, Jul 09, 2024 at 11:45:54AM GMT, Eelco Chaudron wrote:
> On 7 Jul 2024, at 22:08, Adrian Moreno wrote:
>
> > Add a cache entry type for local sample objects.
> > Store both the dpif_lsample reference and the collector_set_id so we can
> > quickly find the particular exporter.
> >
> > Using this mechanism, account for packet and byte statistics.
>
> Small nit below, rest looks good to me. If this is the only change in the
> next series you can add my ACK.
>
> //Eelco
>
> > Signed-off-by: Adrian Moreno
> > ---
> > ofproto/ofproto-dpif-lsample.c | 18 ++
> > ofproto/ofproto-dpif-lsample.h | 4
> > ofproto/ofproto-dpif-xlate-cache.c | 11 ++-
> > ofproto/ofproto-dpif-xlate-cache.h | 6 ++
> > ofproto/ofproto-dpif-xlate.c | 14 ++
> > ofproto/ofproto-dpif.c | 1 +
> > 6 files changed, 53 insertions(+), 1 deletion(-)
> >
> > diff --git a/ofproto/ofproto-dpif-lsample.c b/ofproto/ofproto-dpif-lsample.c
> > index 534ad96f0..171129d5b 100644
> > --- a/ofproto/ofproto-dpif-lsample.c
> > +++ b/ofproto/ofproto-dpif-lsample.c
> > @@ -18,6 +18,7 @@
> > #include "ofproto-dpif-lsample.h"
> >
> > #include "cmap.h"
> > +#include "dpif.h"
> > #include "hash.h"
> > #include "ofproto.h"
> > #include "openvswitch/thread.h"
> > @@ -44,6 +45,8 @@ struct dpif_lsample {
> >
> > struct lsample_exporter {
> > struct ofproto_lsample_options options;
> > +atomic_uint64_t n_packets;
> > +atomic_uint64_t n_bytes;
> > };
> >
> > struct lsample_exporter_node {
> > @@ -156,6 +159,21 @@ dpif_lsample_get_group_id(struct dpif_lsample *ps,
> > uint32_t collector_set_id,
> > return found;
> > }
> >
> > +void
> > +dpif_lsample_credit_stats(struct dpif_lsample *lsample,
> > + uint32_t collector_set_id,
> > + const struct dpif_flow_stats *stats)
> > +{
> > +struct lsample_exporter_node *node;
> > +uint64_t orig;
> > +
> > +node = dpif_lsample_find_exporter_node(lsample, collector_set_id);
> > +if (node) {
> > +atomic_add_relaxed(&node->exporter.n_packets, stats->n_packets,
> > &orig);
> > +atomic_add_relaxed(&node->exporter.n_bytes, stats->n_bytes, &orig);
> > +}
> > +}
> > +
> > struct dpif_lsample *
> > dpif_lsample_create(void)
> > {
> > diff --git a/ofproto/ofproto-dpif-lsample.h b/ofproto/ofproto-dpif-lsample.h
> > index 9c1026551..2666e5478 100644
> > --- a/ofproto/ofproto-dpif-lsample.h
> > +++ b/ofproto/ofproto-dpif-lsample.h
> > @@ -23,6 +23,7 @@
> >
> > struct dpif_lsample;
> > struct ofproto_lsample_options;
> > +struct dpif_flow_stats;
> >
> > struct dpif_lsample *dpif_lsample_create(void);
> >
> > @@ -38,4 +39,7 @@ bool dpif_lsample_get_group_id(struct dpif_lsample *,
> > uint32_t collector_set_id,
> > uint32_t *group_id);
> >
> > +void dpif_lsample_credit_stats(struct dpif_lsample *,
> > + uint32_t collector_set_id,
> > + const struct dpif_flow_stats *);
> > #endif /* OFPROTO_DPIF_LSAMPLE_H */
> > diff --git a/ofproto/ofproto-dpif-xlate-cache.c
> > b/ofproto/ofproto-dpif-xlate-cache.c
> > index 2e1fcb3a6..73d79bfc7 100644
> > --- a/ofproto/ofproto-dpif-xlate-cache.c
> > +++ b/ofproto/ofproto-dpif-xlate-cache.c
> > @@ -35,9 +35,10 @@
> > #include "learn.h"
> > #include "mac-learning.h"
> > #include "netdev-vport.h"
> > +#include "ofproto/ofproto-dpif.h"
> > +#include "ofproto/ofproto-dpif-lsample.h"
> > #include "ofproto/ofproto-dpif-mirror.h"
> > #include "ofproto/ofproto-dpif-xlate.h"
> > -#include "ofproto/ofproto-dpif.h"
> > #include "ofproto/ofproto-provider.h"
> > #include "openvswitch/dynamic-string.h"
> > #include "openvswitch/vlog.h"
> > @@ -162,6 +163,11 @@ xlate_push_stats_entry(struct xc_entry *entry,
> > }
> >
> > break;
> > +case XC_LSAMPLE:
> > +dpif_lsample_credit_stats(entry->lsample.lsample,
> > + entry->lsample.collector_set_id,
> > + stats);
> > +break;
> > default:
> > OVS_NOT_REACHED();
> > }
> > @@ -245,6 +251,9 @@ xlate_cache_clear_entry(struct xc_entry *entry)
> > break;
> > case XC_TUNNEL_HEADER:
> > break;
> > +case XC_LSAMPLE:
> > +dpif_lsample_unref(entry->lsample.lsample);
> > +break;
> > default:
> > OVS_NOT_REACHED();
> > }
> > diff --git a/ofproto/ofproto-dpif-xlate-cache.h
> > b/ofproto/ofproto-dpif-xlate-cache.h
> > index 0fc6d2ea6..df8115419 100644
> > --- a/ofproto/ofproto-dpif-xlate-cache.h
> > +++ b/ofproto/ofproto-dpif-xlate-cache.h
> > @@ -29,6 +29,7 @@
> > struct bfd;
> > struct bond;
> > struct dpif_flow_stats;
> > +struct dpif_lsample;
> > struct flow;
> > struct group_dpif;
> > struct mbridge;
> > @@ -53,6 +54,7 @@ enum xc_type {
> > XC_GROUP,
> > XC_TNL_NEIGH,
>
Re: [ovs-dev] [PATCH v1 06/13] ofproto-dpif-xlate: Use psample for local sample.
On Tue, Jul 09, 2024 at 11:45:51AM GMT, Eelco Chaudron wrote:
> On 7 Jul 2024, at 22:08, Adrian Moreno wrote:
>
> > Use the newly added psample action to implement OpenFlow sample() actions
> > with local sampling configuration if possible.
> >
> > A bit of refactoring in compose_sample_actions arguments helps make it a
> > bit more readable.
>
> Some comments below.
>
> Cheers,
>
> Eelco
>
> > Signed-off-by: Adrian Moreno
> > ---
> > ofproto/ofproto-dpif-lsample.c | 16 +++
> > ofproto/ofproto-dpif-lsample.h | 5 +
> > ofproto/ofproto-dpif-xlate.c | 251 +++--
> > ofproto/ofproto-dpif-xlate.h | 5 +-
> > ofproto/ofproto-dpif.c | 2 +-
> > tests/ofproto-dpif.at | 146 +++
> > 6 files changed, 345 insertions(+), 80 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif-lsample.c b/ofproto/ofproto-dpif-lsample.c
> > index d675a116f..534ad96f0 100644
> > --- a/ofproto/ofproto-dpif-lsample.c
> > +++ b/ofproto/ofproto-dpif-lsample.c
> > @@ -140,6 +140,22 @@ dpif_lsample_set_options(struct dpif_lsample *lsample,
> > return changed;
> > }
> >
> > +/* Returns the group_id for a given collector_set_id, if it exists. */
> > +bool
> > +dpif_lsample_get_group_id(struct dpif_lsample *ps, uint32_t
> > collector_set_id,
> > + uint32_t *group_id)
> > +{
> > +struct lsample_exporter_node *node;
> > +bool found = false;
> > +
> > +node = dpif_lsample_find_exporter_node(ps, collector_set_id);
> > +if (node) {
> > +found = true;
> > +*group_id = node->exporter.options.group_id;
> > +}
> > +return found;
> > +}
> > +
> > struct dpif_lsample *
> > dpif_lsample_create(void)
> > {
> > diff --git a/ofproto/ofproto-dpif-lsample.h b/ofproto/ofproto-dpif-lsample.h
> > index bee36c9c5..9c1026551 100644
> > --- a/ofproto/ofproto-dpif-lsample.h
> > +++ b/ofproto/ofproto-dpif-lsample.h
> > @@ -18,6 +18,7 @@
> > #define OFPROTO_DPIF_LSAMPLE_H 1
> >
> > #include
> > +#include
> > #include
> >
> > struct dpif_lsample;
> > @@ -33,4 +34,8 @@ bool dpif_lsample_set_options(struct dpif_lsample *,
> >const struct ofproto_lsample_options *,
> >size_t n_opts);
> >
> > +bool dpif_lsample_get_group_id(struct dpif_lsample *,
> > + uint32_t collector_set_id,
> > + uint32_t *group_id);
> > +
> > #endif /* OFPROTO_DPIF_LSAMPLE_H */
> > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> > index 7c4950895..5e8113d5e 100644
> > --- a/ofproto/ofproto-dpif-xlate.c
> > +++ b/ofproto/ofproto-dpif-xlate.c
> > @@ -47,6 +47,7 @@
> > #include "ofproto/ofproto-dpif-ipfix.h"
> > #include "ofproto/ofproto-dpif-mirror.h"
> > #include "ofproto/ofproto-dpif-monitor.h"
> > +#include "ofproto/ofproto-dpif-lsample.h"
>
> Add in alphabetical order?
Ack.
>
> > #include "ofproto/ofproto-dpif-sflow.h"
> > #include "ofproto/ofproto-dpif-trace.h"
> > #include "ofproto/ofproto-dpif-xlate-cache.h"
> > @@ -117,6 +118,7 @@ struct xbridge {
> > struct dpif_sflow *sflow; /* SFlow handle, or null. */
> > struct dpif_ipfix *ipfix; /* Ipfix handle, or null. */
> > struct netflow *netflow; /* Netflow handle, or null. */
> > +struct dpif_lsample *lsample; /* Local sample handle, or null. */
>
> I would move above netflow, as you also do the actual init/unref before
> netflow.
Ack.
>
> > struct stp *stp; /* STP or null if disabled. */
> > struct rstp *rstp;/* RSTP or null if disabled. */
> >
> > @@ -686,6 +688,7 @@ static void xlate_xbridge_set(struct xbridge *, struct
> > dpif *,
> >const struct mbridge *,
> >const struct dpif_sflow *,
> >const struct dpif_ipfix *,
> > + const struct dpif_lsample *,
> >const struct netflow *,
> >bool forward_bpdu, bool has_in_band,
> >const struct dpif_backer_support *,
> > @@ -1069,6 +1072,7 @@ xlate_xbridge_set(struct xbridge *xbridge,
> >const struct mbridge *mbridge,
> >const struct dpif_sflow *sflow,
> >const struct dpif_ipfix *ipfix,
> > + const struct dpif_lsample *lsample,
> >const struct netflow *netflow,
> >bool forward_bpdu, bool has_in_band,
> >const struct dpif_backer_support *support,
> > @@ -1099,6 +1103,11 @@ xlate_xbridge_set(struct xbridge *xbridge,
> > xbridge->ipfix = dpif_ipfix_ref(ipfix);
> > }
> >
> > +if (xbridge->lsample != lsample) {
> > +dpif_lsample_unref(xbridge->lsample);
> > +xbridge->lsample = dpif_lsample_ref(lsample);
> > +}
> > +
> > if (xbridge->stp
Re: [ovs-dev] [PATCH v1 05/13] vswitchd: Add local sampling to vswitchd schema.
On Tue, Jul 09, 2024 at 11:45:35AM GMT, Eelco Chaudron wrote:
> On 7 Jul 2024, at 22:08, Adrian Moreno wrote:
>
> > Add as new column in the Flow_Sample_Collector_Set table named
> > "local_group_id" which enables this feature.
>
> Small nit below, if fixed add my ACK to the next revision.
>
> //Eelco
>
> > Signed-off-by: Adrian Moreno
> > ---
> > NEWS | 4 ++
> > vswitchd/bridge.c | 78 +++---
> > vswitchd/vswitch.ovsschema | 9 -
> > vswitchd/vswitch.xml | 40 +--
> > 4 files changed, 120 insertions(+), 11 deletions(-)
> >
> > diff --git a/NEWS b/NEWS
> > index e0359b759..15faf9fc3 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -16,6 +16,10 @@ Post-v3.3.0
> > per interface 'options:dpdk-lsc-interrupt' to 'false'.
> > - Python:
> > * Added custom transaction support to the Idl via add_op().
> > + - Local sampling is introduced. It reuses the OpenFlow sample action and
> > + allows samples to be emitted locally (instead of via IPFIX) in a
> > + datapath-specific manner. The Linux kernel datapath is the first to
> > + support this feature by using the new datapath "psample" action.
> >
> >
> > v3.3.0 - 16 Feb 2024
> > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> > index 95a65fcdc..b7db681f3 100644
> > --- a/vswitchd/bridge.c
> > +++ b/vswitchd/bridge.c
> > @@ -288,6 +288,7 @@ static void bridge_configure_mac_table(struct bridge *);
> > static void bridge_configure_mcast_snooping(struct bridge *);
> > static void bridge_configure_sflow(struct bridge *, int
> > *sflow_bridge_number);
> > static void bridge_configure_ipfix(struct bridge *);
> > +static void bridge_configure_lsample(struct bridge *);
> > static void bridge_configure_spanning_tree(struct bridge *);
> > static void bridge_configure_tables(struct bridge *);
> > static void bridge_configure_dp_desc(struct bridge *);
> > @@ -989,6 +990,7 @@ bridge_reconfigure(const struct ovsrec_open_vswitch
> > *ovs_cfg)
> > bridge_configure_netflow(br);
> > bridge_configure_sflow(br, &sflow_bridge_number);
> > bridge_configure_ipfix(br);
> > +bridge_configure_lsample(br);
> > bridge_configure_spanning_tree(br);
> > bridge_configure_tables(br);
> > bridge_configure_dp_desc(br);
> > @@ -1537,10 +1539,11 @@ ovsrec_ipfix_is_valid(const struct ovsrec_ipfix
> > *ipfix)
> > return ipfix && ipfix->n_targets > 0;
> > }
> >
> > -/* Returns whether a Flow_Sample_Collector_Set row is valid. */
> > +/* Returns whether a Flow_Sample_Collector_Set row contains a valid IPFIX
> > + * configuration. */
> > static bool
> > -ovsrec_fscs_is_valid(const struct ovsrec_flow_sample_collector_set *fscs,
> > - const struct bridge *br)
> > +ovsrec_fscs_is_valid_ipfix(const struct ovsrec_flow_sample_collector_set
> > *fscs,
> > + const struct bridge *br)
> > {
> > return ovsrec_ipfix_is_valid(fscs->ipfix) && fscs->bridge == br->cfg;
> > }
> > @@ -1558,7 +1561,7 @@ bridge_configure_ipfix(struct bridge *br)
> > const char *virtual_obs_id;
> >
> > OVSREC_FLOW_SAMPLE_COLLECTOR_SET_FOR_EACH(fe_cfg, idl) {
> > -if (ovsrec_fscs_is_valid(fe_cfg, br)) {
> > +if (ovsrec_fscs_is_valid_ipfix(fe_cfg, br)) {
> > n_fe_opts++;
> > }
> > }
> > @@ -1621,7 +1624,7 @@ bridge_configure_ipfix(struct bridge *br)
> > fe_opts = xcalloc(n_fe_opts, sizeof *fe_opts);
> > opts = fe_opts;
> > OVSREC_FLOW_SAMPLE_COLLECTOR_SET_FOR_EACH(fe_cfg, idl) {
> > -if (ovsrec_fscs_is_valid(fe_cfg, br)) {
> > +if (ovsrec_fscs_is_valid_ipfix(fe_cfg, br)) {
> > opts->collector_set_id = fe_cfg->id;
> > sset_init(&opts->targets);
> > sset_add_array(&opts->targets, fe_cfg->ipfix->targets,
> > @@ -1667,6 +1670,71 @@ bridge_configure_ipfix(struct bridge *br)
> > }
> > }
> >
> > +/* Returns whether a Flow_Sample_Collector_Set row contains a valid local
> > + * sampling configuration. */
> > +static bool
> > +ovsrec_fscs_is_valid_local(const struct ovsrec_flow_sample_collector_set
> > *fscs,
> > + const struct bridge *br)
> > +{
> > +return fscs->local_group_id && fscs->n_local_group_id == 1 &&
> > + fscs->bridge == br->cfg;
> > +}
> > +
> > +/* Set local sample configuration on 'br'. */
> > +static void
> > +bridge_configure_lsample(struct bridge *br)
> > +{
> > +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> > +const struct ovsrec_flow_sample_collector_set *fscs;
> > +struct ofproto_lsample_options *opts_array, *opts;
> > +size_t n_opts = 0;
> > +int ret;
> > +
> > +/* Iterate the Flow_Sample_Collector_Set table twice.
> > + * First to get the number of valid configuration entries, then to
> > process
> > + * each of them and build an
Re: [ovs-dev] [PATCH v1 04/13] ofproto: Add ofproto-dpif-lsample.
On Tue, Jul 09, 2024 at 11:45:19AM GMT, Eelco Chaudron wrote:
> On 7 Jul 2024, at 22:08, Adrian Moreno wrote:
>
> > Add a new resource in ofproto-dpif and the corresponding API in
> > ofproto_provider.h to represent and local sampling configuration.
> >
> > Signed-off-by: Adrian Moreno
>
> Some small nits below.
>
> //Eelco
>
> > ---
> > ofproto/automake.mk| 2 +
> > ofproto/ofproto-dpif-lsample.c | 187 +
> > ofproto/ofproto-dpif-lsample.h | 36 +++
> > ofproto/ofproto-dpif.c | 38 +++
> > ofproto/ofproto-dpif.h | 1 +
> > ofproto/ofproto-provider.h | 9 ++
> > ofproto/ofproto.c | 12 +++
> > ofproto/ofproto.h | 8 ++
> > 8 files changed, 293 insertions(+)
> > create mode 100644 ofproto/ofproto-dpif-lsample.c
> > create mode 100644 ofproto/ofproto-dpif-lsample.h
> >
> > diff --git a/ofproto/automake.mk b/ofproto/automake.mk
> > index 7c08b563b..cb1361b8a 100644
> > --- a/ofproto/automake.mk
> > +++ b/ofproto/automake.mk
> > @@ -30,6 +30,8 @@ ofproto_libofproto_la_SOURCES = \
> > ofproto/ofproto-dpif.h \
> > ofproto/ofproto-dpif-ipfix.c \
> > ofproto/ofproto-dpif-ipfix.h \
> > + ofproto/ofproto-dpif-lsample.c \
> > + ofproto/ofproto-dpif-lsample.h \
> > ofproto/ofproto-dpif-mirror.c \
> > ofproto/ofproto-dpif-mirror.h \
> > ofproto/ofproto-dpif-monitor.c \
> > diff --git a/ofproto/ofproto-dpif-lsample.c b/ofproto/ofproto-dpif-lsample.c
> > new file mode 100644
> > index 0..d675a116f
> > --- /dev/null
> > +++ b/ofproto/ofproto-dpif-lsample.c
> > @@ -0,0 +1,187 @@
> > +/*
> > + * Copyright (c) 2024 Red Hat, Inc.
> > + *
> > + * Licensed under the Apache License, Version 2.0 (the "License");
> > + * you may not use this file except in compliance with the License.
> > + * You may obtain a copy of the License at:
> > + *
> > + * http://www.apache.org/licenses/LICENSE-2.0
> > + *
> > + * Unless required by applicable law or agreed to in writing, software
> > + * distributed under the License is distributed on an "AS IS" BASIS,
> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> > + * See the License for the specific language governing permissions and
> > + * limitations under the License.
> > + */
> > +
> > +#include
> > +#include "ofproto-dpif-lsample.h"
> > +
> > +#include "cmap.h"
> > +#include "hash.h"
> > +#include "ofproto.h"
> > +#include "openvswitch/thread.h"
> > +
> > +/* Dpif local sampling.
> > + *
> > + * Thread safety: dpif_lsample allows lockless concurrent reads of local
> > + * sampling exporters as long as the following restrictions are met:
> > + * 1) While the last reference is being dropped, i.e: a thread is calling
> > + * "dpif_lsample_unref" on the last reference, other threads cannot
> > call
> > + * "dpif_lsample_ref".
> > + * 2) Threads do not quiese while holding references to internal
> > + * lsample_exporter objects.
> > + */
> > +
> > +struct dpif_lsample {
> > +struct cmap exporters; /* Contains lsample_exporter_node
> > instances
> > + indexed by collector_set_id. */
> > +struct ovs_mutex mutex; /* Protects concurrent
> > insertion/deletion
> > + * of exporters. */
> > +
> > +struct ovs_refcount ref_cnt;/* Controls references to this
> > instance. */
> > +};
> > +
> > +struct lsample_exporter {
> > +struct ofproto_lsample_options options;
> > +};
> > +
> > +struct lsample_exporter_node {
> > +struct cmap_node node; /* In dpif_lsample->exporters. */
> > +struct lsample_exporter exporter;
> > +};
> > +
> > +static void
> > +dpif_lsample_delete_exporter(struct dpif_lsample *lsample,
> > + struct lsample_exporter_node *node)
> > +{
> > +ovs_mutex_lock(&lsample->mutex);
> > +cmap_remove(&lsample->exporters, &node->node,
> > +hash_int(node->exporter.options.collector_set_id, 0));
> > +ovs_mutex_unlock(&lsample->mutex);
> > +
> > +ovsrcu_postpone(free, node);
> > +}
> > +
> > +/* Adds an exporter with the provided options which are copied. */
> > +static struct lsample_exporter_node *
> > +dpif_lsample_add_exporter(struct dpif_lsample *lsample,
> > + const struct ofproto_lsample_options *options)
> > +{
> > +struct lsample_exporter_node *node;
> > +
> > +node = xzalloc(sizeof *node);
> > +node->exporter.options = *options;
> > +
> > +ovs_mutex_lock(&lsample->mutex);
> > +cmap_insert(&lsample->exporters, &node->node,
> > +hash_int(options->collector_set_id, 0));
> > +ovs_mutex_unlock(&lsample->mutex);
> > +
> > +return node;
> > +}
> > +
> > +static struct lsample_exporter_node *
> > +dpif_lsample_find_exporter_node(const struct dpif_lsample *lsample,
> > +const uint32_t collector_set_id)
> > +{
Re: [ovs-dev] [PATCH v1 03/13] ofproto_dpif: Check for psample support.
On Tue, Jul 09, 2024 at 11:45:17AM GMT, Eelco Chaudron wrote:
> On 4 Jul 2024, at 9:52, Adrian Moreno wrote:
>
> > Only kernel datapath supports this action so add a function in dpif.c
> > that checks for that.
>
> One small nit below, rest looks good. If this is the only change in the
> next series you can add my ACK.
>
> //Eelco
>
> > Signed-off-by: Adrian Moreno
> > ---
> > lib/dpif.c | 7 +++
> > lib/dpif.h | 1 +
> > ofproto/ofproto-dpif.c | 43 ++
> > ofproto/ofproto-dpif.h | 7 ++-
> > 4 files changed, 57 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/dpif.c b/lib/dpif.c
> > index 71728badc..0a964ba89 100644
> > --- a/lib/dpif.c
> > +++ b/lib/dpif.c
> > @@ -1953,6 +1953,13 @@ dpif_supports_lb_output_action(const struct dpif
> > *dpif)
> > return dpif_is_netdev(dpif);
> > }
> >
> > +bool
> > +dpif_may_support_psample(const struct dpif *dpif)
> > +{
> > +/* Userspace datapath does not support this action. */
> > +return !dpif_is_netdev(dpif);
> > +}
> > +
> > /* Meters */
> > void
> > dpif_meter_get_features(const struct dpif *dpif,
> > diff --git a/lib/dpif.h b/lib/dpif.h
> > index a764e8a59..6bef7d5b3 100644
> > --- a/lib/dpif.h
> > +++ b/lib/dpif.h
> > @@ -941,6 +941,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_may_support_explicit_drop_action(const struct dpif *);
> > +bool dpif_may_support_psample(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 4712d10a8..94c84d697 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -873,6 +873,12 @@ ovs_lb_output_action_supported(struct ofproto_dpif
> > *ofproto)
> > return ofproto->backer->rt_support.lb_output_action;
> > }
> >
> > +bool
> > +ovs_psample_supported(struct ofproto_dpif *ofproto)
> > +{
> > +return ofproto->backer->rt_support.psample;
> > +}
> > +
> > /* Tests whether 'backer''s datapath supports recirculation. Only newer
> > * datapaths support OVS_KEY_ATTR_RECIRC_ID in keys. We need to disable
> > some
> > * features on older datapaths that don't support this feature.
> > @@ -1609,6 +1615,42 @@ check_add_mpls(struct dpif_backer *backer)
> > return supported;
> > }
> >
> > +/* Tests whether 'backer''s datapath supports the OVS_ACTION_ATTR_PSAMPLE
> > + * action. */
> > +static bool
> > +check_psample(struct dpif_backer *backer)
> > +{
> > +uint8_t cookie[OVS_PSAMPLE_COOKIE_MAX_SIZE];
> > +struct odputil_keybuf keybuf;
> > +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_init(&actions, 64);
> > +
> > +/* Generate a random max-size cookie. */
> > +random_bytes(cookie, sizeof(cookie));
> > +
> > +odp_put_psample_action(&actions, 10, cookie, sizeof cookie);
> > +
> > +supported = dpif_may_support_psample(backer->dpif) &&
> > +dpif_probe_feature(backer->dpif, "psample", &key, &actions, NULL);
> > +
> > +ofpbuf_uninit(&actions);
> > +VLOG_INFO("%s: Datapath %s psample", dpif_name(backer->dpif),
> > + supported ? "supports" : "does not support");
> > +return supported;
> > +}
> > +
> > #define CHECK_FEATURE__(NAME, SUPPORT, FIELD, VALUE, ETHTYPE)
> > \
> > static bool
> > \
> > check_##NAME(struct dpif_backer *backer)
> > \
> > @@ -1698,6 +1740,7 @@ check_support(struct dpif_backer *backer)
> > dpif_supports_lb_output_action(backer->dpif);
> > backer->rt_support.ct_zero_snat = dpif_supports_ct_zero_snat(backer);
> > backer->rt_support.add_mpls = check_add_mpls(backer);
> > +backer->rt_support.psample = check_psample(backer);
> >
> > /* Flow fields. */
> > backer->rt_support.odp.ct_state = check_ct_state(backer);
> > diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
> > index d33f73df8..bc7a19dab 100644
> > --- a/ofproto/ofproto-dpif.h
> > +++ b/ofproto/ofproto-dpif.h
> > @@ -213,7 +213,10 @@ struct group_dpif *group_dpif_lookup(struct
> > ofproto_dpif *,
> > DPIF_SUPPORT_FIELD(bool, ct_zero_snat, "Conntrack all-zero IP SNAT")
> > \
> >
> > \
> > /* True if the datapath supports add_mpls action. */
> > \
> > -DPIF_SUPPORT_FIELD(bool, add_mp
Re: [ovs-dev] [PATCH v1 02/13] odp-util: Add support OVS_ACTION_ATTR_PSAMPLE.
On Tue, Jul 09, 2024 at 11:45:15AM GMT, Eelco Chaudron wrote:
> On 4 Jul 2024, at 9:52, Adrian Moreno wrote:
>
> > Add support for parsing and formatting the new action.
> >
> > Also, flag OVS_ACTION_ATTR_SAMPLE as requiring datapath assistance if it
> > contains a nested OVS_ACTION_ATTR_PSAMPLE. The reason is that the
> > sampling rate form the parent "sample" is made available to the nested
>
> form -> from
>
> > "psample" by the kernel.
>
> Two small comments below, the rest looks good.
>
> > Signed-off-by: Adrian Moreno
> > ---
> > include/linux/openvswitch.h | 28 +++
> > lib/dpif-netdev.c| 1 +
> > lib/dpif.c | 1 +
> > lib/odp-execute.c| 25 +-
> > lib/odp-util.c | 93
> > lib/odp-util.h | 3 ++
> > ofproto/ofproto-dpif-ipfix.c | 1 +
> > ofproto/ofproto-dpif-sflow.c | 1 +
> > python/ovs/flow/odp.py | 8
> > tests/odp.at | 16 +++
> > 10 files changed, 176 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
> > index d9fb991ef..0023b65fb 100644
> > --- a/include/linux/openvswitch.h
> > +++ b/include/linux/openvswitch.h
> > @@ -992,6 +992,31 @@ struct check_pkt_len_arg {
> > };
> > #endif
> >
> > +#define OVS_PSAMPLE_COOKIE_MAX_SIZE 16
> > +/**
> > + * enum ovs_pample_attr - Attributes for %OVS_ACTION_ATTR_PSAMPLE
> > + * action.
> > + *
> > + * @OVS_PSAMPLE_ATTR_GROUP: 32-bit number to identify the source of the
> > + * sample.
> > + * @OVS_PSAMPLE_ATTR_COOKIE: An optional variable-length binary cookie that
> > + * contains user-defined metadata. The maximum length is
> > + * OVS_PSAMPLE_COOKIE_MAX_SIZE bytes.
> > + *
> > + * Sends the packet to the psample multicast group with the specified
> > group and
> > + * cookie. It is possible to combine this action with the
> > + * %OVS_ACTION_ATTR_TRUNC action to limit the size of the sample.
> > + */
> > +enum ovs_psample_attr {
> > +OVS_PSAMPLE_ATTR_GROUP = 1,/* u32 number. */
> > +OVS_PSAMPLE_ATTR_COOKIE, /* Optional, user specified cookie.
> > */
> > +
> > +/* private: */
>
> None of the other definitions have this private marking, so I see no need to
> start adding it here.
OK. The uAPI file has it (requested by netdev maintainers) but I guess
it's kernel-specific and this file is not a blind copy of the uAPI
anyways so I think we can remove it. I'll do on the next version.
>
> > +__OVS_PSAMPLE_ATTR_MAX
> > +};
> > +
> > +#define OVS_PSAMPLE_ATTR_MAX (__OVS_PSAMPLE_ATTR_MAX - 1)
> > +
> > /**
> > * enum ovs_action_attr - Action types.
> > *
> > @@ -1056,6 +1081,8 @@ struct check_pkt_len_arg {
> > * of l3 tunnel flag in the tun_flags field of OVS_ACTION_ATTR_ADD_MPLS
> > * argument.
> > * @OVS_ACTION_ATTR_DROP: Explicit drop action.
> > + * @OVS_ACTION_ATTR_PSAMPLE: Send a sample of the packet to external
> > observers
> > + * via psample.
> > */
> >
> > enum ovs_action_attr {
> > @@ -1087,6 +1114,7 @@ enum ovs_action_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. */
> > + OVS_ACTION_ATTR_PSAMPLE, /* Nested OVS_PSAMPLE_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 c7f9e1490..f0594e5f5 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -9519,6 +9519,7 @@ dp_execute_cb(void *aux_, struct dp_packet_batch
> > *packets_,
> > case OVS_ACTION_ATTR_DROP:
> > case OVS_ACTION_ATTR_ADD_MPLS:
> > case OVS_ACTION_ATTR_DEC_TTL:
> > +case OVS_ACTION_ATTR_PSAMPLE:
> > case __OVS_ACTION_ATTR_MAX:
> > OVS_NOT_REACHED();
> > }
> > diff --git a/lib/dpif.c b/lib/dpif.c
> > index 23eb18495..71728badc 100644
> > --- a/lib/dpif.c
> > +++ b/lib/dpif.c
> > @@ -1192,6 +1192,7 @@ dpif_execute_helper_cb(void *aux_, struct
> > dp_packet_batch *packets_,
> > case OVS_ACTION_ATTR_TUNNEL_PUSH:
> > case OVS_ACTION_ATTR_TUNNEL_POP:
> > case OVS_ACTION_ATTR_USERSPACE:
> > +case OVS_ACTION_ATTR_PSAMPLE:
> > case OVS_ACTION_ATTR_RECIRC: {
> > struct dpif_execute execute;
> > struct ofpbuf execute_actions;
> > diff --git a/lib/odp-execute.c b/lib/odp-execute.c
> > index 081e4d432..15577d539 100644
> > --- a/lib/odp-execute.c
> > +++ b/lib/odp-execute.c
> > @@ -818,13 +818,13 @@ requires_datapath_assistance(const struct nlattr *a)
> > case OVS_ACTION_ATTR_RECIRC:
> > case OVS_ACTION_ATTR_CT:
> > case OVS_ACTION_ATTR_METER:
> > +case OVS_ACTION_ATTR_PSAMPLE:
> > return true;
> >
> > case OVS_ACTION_ATTR_SET:
> > case OVS_ACTION_ATTR_SET_MASKED:
> > case OVS_ACTION_ATTR_PUSH_VLAN:
>
[ovs-dev] [PATCH v5] netdev-offload-dpdk: Support offload of set dscp action.
Add the "set dscp action" parsing function,
so that the "set dscp action" can be offloaded.
Signed-off-by: Sunyang Wu
---
Documentation/howto/dpdk.rst | 5 +++--
lib/netdev-offload-dpdk.c| 27 ++-
2 files changed, 29 insertions(+), 3 deletions(-)
diff --git a/Documentation/howto/dpdk.rst b/Documentation/howto/dpdk.rst
index 04609b20b..f0e46c95b 100644
--- a/Documentation/howto/dpdk.rst
+++ b/Documentation/howto/dpdk.rst
@@ -395,10 +395,11 @@ Supported actions for hardware offload are:
- Output.
- Drop.
- Modification of Ethernet (mod_dl_src/mod_dl_dst).
-- Modification of IPv4 (mod_nw_src/mod_nw_dst/mod_nw_ttl).
+- Modification of IPv4 (mod_nw_src/mod_nw_dst/mod_nw_ttl/mod_nw_tos).
- Modification of TCP/UDP (mod_tp_src/mod_tp_dst).
- VLAN Push/Pop (push_vlan/pop_vlan).
-- Modification of IPv6 (set_field:->ipv6_src/ipv6_dst/mod_nw_ttl).
+- Modification of IPv6 (set_field:->ipv6_src/ipv6_dst/
+mod_nw_ttl/mod_nw_tos).
- Clone/output (tnl_push and output) for encapsulating over a tunnel.
- Tunnel pop, for packets received on physical ports.
diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
index 623005b1c..5a74c6d19 100644
--- a/lib/netdev-offload-dpdk.c
+++ b/lib/netdev-offload-dpdk.c
@@ -791,6 +791,17 @@ dump_flow_action(struct ds *s, struct ds *s_extra,
ds_put_format(s, "port %"PRIu16" ", ntohs(set_tp->port));
}
ds_put_cstr(s, "/ ");
+} else if (actions->type == RTE_FLOW_ACTION_TYPE_SET_IPV4_DSCP ||
+ actions->type == RTE_FLOW_ACTION_TYPE_SET_IPV6_DSCP) {
+const struct rte_flow_action_set_dscp *set_dscp = actions->conf;
+char *dirstr = actions->type == RTE_FLOW_ACTION_TYPE_SET_IPV4_DSCP
+ ? "set_ipv4_dscp " : "set_ipv6_dscp ";
+
+ds_put_cstr(s, dirstr);
+if (set_dscp) {
+ds_put_format(s, "dscp_value %d ", set_dscp->dscp);
+}
+ds_put_cstr(s, "/ ");
} else if (actions->type == RTE_FLOW_ACTION_TYPE_OF_PUSH_VLAN) {
const struct rte_flow_action_of_push_vlan *of_push_vlan =
actions->conf;
@@ -1836,11 +1847,22 @@ add_set_flow_action__(struct flow_actions *actions,
return 0;
}
if (!is_all_ones(mask, size)) {
-VLOG_DBG_RL(&rl, "Partial mask is not supported");
+if (attr != RTE_FLOW_ACTION_TYPE_SET_IPV4_DSCP ||
+attr != RTE_FLOW_ACTION_TYPE_SET_IPV6_DSCP) {
+if (*(uint8_t *) mask & IP_ECN_MASK) {
+VLOG_DBG_RL(&rl, "ECN hw offload is not supported!");
+} else {
+goto add_action;
+}
+} else {
+VLOG_DBG_RL(&rl, "Partial mask is not supported");
+}
+
return -1;
}
}
+add_action:
spec = xzalloc(size);
memcpy(spec, value, size);
add_flow_action(actions, attr, spec);
@@ -1912,6 +1934,7 @@ parse_set_actions(struct flow_actions *actions,
add_set_flow_action(ipv4_src, RTE_FLOW_ACTION_TYPE_SET_IPV4_SRC);
add_set_flow_action(ipv4_dst, RTE_FLOW_ACTION_TYPE_SET_IPV4_DST);
add_set_flow_action(ipv4_ttl, RTE_FLOW_ACTION_TYPE_SET_TTL);
+add_set_flow_action(ipv4_tos, RTE_FLOW_ACTION_TYPE_SET_IPV4_DSCP);
if (mask && !is_all_zeros(mask, sizeof *mask)) {
VLOG_DBG_RL(&rl, "Unsupported IPv4 set action");
@@ -1924,6 +1947,8 @@ parse_set_actions(struct flow_actions *actions,
add_set_flow_action(ipv6_src, RTE_FLOW_ACTION_TYPE_SET_IPV6_SRC);
add_set_flow_action(ipv6_dst, RTE_FLOW_ACTION_TYPE_SET_IPV6_DST);
add_set_flow_action(ipv6_hlimit, RTE_FLOW_ACTION_TYPE_SET_TTL);
+add_set_flow_action(ipv6_tclass,
+RTE_FLOW_ACTION_TYPE_SET_IPV6_DSCP);
if (mask && !is_all_zeros(mask, sizeof *mask)) {
VLOG_DBG_RL(&rl, "Unsupported IPv6 set action");
--
2.19.0.rc0.windows.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4] netdev-offload-dpdk: Support offload of set dscp action
References: <20240709122112.34864-1-sunyang...@jaguarmicro.com> Bleep bloop. Greetings Sunyang Wu, 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 summary should end with a dot. Subject: netdev-offload-dpdk: Support offload of set dscp action Lines checked: 99, 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 v4] netdev-offload-dpdk: Support offload of set dscp action
Add the "set dscp action" parsing function,
so that the "set dscp action" can be offloaded.
Signed-off-by: Sunyang Wu
---
Documentation/howto/dpdk.rst | 5 +++--
lib/netdev-offload-dpdk.c| 27 ++-
2 files changed, 29 insertions(+), 3 deletions(-)
diff --git a/Documentation/howto/dpdk.rst b/Documentation/howto/dpdk.rst
index 04609b20b..f0e46c95b 100644
--- a/Documentation/howto/dpdk.rst
+++ b/Documentation/howto/dpdk.rst
@@ -395,10 +395,11 @@ Supported actions for hardware offload are:
- Output.
- Drop.
- Modification of Ethernet (mod_dl_src/mod_dl_dst).
-- Modification of IPv4 (mod_nw_src/mod_nw_dst/mod_nw_ttl).
+- Modification of IPv4 (mod_nw_src/mod_nw_dst/mod_nw_ttl/mod_nw_tos).
- Modification of TCP/UDP (mod_tp_src/mod_tp_dst).
- VLAN Push/Pop (push_vlan/pop_vlan).
-- Modification of IPv6 (set_field:->ipv6_src/ipv6_dst/mod_nw_ttl).
+- Modification of IPv6 (set_field:->ipv6_src/ipv6_dst/
+mod_nw_ttl/mod_nw_tos).
- Clone/output (tnl_push and output) for encapsulating over a tunnel.
- Tunnel pop, for packets received on physical ports.
diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
index 623005b1c..5a48da963 100644
--- a/lib/netdev-offload-dpdk.c
+++ b/lib/netdev-offload-dpdk.c
@@ -791,6 +791,17 @@ dump_flow_action(struct ds *s, struct ds *s_extra,
ds_put_format(s, "port %"PRIu16" ", ntohs(set_tp->port));
}
ds_put_cstr(s, "/ ");
+} else if (actions->type == RTE_FLOW_ACTION_TYPE_SET_IPV4_DSCP ||
+ actions->type == RTE_FLOW_ACTION_TYPE_SET_IPV6_DSCP) {
+const struct rte_flow_action_set_dscp *set_dscp = actions->conf;
+char *dirstr = actions->type == RTE_FLOW_ACTION_TYPE_SET_IPV4_DSCP
+ ? "set_ipv4_dscp " : "set_ipv6_dscp ";
+
+ds_put_cstr(s, dirstr);
+if (set_dscp) {
+ds_put_format(s, "dscp_value %d ", set_dscp->dscp);
+}
+ds_put_cstr(s, "/ ");
} else if (actions->type == RTE_FLOW_ACTION_TYPE_OF_PUSH_VLAN) {
const struct rte_flow_action_of_push_vlan *of_push_vlan =
actions->conf;
@@ -1836,11 +1847,22 @@ add_set_flow_action__(struct flow_actions *actions,
return 0;
}
if (!is_all_ones(mask, size)) {
-VLOG_DBG_RL(&rl, "Partial mask is not supported");
+if (attr != RTE_FLOW_ACTION_TYPE_SET_IPV4_DSCP ||
+attr != RTE_FLOW_ACTION_TYPE_SET_IPV6_DSCP) {
+if (mask & IP_ECN_MASK) {
+VLOG_DBG_RL(&rl, "ECN hw offload is not supported!");
+} else {
+goto add_action;
+}
+} else {
+VLOG_DBG_RL(&rl, "Partial mask is not supported");
+}
+
return -1;
}
}
+add_action:
spec = xzalloc(size);
memcpy(spec, value, size);
add_flow_action(actions, attr, spec);
@@ -1912,6 +1934,7 @@ parse_set_actions(struct flow_actions *actions,
add_set_flow_action(ipv4_src, RTE_FLOW_ACTION_TYPE_SET_IPV4_SRC);
add_set_flow_action(ipv4_dst, RTE_FLOW_ACTION_TYPE_SET_IPV4_DST);
add_set_flow_action(ipv4_ttl, RTE_FLOW_ACTION_TYPE_SET_TTL);
+add_set_flow_action(ipv4_tos, RTE_FLOW_ACTION_TYPE_SET_IPV4_DSCP);
if (mask && !is_all_zeros(mask, sizeof *mask)) {
VLOG_DBG_RL(&rl, "Unsupported IPv4 set action");
@@ -1924,6 +1947,8 @@ parse_set_actions(struct flow_actions *actions,
add_set_flow_action(ipv6_src, RTE_FLOW_ACTION_TYPE_SET_IPV6_SRC);
add_set_flow_action(ipv6_dst, RTE_FLOW_ACTION_TYPE_SET_IPV6_DST);
add_set_flow_action(ipv6_hlimit, RTE_FLOW_ACTION_TYPE_SET_TTL);
+add_set_flow_action(ipv6_tclass,
+RTE_FLOW_ACTION_TYPE_SET_IPV6_DSCP);
if (mask && !is_all_zeros(mask, sizeof *mask)) {
VLOG_DBG_RL(&rl, "Unsupported IPv6 set action");
--
2.19.0.rc0.windows.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn 8/8] northd: Add ACL Sampling.
On Mon, Jul 08, 2024 at 01:24:14PM GMT, Dumitru Ceara wrote:
> From: Adrian Moreno
>
> Introduce a new table called Sample where per-flow IPFIX configuration
> can be specified.
> Also, reference rows from such table from the ACL table to enable the
> configuration of ACL sampling. If enabled, northd will add a sample
> action to each ACL related logical flow.
>
> Packets that hit stateful ACLs are sampled in different ways depending
> whether they are initiating a new session or are just forwarded on an
> existing (already allowed) session. Two new columns ("sample_new" and
> "sample_est") are added to the ACL table to allow for potentially
> different sampling rates for the two cases.
>
> Note: If an ACL has both sampling enabled and a label associated to it
> then the label value overrides the observation point ID defined in the
> sample configuration. This is a side effect of the implementation
> (observation point IDs are stored in conntrack in the same part of the
> ct_label where ACL labels are also stored). The two features
> (sampling and ACL labels) serve however similar purposes so it's not
> expected that they're both enabled together.
>
> When sampling is enabled on an ACL additional logical flows are created
> for that ACL (one for stateless ACLs and 3 for stateful ACLs) in the ACL
> action stage of the logical pipeline. These additional flows match on a
> combination of conntrack state values and observation point id values
> (either against a logical register or against the stored ct_label state)
> in order to determine whether the packets hitting the ACLs must be
> sampled or not. This comes with a slight increase in the number of
> logical flows and in the number of OpenFlow rules. The number of
> additional flows _does not_ depend on the original ACL match or action.
>
> New --sample-new and --sample-est optional arguments are added to the
> 'ovn-nbctl acl-add' command to allow configuring these new types of
> sampling for ACLs.
>
> An example workflow of configuring ACL samples is:
> # Create Sampling_App mappings for ACL traffic types:
> ovn-nbctl create Sampling_App name="acl-new-traffic-sampling" \
> id="42"
> ovn-nbctl create sampling_app name="acl-est-traffic-sampling" \
> id="43"
> # Create two sample collectors, one that samples all packets (c1)
> # and another one that samples with a probability of 10% (c2):
> c1=$(ovn-nbctl create Sample_Collector name=c1 \
>probability=65535 set_id=1)
> c2=$(ovn-nbctl create Sample_Collector name=c2 \
>probability=6553 set_id=2)
> # Create two sample configurations (for new and for established
> # traffic):
> s1=$(ovn-nbctl create sample collector="$c1 $c2" metadata=4301)
> s2=$(ovn-nbctl create sample collector="$c1 $c2" metadata=4302)
> # Create an ingress ACL to allow IP traffic:
> ovn-nbctl --sample-new=$s1 --sample-est=$s2 acl-add ls \
> from-lport 1 "ip" allow-related
>
> The config above will generate IPFIX samples with:
> - observation domain id set to 42 (Sampling_App
> "acl-new-traffic-sampling" config) and observation point id
> set to 4301 (Sample s1) for packets that create a new
> connection
> - observation domain id set to 43 (Sampling_app
> "acl-est-traffic-sampling" config) and observation point id
> set to 4302 (Sample s2) for packets that are part of an already
> existing connection
>
We assume the dp_key is 0 in this case, right? Maybe worth mentioning
it.
> Reported-at: https://issues.redhat.com/browse/FDP-305
> Signed-off-by: Adrian Moreno
> Co-authored-by: Dumitru Ceara
> Signed-off-by: Dumitru Ceara
> ---
> NEWS | 3 +
> include/ovn/logical-fields.h | 2 +
> lib/logical-fields.c | 6 +
> northd/northd.c| 519 +++--
> northd/ovn-northd.8.xml| 26 ++
> ovn-nb.ovsschema | 44 ++-
> ovn-nb.xml | 56 +++
> tests/atlocal.in | 6 +
> tests/ovn-macros.at| 4 +
> tests/ovn-nbctl.at | 20 +
> tests/ovn-northd.at| 240 ++--
> tests/ovn.at | 3 +
> tests/system-common-macros.at | 11 +
> tests/system-ovn.at| 149 +++
> utilities/containers/fedora/Dockerfile | 1 +
> utilities/containers/ubuntu/Dockerfile | 1 +
> utilities/ovn-nbctl.8.xml | 8 +-
> utilities/ovn-nbctl.c | 43 +-
> 18 files changed, 1079 insertions(+), 63 deletions(-)
I'm not done reviewing. I need to spend some time understanding how
locial flows are built. Not very familiar with this. Plus I want to run
some tests. However, here are some initial thoughts.
>
> diff --git a/NEWS b/NEWS
> index fcf182bc02..7899c623f2 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -41,
Re: [ovs-dev] [PATCH v1 11/13] ofproto: xlate: Make flow-sampled drops explicit.
On 7 Jul 2024, at 22:09, Adrian Moreno wrote:
> When an action set ends in a an OFP_SAMPLE action, it means the packet
> will be dropped and sampled. Make the drop explicit in this case so that
> drop statistics remain accurate.
>
> This could be done outside of the sample action, i.e: "sample(...),drop"
> but datapaths optimize sample actions that are found in the last
> position. So, taking into account that datapaths already report when the
> last sample probability fails, it is safe to put the drop inside the
> sample, i.e: "sample(...,drop)".
Some style comments below, as this was discussed with Ilya, I let him comment
on the feature side.
> Signed-off-by: Adrian Moreno
> ---
> ofproto/ofproto-dpif-xlate.c | 16 +++--
> tests/drop-stats.at | 65
> tests/ofproto-dpif.at| 44
> 3 files changed, 123 insertions(+), 2 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index b9546dc5b..094fe5d72 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -715,6 +715,7 @@ static void xlate_xbundle_copy(struct xbridge *, struct
> xbundle *);
> static void xlate_xport_copy(struct xbridge *, struct xbundle *,
> struct xport *);
> static void xlate_xcfg_free(struct xlate_cfg *);
> +static void put_drop_action(struct ofpbuf *, enum xlate_error);
>
> /* Tracing helpers. */
>
> @@ -3392,6 +3393,8 @@ struct sample_userspace_args {
> struct compose_sample_args {
> uint32_t probability; /* Number of packets out of
> * UINT32_MAX to sample. */
> +bool last;/* If it's the last action and
> a
> + * drop action must be added.
> */
Should describing this means this is the last action not be enough?
Maybe also rename it to be last_action, so it's more clear what this means.
> struct sample_userspace_args *userspace; /* Optional,
> * arguments for userspace. */
> struct sample_psample_args *psample; /* Optional,
> @@ -3456,6 +3459,11 @@ compose_sample_action(struct xlate_ctx *ctx,
> ovs_assert(res == 0);
> }
>
> +if (args->last &&
> +ovs_explicit_drop_action_supported(ctx->xbridge->ofproto)) {
> +put_drop_action(ctx->odp_actions, ctx->error);
Any reason why we use ctx->error here? Should we just not use XLATE_OK?
> +}
> +
> if (is_sample) {
> nl_msg_end_nested(ctx->odp_actions, actions_offset);
> nl_msg_end_nested(ctx->odp_actions, sample_offset);
> @@ -3490,6 +3498,7 @@ compose_sflow_action(struct xlate_ctx *ctx)
>
> args.probability = dpif_sflow_get_probability(sflow);
> args.userspace = &userspace;
> +args.last = false;
>
> return compose_sample_action(ctx, &args);
> }
> @@ -3542,6 +3551,7 @@ compose_ipfix_action(struct xlate_ctx *ctx, odp_port_t
> output_odp_port)
>
> args.probability = dpif_ipfix_get_bridge_exporter_probability(ipfix);
> args.userspace = &userspace;
> +args.last = false;
>
> compose_sample_action(ctx, &args);
> }
> @@ -5974,7 +5984,8 @@ xlate_fill_ipfix_sample(struct xlate_ctx *ctx,
>
> static void
> xlate_sample_action(struct xlate_ctx *ctx,
> -const struct ofpact_sample *os)
> +const struct ofpact_sample *os,
> +bool last)
> {
> uint8_t cookie_buf[sizeof(os->obs_domain_id) + sizeof(os->obs_point_id)];
> struct dpif_lsample *lsample = ctx->xbridge->lsample;
> @@ -5991,6 +6002,7 @@ xlate_sample_action(struct xlate_ctx *ctx,
> * the same percentage. */
> compose_args.probability =
> ((uint32_t) os->probability << 16) | os->probability;
> +compose_args.last = last;
>
> if (ipfix) {
> xlate_fill_ipfix_sample(ctx, os, ipfix, &userspace);
> @@ -7726,7 +7738,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t
> ofpacts_len,
> break;
>
> case OFPACT_SAMPLE:
> -xlate_sample_action(ctx, ofpact_get_SAMPLE(a));
> +xlate_sample_action(ctx, ofpact_get_SAMPLE(a), last);
> break;
>
> case OFPACT_CLONE:
> diff --git a/tests/drop-stats.at b/tests/drop-stats.at
> index 1d3af98da..44c5cc35b 100644
> --- a/tests/drop-stats.at
> +++ b/tests/drop-stats.at
> @@ -191,3 +191,68 @@ ovs-appctl coverage/read-counter
> drop_action_too_many_mpls_labels
>
> OVS_VSWITCHD_STOP(["/|WARN|/d"])
> AT_CLEANUP
> +
> +AT_SETUP([drop-stats - sampling action])
> +
> +OVS_VSWITCHD_START([dnl
> +set bridge br0 datapath_type=dummy \
> +protocols=OpenFlow10,OpenFlow13,OpenFlow14,OpenFlow15 -- \
> +add-port br0 p1 -- set Interface p1 type=dummy ofport_request=1 -- \
> +add-port br0 p2 -- set Interface p2 type=dummy ofport_request=2])
Re: [ovs-dev] [PATCH v1 11/13] ofproto: xlate: Make flow-sampled drops explicit.
On 7 Jul 2024, at 22:09, Adrian Moreno wrote:
> When an action set ends in a an OFP_SAMPLE action, it means the packet
> will be dropped and sampled. Make the drop explicit in this case so that
> drop statistics remain accurate.
>
> This could be done outside of the sample action, i.e: "sample(...),drop"
> but datapaths optimize sample actions that are found in the last
> position. So, taking into account that datapaths already report when the
> last sample probability fails, it is safe to put the drop inside the
> sample, i.e: "sample(...,drop)".
>
> Signed-off-by: Adrian Moreno
> ---
> ofproto/ofproto-dpif-xlate.c | 16 +++--
> tests/drop-stats.at | 65
> tests/ofproto-dpif.at| 44
> 3 files changed, 123 insertions(+), 2 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index b9546dc5b..094fe5d72 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -715,6 +715,7 @@ static void xlate_xbundle_copy(struct xbridge *, struct
> xbundle *);
> static void xlate_xport_copy(struct xbridge *, struct xbundle *,
> struct xport *);
> static void xlate_xcfg_free(struct xlate_cfg *);
> +static void put_drop_action(struct ofpbuf *, enum xlate_error);
>
> /* Tracing helpers. */
>
> @@ -3392,6 +3393,8 @@ struct sample_userspace_args {
> struct compose_sample_args {
> uint32_t probability; /* Number of packets out of
> * UINT32_MAX to sample. */
> +bool last;/* If it's the last action and
> a
> + * drop action must be added.
> */
> struct sample_userspace_args *userspace; /* Optional,
> * arguments for userspace. */
> struct sample_psample_args *psample; /* Optional,
> @@ -3456,6 +3459,11 @@ compose_sample_action(struct xlate_ctx *ctx,
> ovs_assert(res == 0);
> }
>
> +if (args->last &&
> +ovs_explicit_drop_action_supported(ctx->xbridge->ofproto)) {
> +put_drop_action(ctx->odp_actions, ctx->error);
> +}
> +
> if (is_sample) {
> nl_msg_end_nested(ctx->odp_actions, actions_offset);
> nl_msg_end_nested(ctx->odp_actions, sample_offset);
> @@ -3490,6 +3498,7 @@ compose_sflow_action(struct xlate_ctx *ctx)
>
> args.probability = dpif_sflow_get_probability(sflow);
> args.userspace = &userspace;
> +args.last = false;
>
> return compose_sample_action(ctx, &args);
> }
> @@ -3542,6 +3551,7 @@ compose_ipfix_action(struct xlate_ctx *ctx, odp_port_t
> output_odp_port)
>
> args.probability = dpif_ipfix_get_bridge_exporter_probability(ipfix);
> args.userspace = &userspace;
> +args.last = false;
>
> compose_sample_action(ctx, &args);
> }
> @@ -5974,7 +5984,8 @@ xlate_fill_ipfix_sample(struct xlate_ctx *ctx,
>
> static void
> xlate_sample_action(struct xlate_ctx *ctx,
> -const struct ofpact_sample *os)
> +const struct ofpact_sample *os,
> +bool last)
> {
> uint8_t cookie_buf[sizeof(os->obs_domain_id) + sizeof(os->obs_point_id)];
> struct dpif_lsample *lsample = ctx->xbridge->lsample;
> @@ -5991,6 +6002,7 @@ xlate_sample_action(struct xlate_ctx *ctx,
> * the same percentage. */
> compose_args.probability =
> ((uint32_t) os->probability << 16) | os->probability;
> +compose_args.last = last;
>
> if (ipfix) {
> xlate_fill_ipfix_sample(ctx, os, ipfix, &userspace);
> @@ -7726,7 +7738,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t
> ofpacts_len,
> break;
>
> case OFPACT_SAMPLE:
> -xlate_sample_action(ctx, ofpact_get_SAMPLE(a));
> +xlate_sample_action(ctx, ofpact_get_SAMPLE(a), last);
> break;
>
> case OFPACT_CLONE:
> diff --git a/tests/drop-stats.at b/tests/drop-stats.at
> index 1d3af98da..44c5cc35b 100644
> --- a/tests/drop-stats.at
> +++ b/tests/drop-stats.at
> @@ -191,3 +191,68 @@ ovs-appctl coverage/read-counter
> drop_action_too_many_mpls_labels
>
> OVS_VSWITCHD_STOP(["/|WARN|/d"])
> AT_CLEANUP
> +
> +AT_SETUP([drop-stats - sampling action])
> +
> +OVS_VSWITCHD_START([dnl
> +set bridge br0 datapath_type=dummy \
> +protocols=OpenFlow10,OpenFlow13,OpenFlow14,OpenFlow15 -- \
> +add-port br0 p1 -- set Interface p1 type=dummy ofport_request=1 -- \
> +add-port br0 p2 -- set Interface p2 type=dummy ofport_request=2])
> +
> +AT_DATA([flows.txt], [dnl
> +table=0,in_port=1,actions=sample(probability=65535,collector_set_id=1)
> +table=0,in_port=2,actions=sample(probability=32767,collector_set_id=1)
> +])
> +
> +AT_CHECK([
> +ovs-ofctl del-flows br0
> +ovs-ofctl -Oopenflow13 add-flows br0 flows.txt
> +])
> +
> +AT_CHECK([ov
Re: [ovs-dev] [PATCH v1 13/13] ofp-actions: Load data from fields in sample action.
On 7 Jul 2024, at 22:09, Adrian Moreno wrote:
> When sample action gets used as a way of sampling traffic with
> controller-generated metadata (i.e: obs_domain_id and obs_point_id),
> the controller will have to increase the number of flows to ensure each
> part of the pipeline contains the right metadata.
>
> As an example, if the controller decides to sample stateful traffic, it
> could store the computed metadata for each connection in the conntrack
> label. However, for established connections, a flow must be created for
> each different ct_label value with a sample action that contains a
> different hardcoded obs_domain and obs_point id.
>
> This patch adds a new version of the NXAST_RAW_SAMPLE* action (number 4)
> that supports specifying the observation point and domain using an
> OpenFlow field reference, so now the controller can express:
>
> sample(...
> obs_domain_id=NXM_NX_CT_LABEL[0..31],
> obs_point_id=NXM_NX_CT_LABEL[32..63]
> ...
>)
>
> Signed-off-by: Adrian Moreno
See some comments inline. I’m missing the documentation update.
Cheers,
Eelco
> ---
> include/openvswitch/ofp-actions.h | 8 +-
> lib/ofp-actions.c | 249 +++---
> ofproto/ofproto-dpif-xlate.c | 55 ++-
> python/ovs/flow/ofp.py| 8 +-
> python/ovs/flow/ofp_act.py| 4 +-
> tests/ofp-actions.at | 5 +
> tests/ofproto-dpif.at | 41 +
> tests/system-traffic.at | 74 +
> 8 files changed, 405 insertions(+), 39 deletions(-)
>
> diff --git a/include/openvswitch/ofp-actions.h
> b/include/openvswitch/ofp-actions.h
> index 7b57e49ad..56dc2c147 100644
> --- a/include/openvswitch/ofp-actions.h
> +++ b/include/openvswitch/ofp-actions.h
> @@ -1015,14 +1015,16 @@ enum nx_action_sample_direction {
>
> /* OFPACT_SAMPLE.
> *
> - * Used for NXAST_SAMPLE, NXAST_SAMPLE2, and NXAST_SAMPLE3. */
> + * Used for NXAST_SAMPLE, NXAST_SAMPLE2, NXAST_SAMPLE3 and NXAST_SAMPLE4. */
> struct ofpact_sample {
> OFPACT_PADDED_MEMBERS(
> struct ofpact ofpact;
> uint16_t probability; /* Always positive. */
> uint32_t collector_set_id;
> -uint32_t obs_domain_id;
> -uint32_t obs_point_id;
> +uint32_t obs_domain_imm;
> +struct mf_subfield obs_domain_src;
> +uint32_t obs_point_imm;
> +struct mf_subfield obs_point_src;
> ofp_port_t sampling_port;
> enum nx_action_sample_direction direction;
> );
> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> index da7b1dd31..e329a7e3f 100644
> --- a/lib/ofp-actions.c
> +++ b/lib/ofp-actions.c
> @@ -330,6 +330,8 @@ enum ofp_raw_action_type {
> NXAST_RAW_SAMPLE2,
> /* NX1.0+(41): struct nx_action_sample2. */
> NXAST_RAW_SAMPLE3,
> +/* NX1.0+(51): struct nx_action_sample4, ... VLMFF */
> +NXAST_RAW_SAMPLE4,
>
> /* NX1.0+(34): struct nx_action_conjunction. */
> NXAST_RAW_CONJUNCTION,
> @@ -6188,6 +6190,34 @@ struct nx_action_sample2 {
> };
> OFP_ASSERT(sizeof(struct nx_action_sample2) == 32);
>
> +/* Action structure for NXAST_SAMPLE4
> + *
> + * NXAST_SAMPLE4 was added in Open vSwitch 3.4.0. Compared to NXAST_SAMPLE3,
> + * it adds support for using field specifiers for observation_domain_id and
> + * observation_point_id. */
> +struct nx_action_sample4 {
> +ovs_be16 type; /* OFPAT_VENDOR. */
> +ovs_be16 len; /* Length is 32. */
> +ovs_be32 vendor;/* NX_VENDOR_ID. */
> +ovs_be16 subtype; /* NXAST_SAMPLE. */
> +ovs_be16 probability; /* Fraction of packets to sample. */
> +ovs_be32 collector_set_id; /* ID of collector set in OVSDB. */
> +ovs_be32 obs_domain_src;/* Source of the observation_domain_id.
> */
> +union {
> +ovs_be16 obs_domain_ofs_nbits; /* Range to use from source field. */
> +ovs_be32 obs_domain_imm;/* Immediate value for domain id. */
> +};
> +ovs_be32 obs_point_src; /* Source of the observation_point_id */
> +union {
> +ovs_be16 obs_point_ofs_nbits; /* Range to use from source field. */
> +ovs_be32 obs_point_imm;/* Immediate value for point id. */
> +};
> +ovs_be16 sampling_port; /* Sampling port. */
> +uint8_t direction; /* Sampling direction. */
> +uint8_t zeros[5]; /* Pad to a multiple of 8 bytes */
> + };
Maybe align all comments, and make sure all end with a dot.
struct nx_action_sample4 {
ovs_be16 type; /* OFPAT_VENDOR. */
ovs_be16 len; /* Length is 32. */
ovs_be32 vendor; /* NX_VENDOR_ID. */
ovs_be16 subtype; /* NXAST_SAMPLE. */
ovs_be16 probability; /* Fraction of packets to sample. */
ovs_be32 collector_set_id; /* ID of collector set in OVSDB. *
Re: [ovs-dev] [PATCH v1 12/13] ofproto: xlate: Make bridge-sampled drops explicit.
On 7 Jul 2024, at 22:09, Adrian Moreno wrote:
> The decision to add or not the explicit drop action is currently based
> on whether the resulting dp action list is empty or not.
>
> This is OK for most of the cases but if per-flow IPFIX/sFlow sampling
> is enabled on the bridge, it doesn't work as expected.
>
> Fix this by taking into account the size of these sampling actions.
Same comments as on the previous patch.
> Fixes: a13a0209750c ("userspace: Improved packet drop statistics.")
> Cc: anju.tho...@ericsson.com
> Signed-off-by: Adrian Moreno
> ---
> ofproto/ofproto-dpif-xlate.c | 5 ++--
> tests/drop-stats.at | 44
> tests/ofproto-dpif.at| 4 ++--
> 3 files changed, 49 insertions(+), 4 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 094fe5d72..323a58cbf 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -8153,6 +8153,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out
> *xout)
> uint64_t action_set_stub[1024 / 8];
> uint64_t frozen_actions_stub[1024 / 8];
> uint64_t actions_stub[256 / 8];
> +size_t sample_actions_len = 0;
> struct ofpbuf scratch_actions = OFPBUF_STUB_INITIALIZER(actions_stub);
> struct xlate_ctx ctx = {
> .xin = xin,
> @@ -8412,7 +8413,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out
> *xout)
> user_cookie_offset = compose_sflow_action(&ctx);
> compose_ipfix_action(&ctx, ODPP_NONE);
> }
> -size_t sample_actions_len = ctx.odp_actions->size;
> +sample_actions_len = ctx.odp_actions->size;
> bool ecn_drop = !tnl_process_ecn(flow);
>
> if (!ecn_drop
> @@ -8575,7 +8576,7 @@ exit:
> }
>
> /* Install drop action if datapath supports explicit drop action. */
> -if (xin->odp_actions && !xin->odp_actions->size &&
> +if (xin->odp_actions && xin->odp_actions->size == sample_actions_len &&
> ovs_explicit_drop_action_supported(ctx.xbridge->ofproto)) {
> put_drop_action(xin->odp_actions, ctx.error);
See other patch, do we need ctx.error here?
> }
> diff --git a/tests/drop-stats.at b/tests/drop-stats.at
> index 44c5cc35b..31782ac52 100644
> --- a/tests/drop-stats.at
> +++ b/tests/drop-stats.at
> @@ -192,6 +192,50 @@ ovs-appctl coverage/read-counter
> drop_action_too_many_mpls_labels
> OVS_VSWITCHD_STOP(["/|WARN|/d"])
> AT_CLEANUP
>
> +AT_SETUP([drop-stats - sampling])
Should this maybe be 'bridge sampling'?
> +
> +OVS_VSWITCHD_START([dnl
> +set bridge br0 datapath_type=dummy \
> +protocols=OpenFlow10,OpenFlow13,OpenFlow14,OpenFlow15 -- \
> +add-port br0 p1 -- set Interface p1 type=dummy ofport_request=1])
> +
> +AT_DATA([flows.txt], [dnl
> +table=0,in_port=1,actions=drop
> +])
> +
> +AT_CHECK([
> +ovs-ofctl del-flows br0
> +ovs-ofctl -Oopenflow13 add-flows br0 flows.txt
> +ovs-ofctl -Oopenflow13 dump-flows br0 | ofctl_strip | sort | grep
> actions ], [0], [dnl
> + in_port=1 actions=drop
> +])
> +
> +AT_CHECK([ovs-vsctl -- set bridge br0 ipfix=@fix -- \
> +--id=@fix create ipfix targets=\"127.0.0.1:4739\" \
> + sampling=1],
> + [0], [ignore])
> +
> +AT_CHECK([
> +ovs-appctl netdev-dummy/receive p1
> 'in_port(1),packet_type(ns=0,id=0),eth(src=3a:6d:d2:09:9c:ab,dst=1e:2c:e9:2a:66:9e),ipv4(src=192.168.10.10,dst=192.168.10.30,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'
> +ovs-appctl netdev-dummy/receive p1
> 'in_port(1),packet_type(ns=0,id=0),eth(src=3a:6d:d2:09:9c:ab,dst=1e:2c:e9:2a:66:9e),ipv4(src=192.168.10.10,dst=192.168.10.30,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'
> +ovs-appctl netdev-dummy/receive p1
> 'in_port(1),packet_type(ns=0,id=0),eth(src=3a:6d:d2:09:9c:ab,dst=1e:2c:e9:2a:66:9e),ipv4(src=192.168.10.10,dst=192.168.10.30,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'
> +], [0], [ignore])
>
For loop?
> +AT_CHECK([ovs-appctl dpctl/dump-flows | sed
> 's/used:[[0-9]].[[0-9]]*s/used:0.0/' | sort], [0], [flow-dump from the main
> thread:
> +recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no),
> packets:2, bytes:212, used:0.0,
> actions:userspace(pid=0,ipfix(output_port=4294967295)),drop
> +])
> +
> +ovs-appctl time/warp 5000
> +
> +AT_CHECK([
> +ovs-appctl coverage/read-counter drop_action_of_pipeline
> +], [0], [dnl
> +3
> +])
> +
> +OVS_VSWITCHD_STOP(["/sending to collector failed/d"])
> +
> +AT_CLEANUP
> AT_SETUP([drop-stats - sampling action])
>
> OVS_VSWITCHD_START([dnl
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 701b8a851..ba8f3b69c 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -8082,7 +8082,7 @@ for i in `seq 1 3`; do
> done
> AT_CHECK([ovs-appctl dpctl/dump-flows | sed 's/.*\(packets:\)/\1/' | sed
> 's/used:[[0-9]].[[0-9]]*s/used:0.001s/'], [0], [dnl
> flow-dump from the ma
Re: [ovs-dev] [PATCH v1 06/13] ofproto-dpif-xlate: Use psample for local sample.
On 7 Jul 2024, at 22:08, Adrian Moreno wrote:
> Use the newly added psample action to implement OpenFlow sample() actions
> with local sampling configuration if possible.
>
> A bit of refactoring in compose_sample_actions arguments helps make it a
> bit more readable.
Some comments below.
Cheers,
Eelco
> Signed-off-by: Adrian Moreno
> ---
> ofproto/ofproto-dpif-lsample.c | 16 +++
> ofproto/ofproto-dpif-lsample.h | 5 +
> ofproto/ofproto-dpif-xlate.c | 251 +++--
> ofproto/ofproto-dpif-xlate.h | 5 +-
> ofproto/ofproto-dpif.c | 2 +-
> tests/ofproto-dpif.at | 146 +++
> 6 files changed, 345 insertions(+), 80 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-lsample.c b/ofproto/ofproto-dpif-lsample.c
> index d675a116f..534ad96f0 100644
> --- a/ofproto/ofproto-dpif-lsample.c
> +++ b/ofproto/ofproto-dpif-lsample.c
> @@ -140,6 +140,22 @@ dpif_lsample_set_options(struct dpif_lsample *lsample,
> return changed;
> }
>
> +/* Returns the group_id for a given collector_set_id, if it exists. */
> +bool
> +dpif_lsample_get_group_id(struct dpif_lsample *ps, uint32_t collector_set_id,
> + uint32_t *group_id)
> +{
> +struct lsample_exporter_node *node;
> +bool found = false;
> +
> +node = dpif_lsample_find_exporter_node(ps, collector_set_id);
> +if (node) {
> +found = true;
> +*group_id = node->exporter.options.group_id;
> +}
> +return found;
> +}
> +
> struct dpif_lsample *
> dpif_lsample_create(void)
> {
> diff --git a/ofproto/ofproto-dpif-lsample.h b/ofproto/ofproto-dpif-lsample.h
> index bee36c9c5..9c1026551 100644
> --- a/ofproto/ofproto-dpif-lsample.h
> +++ b/ofproto/ofproto-dpif-lsample.h
> @@ -18,6 +18,7 @@
> #define OFPROTO_DPIF_LSAMPLE_H 1
>
> #include
> +#include
> #include
>
> struct dpif_lsample;
> @@ -33,4 +34,8 @@ bool dpif_lsample_set_options(struct dpif_lsample *,
>const struct ofproto_lsample_options *,
>size_t n_opts);
>
> +bool dpif_lsample_get_group_id(struct dpif_lsample *,
> + uint32_t collector_set_id,
> + uint32_t *group_id);
> +
> #endif /* OFPROTO_DPIF_LSAMPLE_H */
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 7c4950895..5e8113d5e 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -47,6 +47,7 @@
> #include "ofproto/ofproto-dpif-ipfix.h"
> #include "ofproto/ofproto-dpif-mirror.h"
> #include "ofproto/ofproto-dpif-monitor.h"
> +#include "ofproto/ofproto-dpif-lsample.h"
Add in alphabetical order?
> #include "ofproto/ofproto-dpif-sflow.h"
> #include "ofproto/ofproto-dpif-trace.h"
> #include "ofproto/ofproto-dpif-xlate-cache.h"
> @@ -117,6 +118,7 @@ struct xbridge {
> struct dpif_sflow *sflow; /* SFlow handle, or null. */
> struct dpif_ipfix *ipfix; /* Ipfix handle, or null. */
> struct netflow *netflow; /* Netflow handle, or null. */
> +struct dpif_lsample *lsample; /* Local sample handle, or null. */
I would move above netflow, as you also do the actual init/unref before netflow.
> struct stp *stp; /* STP or null if disabled. */
> struct rstp *rstp;/* RSTP or null if disabled. */
>
> @@ -686,6 +688,7 @@ static void xlate_xbridge_set(struct xbridge *, struct
> dpif *,
>const struct mbridge *,
>const struct dpif_sflow *,
>const struct dpif_ipfix *,
> + const struct dpif_lsample *,
>const struct netflow *,
>bool forward_bpdu, bool has_in_band,
>const struct dpif_backer_support *,
> @@ -1069,6 +1072,7 @@ xlate_xbridge_set(struct xbridge *xbridge,
>const struct mbridge *mbridge,
>const struct dpif_sflow *sflow,
>const struct dpif_ipfix *ipfix,
> + const struct dpif_lsample *lsample,
>const struct netflow *netflow,
>bool forward_bpdu, bool has_in_band,
>const struct dpif_backer_support *support,
> @@ -1099,6 +1103,11 @@ xlate_xbridge_set(struct xbridge *xbridge,
> xbridge->ipfix = dpif_ipfix_ref(ipfix);
> }
>
> +if (xbridge->lsample != lsample) {
> +dpif_lsample_unref(xbridge->lsample);
> +xbridge->lsample = dpif_lsample_ref(lsample);
> +}
> +
> if (xbridge->stp != stp) {
> stp_unref(xbridge->stp);
> xbridge->stp = stp_ref(stp);
> @@ -1213,9 +1222,10 @@ xlate_xbridge_copy(struct xbridge *xbridge)
> xlate_xbridge_set(new_xbridge,
>xbridge->dpif, xbridge->ml, xbridge->stp,
>xbridge->rstp, xbridge->ms, xbridge->mbrid
Re: [ovs-dev] [PATCH v1 09/13] tests: Add test-psample testing utility.
On 7 Jul 2024, at 22:09, Adrian Moreno wrote: > This simple program reads from psample and prints the packets to stdout. > > Signed-off-by: Adrian Moreno This patch looks good to me. Acked-by: Eelco Chaudron //Eelco ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1 10/13] tests: Test local sampling.
On 7 Jul 2024, at 22:09, Adrian Moreno wrote: > Test simultaneous IPFIX and local sampling including slow-path. See comments below. Cheers, Eelco > Signed-off-by: Adrian Moreno > --- > tests/system-common-macros.at | 4 + > tests/system-traffic.at | 285 ++ > 2 files changed, 289 insertions(+) > > diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at > index 2a68cd664..860d6a8c9 100644 > --- a/tests/system-common-macros.at > +++ b/tests/system-common-macros.at > @@ -378,3 +378,7 @@ m4_define([OVS_CHECK_GITHUB_ACTION], > # OVS_CHECK_DROP_ACTION() > m4_define([OVS_CHECK_DROP_ACTION], > [AT_SKIP_IF([! grep -q "Datapath supports explicit drop action" > ovs-vswitchd.log])]) > + > +# OVS_CHECK_PSAMPLE() > +m4_define([OVS_CHECK_PSAMPLE], > +[AT_SKIP_IF([! grep -q "Datapath supports psample" ovs-vswitchd.log])]) > diff --git a/tests/system-traffic.at b/tests/system-traffic.at > index 3f1a15445..ddab4ece3 100644 > --- a/tests/system-traffic.at > +++ b/tests/system-traffic.at > @@ -9103,3 +9103,288 @@ OVS_WAIT_UNTIL([ovs-pcap p2.pcap | grep -q > "m4_join([], [^], > > OVS_TRAFFIC_VSWITCHD_STOP > AT_CLEANUP > + > +AT_BANNER([local-sampling]) > + > +m4_define([SAMPLE_ACTION], > + > [sample(probability=65535,collector_set_id=$1,obs_domain_id=$2,obs_point_id=$3)]dnl > +) > + > +AT_SETUP([psample - sanity check]) Maybe call it '- ipv4' as it's equal to ipv6, or rename the IPv6 one to 'sanity check IPv6' > +OVS_TRAFFIC_VSWITCHD_START() > +OVS_CHECK_PSAMPLE() > + > +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") > + > +AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \ > +-- create Flow_Sample_Collector_Set id=1 bridge=@br0 > local_group_id=10 \ > +-- create Flow_Sample_Collector_Set id=2 bridge=@br0 > local_group_id=12], > + [0], [ignore]) Maybe wrap to stay <80 chars? AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \ -- create Flow_Sample_Collector_Set id=1 bridge=@br0 \ local_group_id=10 \ -- create Flow_Sample_Collector_Set id=2 bridge=@br0 \ local_group_id=12], [0], [ignore]) > + > +AT_DATA([flows.txt], [dnl > +arp actions=NORMAL > +in_port=ovs-p0,ip actions=SAMPLE_ACTION(1, 2853183536, 2856341600),ovs-p1 > +in_port=ovs-p1,ip actions=SAMPLE_ACTION(2, 3138396208, 3141554272),ovs-p0 > +]) > + > +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) > + > +OVS_DAEMONIZE([ovstest test-psample > psample.out], [psample.pid]) > +OVS_WAIT_UNTIL([grep -q "Listening for psample events" psample.out]) > + > +NS_CHECK_EXEC([at_ns0], [ping -q -c 1 10.1.1.2 | FORMAT_PING], [0], [dnl > +1 packets transmitted, 1 received, 0% packet loss, time 0ms > +]) > + > +AT_CHECK([ovs-appctl dpctl/dump-flows -m --names], [0], [stdout]) > +AT_CHECK([grep -q > 'actions:psample(group=10,cookie=0xaa102030aa405060),ovs-p1' stdout]) > +AT_CHECK([grep -q > 'actions:psample(group=12,cookie=0xbb102030bb405060),ovs-p0' stdout]) > + > +m4_define([SAMPLE1], [m4_join([ ], > +[group_id=0xa], > +[obs_domain=0xaa102030,obs_point=0xaa405060], > +[.*icmp.*nw_src=10.1.1.1,nw_dst=10.1.1.2])]) > + > +m4_define([SAMPLE2], [m4_join([ ], > +[group_id=0xc], > +[obs_domain=0xbb102030,obs_point=0xbb405060], > +[.*icmp.*nw_src=10.1.1.2,nw_dst=10.1.1.1])]) > + > +OVS_WAIT_UNTIL([grep -qE 'SAMPLE1' psample.out]) > +OVS_WAIT_UNTIL([grep -qE 'SAMPLE2' psample.out]) > + > +AT_CHECK([ovs-appctl lsample/show br0], [0], [dnl > +Local sample statistics for bridge "br0": > +Collector Set ID: 1: > + Group ID : 10 > + Total packets: 1 > + Total bytes : 98 > + > +Collector Set ID: 2: > + Group ID : 12 > + Total packets: 1 > + Total bytes : 98 > +]) > + We miss OVS_TRAFFIC_VSWITCHD_STOP() here. > +AT_CLEANUP > + > +AT_SETUP([psample - ipv6]) See comment above, if you decide to keep it ipv6, please change it to IPv6. > +OVS_TRAFFIC_VSWITCHD_START() > +OVS_CHECK_PSAMPLE() > + > +ADD_NAMESPACES(at_ns0, at_ns1) > + > +ADD_VETH(p0, at_ns0, br0, "fc00::1/96") > +ADD_VETH(p1, at_ns1, br0, "fc00::2/96") > + > +AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \ > +-- create Flow_Sample_Collector_Set id=1 bridge=@br0 > local_group_id=10 \ > +-- create Flow_Sample_Collector_Set id=2 bridge=@br0 > local_group_id=12], > + [0], [ignore]) > + > +AT_DATA([flows.txt], [dnl > +priority=100,in_port=ovs-p0,ip6,icmp6,icmpv6_type=128 > actions=SAMPLE_ACTION(1, 2853183536, 2856341600),ovs-p1 > +priority=100,in_port=ovs-p1,ip6,icmp6,icmpv6_type=129 > actions=SAMPLE_ACTION(2, 3138396208, 3141554272),ovs-p0 > +priority=0 actions=NORMAL > +]) > + > +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) > + > +OVS_DAEMONIZE([ovstest test-psample > psample.out], [psample.pid]) > +OVS_WAIT_UNTIL([grep -q "Listening for psample event
Re: [ovs-dev] [PATCH v1 08/13] ofproto-dpif-lsample: Show stats via unixctl.
On 7 Jul 2024, at 22:09, Adrian Moreno wrote:
> Add a command to dump statistics per exporter.
Small nit below, rest looks good to me. If this is the only change in the
next series you can add my ACK.
//Eelco
> Signed-off-by: Adrian Moreno
> ---
> NEWS | 2 +
> ofproto/ofproto-dpif-lsample.c | 111 +
> ofproto/ofproto-dpif-lsample.h | 1 +
> ofproto/ofproto-dpif.c | 1 +
> 4 files changed, 115 insertions(+)
>
> diff --git a/NEWS b/NEWS
> index 15faf9fc3..1c53badea 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -20,6 +20,8 @@ Post-v3.3.0
> allows samples to be emitted locally (instead of via IPFIX) in a
> datapath-specific manner. The Linux kernel datapath is the first to
> support this feature by using the new datapath "psample" action.
> + A new unixctl command 'lsample/show' shows packet and bytes statistics
> + per local sample exporter.
>
>
> v3.3.0 - 16 Feb 2024
> diff --git a/ofproto/ofproto-dpif-lsample.c b/ofproto/ofproto-dpif-lsample.c
> index 171129d5b..82a87c27d 100644
> --- a/ofproto/ofproto-dpif-lsample.c
> +++ b/ofproto/ofproto-dpif-lsample.c
> @@ -21,7 +21,10 @@
> #include "dpif.h"
> #include "hash.h"
> #include "ofproto.h"
> +#include "ofproto-dpif.h"
> +#include "openvswitch/dynamic-string.h"
> #include "openvswitch/thread.h"
> +#include "unixctl.h"
>
> /* Dpif local sampling.
> *
> @@ -219,3 +222,111 @@ dpif_lsample_unref(struct dpif_lsample *lsample)
> dpif_lsample_destroy(lsample);
> }
> }
> +
> +static int
> +comp_exporter_collector_id(const void *a_, const void *b_)
> +{
> +const struct lsample_exporter_node *a, *b;
> +
> +a = *(struct lsample_exporter_node **) a_;
> +b = *(struct lsample_exporter_node **) b_;
> +
> +if (a->exporter.options.collector_set_id >
> +b->exporter.options.collector_set_id) {
> +return 1;
> +}
> +if (a->exporter.options.collector_set_id <
> +b->exporter.options.collector_set_id) {
> +return -1;
> +}
> +return 0;
> +}
> +
> +static void
> +lsample_exporter_list(struct dpif_lsample *lsample,
> + struct lsample_exporter_node ***list,
> + size_t *num_exporters)
> +{
> +struct lsample_exporter_node **exporter_list;
> +struct lsample_exporter_node *node;
> +size_t k = 0, n;
> +
> +n = cmap_count(&lsample->exporters);
> +
> +exporter_list = xcalloc(n, sizeof *exporter_list);
> +
> +CMAP_FOR_EACH (node, node, &lsample->exporters) {
> +if (k >= n) {
> +break;
> +}
> +exporter_list[k++] = node;
> +}
> +
> +qsort(exporter_list, k, sizeof *exporter_list,
> comp_exporter_collector_id);
> +
> +*list = exporter_list;
> +*num_exporters = k;
> +}
> +
> +static void
> +lsample_unixctl_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
> + const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
> +{
> +struct lsample_exporter_node **node_list = NULL;
> +struct ds ds = DS_EMPTY_INITIALIZER;
> +const struct ofproto_dpif *ofproto;
> +size_t i, num;
> +
> +ofproto = ofproto_dpif_lookup_by_name(argv[1]);
> +if (!ofproto) {
> +unixctl_command_reply_error(conn, "no such bridge");
> +return;
> +}
> +
> +if (!ofproto->lsample) {
> +unixctl_command_reply_error(conn,
> +"no local sampling exporters
> configured");
> +return;
> +}
> +
> +ds_put_format(&ds, "Local sample statistics for bridge \"%s\":\n",
> + argv[1]);
> +
> +lsample_exporter_list(ofproto->lsample, &node_list, &num);
> +
> +for (i = 0; i < num; i++) {
> +uint64_t n_bytes;
> +uint64_t n_packets;
> +
> +struct lsample_exporter_node *node = node_list[i];
> +
> +atomic_read_relaxed(&node->exporter.n_packets, &n_packets);
> +atomic_read_relaxed(&node->exporter.n_bytes, &n_bytes);
> +
> +if (i) {
> +ds_put_cstr(&ds, "\n");
> +}
> +
> +ds_put_format(&ds, "Collector Set ID: %"PRIu32":\n",
> +node->exporter.options.collector_set_id);
> +ds_put_format(&ds, " Group ID : %"PRIu32"\n",
> +node->exporter.options.group_id);
> +ds_put_format(&ds, " Total packets: %"PRIu64"\n", n_packets);
> +ds_put_format(&ds, " Total bytes : %"PRIu64"\n", n_bytes);
> +}
> +
> +free(node_list);
> +unixctl_command_reply(conn, ds_cstr(&ds));
> +ds_destroy(&ds);
> +}
> +
> +void dpif_lsample_init(void)
> +{
> +static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
> +
> +if (ovsthread_once_start(&once)) {
> +unixctl_command_register("lsample/show", "bridge", 1, 1,
> + lsample_unixctl_show, NULL);
> +ovsthread_once_done(&once);
> +}
> +}
> diff --git a/ofproto/ofproto-dpif-lsample.h b/of
Re: [ovs-dev] [PATCH v1 07/13] ofproto-dpif-xlate-cache: Add lsample to xcache.
On 7 Jul 2024, at 22:08, Adrian Moreno wrote:
> Add a cache entry type for local sample objects.
> Store both the dpif_lsample reference and the collector_set_id so we can
> quickly find the particular exporter.
>
> Using this mechanism, account for packet and byte statistics.
Small nit below, rest looks good to me. If this is the only change in the
next series you can add my ACK.
//Eelco
> Signed-off-by: Adrian Moreno
> ---
> ofproto/ofproto-dpif-lsample.c | 18 ++
> ofproto/ofproto-dpif-lsample.h | 4
> ofproto/ofproto-dpif-xlate-cache.c | 11 ++-
> ofproto/ofproto-dpif-xlate-cache.h | 6 ++
> ofproto/ofproto-dpif-xlate.c | 14 ++
> ofproto/ofproto-dpif.c | 1 +
> 6 files changed, 53 insertions(+), 1 deletion(-)
>
> diff --git a/ofproto/ofproto-dpif-lsample.c b/ofproto/ofproto-dpif-lsample.c
> index 534ad96f0..171129d5b 100644
> --- a/ofproto/ofproto-dpif-lsample.c
> +++ b/ofproto/ofproto-dpif-lsample.c
> @@ -18,6 +18,7 @@
> #include "ofproto-dpif-lsample.h"
>
> #include "cmap.h"
> +#include "dpif.h"
> #include "hash.h"
> #include "ofproto.h"
> #include "openvswitch/thread.h"
> @@ -44,6 +45,8 @@ struct dpif_lsample {
>
> struct lsample_exporter {
> struct ofproto_lsample_options options;
> +atomic_uint64_t n_packets;
> +atomic_uint64_t n_bytes;
> };
>
> struct lsample_exporter_node {
> @@ -156,6 +159,21 @@ dpif_lsample_get_group_id(struct dpif_lsample *ps,
> uint32_t collector_set_id,
> return found;
> }
>
> +void
> +dpif_lsample_credit_stats(struct dpif_lsample *lsample,
> + uint32_t collector_set_id,
> + const struct dpif_flow_stats *stats)
> +{
> +struct lsample_exporter_node *node;
> +uint64_t orig;
> +
> +node = dpif_lsample_find_exporter_node(lsample, collector_set_id);
> +if (node) {
> +atomic_add_relaxed(&node->exporter.n_packets, stats->n_packets,
> &orig);
> +atomic_add_relaxed(&node->exporter.n_bytes, stats->n_bytes, &orig);
> +}
> +}
> +
> struct dpif_lsample *
> dpif_lsample_create(void)
> {
> diff --git a/ofproto/ofproto-dpif-lsample.h b/ofproto/ofproto-dpif-lsample.h
> index 9c1026551..2666e5478 100644
> --- a/ofproto/ofproto-dpif-lsample.h
> +++ b/ofproto/ofproto-dpif-lsample.h
> @@ -23,6 +23,7 @@
>
> struct dpif_lsample;
> struct ofproto_lsample_options;
> +struct dpif_flow_stats;
>
> struct dpif_lsample *dpif_lsample_create(void);
>
> @@ -38,4 +39,7 @@ bool dpif_lsample_get_group_id(struct dpif_lsample *,
> uint32_t collector_set_id,
> uint32_t *group_id);
>
> +void dpif_lsample_credit_stats(struct dpif_lsample *,
> + uint32_t collector_set_id,
> + const struct dpif_flow_stats *);
> #endif /* OFPROTO_DPIF_LSAMPLE_H */
> diff --git a/ofproto/ofproto-dpif-xlate-cache.c
> b/ofproto/ofproto-dpif-xlate-cache.c
> index 2e1fcb3a6..73d79bfc7 100644
> --- a/ofproto/ofproto-dpif-xlate-cache.c
> +++ b/ofproto/ofproto-dpif-xlate-cache.c
> @@ -35,9 +35,10 @@
> #include "learn.h"
> #include "mac-learning.h"
> #include "netdev-vport.h"
> +#include "ofproto/ofproto-dpif.h"
> +#include "ofproto/ofproto-dpif-lsample.h"
> #include "ofproto/ofproto-dpif-mirror.h"
> #include "ofproto/ofproto-dpif-xlate.h"
> -#include "ofproto/ofproto-dpif.h"
> #include "ofproto/ofproto-provider.h"
> #include "openvswitch/dynamic-string.h"
> #include "openvswitch/vlog.h"
> @@ -162,6 +163,11 @@ xlate_push_stats_entry(struct xc_entry *entry,
> }
>
> break;
> +case XC_LSAMPLE:
> +dpif_lsample_credit_stats(entry->lsample.lsample,
> + entry->lsample.collector_set_id,
> + stats);
> +break;
> default:
> OVS_NOT_REACHED();
> }
> @@ -245,6 +251,9 @@ xlate_cache_clear_entry(struct xc_entry *entry)
> break;
> case XC_TUNNEL_HEADER:
> break;
> +case XC_LSAMPLE:
> +dpif_lsample_unref(entry->lsample.lsample);
> +break;
> default:
> OVS_NOT_REACHED();
> }
> diff --git a/ofproto/ofproto-dpif-xlate-cache.h
> b/ofproto/ofproto-dpif-xlate-cache.h
> index 0fc6d2ea6..df8115419 100644
> --- a/ofproto/ofproto-dpif-xlate-cache.h
> +++ b/ofproto/ofproto-dpif-xlate-cache.h
> @@ -29,6 +29,7 @@
> struct bfd;
> struct bond;
> struct dpif_flow_stats;
> +struct dpif_lsample;
> struct flow;
> struct group_dpif;
> struct mbridge;
> @@ -53,6 +54,7 @@ enum xc_type {
> XC_GROUP,
> XC_TNL_NEIGH,
> XC_TUNNEL_HEADER,
> +XC_LSAMPLE,
> };
>
> /* xlate_cache entries hold enough information to perform the side effects of
> @@ -126,6 +128,10 @@ struct xc_entry {
> } operation;
> uint16_t hdr_size;
> } tunnel_hdr;
> +struct {
> +struct dpif_lsample *lsample;
> +uint
Re: [ovs-dev] [PATCH v1 05/13] vswitchd: Add local sampling to vswitchd schema.
On 7 Jul 2024, at 22:08, Adrian Moreno wrote:
> Add as new column in the Flow_Sample_Collector_Set table named
> "local_group_id" which enables this feature.
Small nit below, if fixed add my ACK to the next revision.
//Eelco
> Signed-off-by: Adrian Moreno
> ---
> NEWS | 4 ++
> vswitchd/bridge.c | 78 +++---
> vswitchd/vswitch.ovsschema | 9 -
> vswitchd/vswitch.xml | 40 +--
> 4 files changed, 120 insertions(+), 11 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index e0359b759..15faf9fc3 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -16,6 +16,10 @@ Post-v3.3.0
> per interface 'options:dpdk-lsc-interrupt' to 'false'.
> - Python:
> * Added custom transaction support to the Idl via add_op().
> + - Local sampling is introduced. It reuses the OpenFlow sample action and
> + allows samples to be emitted locally (instead of via IPFIX) in a
> + datapath-specific manner. The Linux kernel datapath is the first to
> + support this feature by using the new datapath "psample" action.
>
>
> v3.3.0 - 16 Feb 2024
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index 95a65fcdc..b7db681f3 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -288,6 +288,7 @@ static void bridge_configure_mac_table(struct bridge *);
> static void bridge_configure_mcast_snooping(struct bridge *);
> static void bridge_configure_sflow(struct bridge *, int
> *sflow_bridge_number);
> static void bridge_configure_ipfix(struct bridge *);
> +static void bridge_configure_lsample(struct bridge *);
> static void bridge_configure_spanning_tree(struct bridge *);
> static void bridge_configure_tables(struct bridge *);
> static void bridge_configure_dp_desc(struct bridge *);
> @@ -989,6 +990,7 @@ bridge_reconfigure(const struct ovsrec_open_vswitch
> *ovs_cfg)
> bridge_configure_netflow(br);
> bridge_configure_sflow(br, &sflow_bridge_number);
> bridge_configure_ipfix(br);
> +bridge_configure_lsample(br);
> bridge_configure_spanning_tree(br);
> bridge_configure_tables(br);
> bridge_configure_dp_desc(br);
> @@ -1537,10 +1539,11 @@ ovsrec_ipfix_is_valid(const struct ovsrec_ipfix
> *ipfix)
> return ipfix && ipfix->n_targets > 0;
> }
>
> -/* Returns whether a Flow_Sample_Collector_Set row is valid. */
> +/* Returns whether a Flow_Sample_Collector_Set row contains a valid IPFIX
> + * configuration. */
> static bool
> -ovsrec_fscs_is_valid(const struct ovsrec_flow_sample_collector_set *fscs,
> - const struct bridge *br)
> +ovsrec_fscs_is_valid_ipfix(const struct ovsrec_flow_sample_collector_set
> *fscs,
> + const struct bridge *br)
> {
> return ovsrec_ipfix_is_valid(fscs->ipfix) && fscs->bridge == br->cfg;
> }
> @@ -1558,7 +1561,7 @@ bridge_configure_ipfix(struct bridge *br)
> const char *virtual_obs_id;
>
> OVSREC_FLOW_SAMPLE_COLLECTOR_SET_FOR_EACH(fe_cfg, idl) {
> -if (ovsrec_fscs_is_valid(fe_cfg, br)) {
> +if (ovsrec_fscs_is_valid_ipfix(fe_cfg, br)) {
> n_fe_opts++;
> }
> }
> @@ -1621,7 +1624,7 @@ bridge_configure_ipfix(struct bridge *br)
> fe_opts = xcalloc(n_fe_opts, sizeof *fe_opts);
> opts = fe_opts;
> OVSREC_FLOW_SAMPLE_COLLECTOR_SET_FOR_EACH(fe_cfg, idl) {
> -if (ovsrec_fscs_is_valid(fe_cfg, br)) {
> +if (ovsrec_fscs_is_valid_ipfix(fe_cfg, br)) {
> opts->collector_set_id = fe_cfg->id;
> sset_init(&opts->targets);
> sset_add_array(&opts->targets, fe_cfg->ipfix->targets,
> @@ -1667,6 +1670,71 @@ bridge_configure_ipfix(struct bridge *br)
> }
> }
>
> +/* Returns whether a Flow_Sample_Collector_Set row contains a valid local
> + * sampling configuration. */
> +static bool
> +ovsrec_fscs_is_valid_local(const struct ovsrec_flow_sample_collector_set
> *fscs,
> + const struct bridge *br)
> +{
> +return fscs->local_group_id && fscs->n_local_group_id == 1 &&
> + fscs->bridge == br->cfg;
> +}
> +
> +/* Set local sample configuration on 'br'. */
> +static void
> +bridge_configure_lsample(struct bridge *br)
> +{
> +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> +const struct ovsrec_flow_sample_collector_set *fscs;
> +struct ofproto_lsample_options *opts_array, *opts;
> +size_t n_opts = 0;
> +int ret;
> +
> +/* Iterate the Flow_Sample_Collector_Set table twice.
> + * First to get the number of valid configuration entries, then to
> process
> + * each of them and build an array of options. */
> +OVSREC_FLOW_SAMPLE_COLLECTOR_SET_FOR_EACH (fscs, idl) {
> +if (ovsrec_fscs_is_valid_local(fscs, br)) {
> +n_opts ++;
> +}
> +}
> +
> +if (n_opts == 0) {
> +ofproto_set_local_sample(br->ofproto, NULL, 0);
> +return;
Re: [ovs-dev] [PATCH v1 04/13] ofproto: Add ofproto-dpif-lsample.
On 7 Jul 2024, at 22:08, Adrian Moreno wrote:
> Add a new resource in ofproto-dpif and the corresponding API in
> ofproto_provider.h to represent and local sampling configuration.
>
> Signed-off-by: Adrian Moreno
Some small nits below.
//Eelco
> ---
> ofproto/automake.mk| 2 +
> ofproto/ofproto-dpif-lsample.c | 187 +
> ofproto/ofproto-dpif-lsample.h | 36 +++
> ofproto/ofproto-dpif.c | 38 +++
> ofproto/ofproto-dpif.h | 1 +
> ofproto/ofproto-provider.h | 9 ++
> ofproto/ofproto.c | 12 +++
> ofproto/ofproto.h | 8 ++
> 8 files changed, 293 insertions(+)
> create mode 100644 ofproto/ofproto-dpif-lsample.c
> create mode 100644 ofproto/ofproto-dpif-lsample.h
>
> diff --git a/ofproto/automake.mk b/ofproto/automake.mk
> index 7c08b563b..cb1361b8a 100644
> --- a/ofproto/automake.mk
> +++ b/ofproto/automake.mk
> @@ -30,6 +30,8 @@ ofproto_libofproto_la_SOURCES = \
> ofproto/ofproto-dpif.h \
> ofproto/ofproto-dpif-ipfix.c \
> ofproto/ofproto-dpif-ipfix.h \
> + ofproto/ofproto-dpif-lsample.c \
> + ofproto/ofproto-dpif-lsample.h \
> ofproto/ofproto-dpif-mirror.c \
> ofproto/ofproto-dpif-mirror.h \
> ofproto/ofproto-dpif-monitor.c \
> diff --git a/ofproto/ofproto-dpif-lsample.c b/ofproto/ofproto-dpif-lsample.c
> new file mode 100644
> index 0..d675a116f
> --- /dev/null
> +++ b/ofproto/ofproto-dpif-lsample.c
> @@ -0,0 +1,187 @@
> +/*
> + * Copyright (c) 2024 Red Hat, Inc.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include
> +#include "ofproto-dpif-lsample.h"
> +
> +#include "cmap.h"
> +#include "hash.h"
> +#include "ofproto.h"
> +#include "openvswitch/thread.h"
> +
> +/* Dpif local sampling.
> + *
> + * Thread safety: dpif_lsample allows lockless concurrent reads of local
> + * sampling exporters as long as the following restrictions are met:
> + * 1) While the last reference is being dropped, i.e: a thread is calling
> + * "dpif_lsample_unref" on the last reference, other threads cannot call
> + * "dpif_lsample_ref".
> + * 2) Threads do not quiese while holding references to internal
> + * lsample_exporter objects.
> + */
> +
> +struct dpif_lsample {
> +struct cmap exporters; /* Contains lsample_exporter_node
> instances
> + indexed by collector_set_id. */
> +struct ovs_mutex mutex; /* Protects concurrent insertion/deletion
> + * of exporters. */
> +
> +struct ovs_refcount ref_cnt;/* Controls references to this instance.
> */
> +};
> +
> +struct lsample_exporter {
> +struct ofproto_lsample_options options;
> +};
> +
> +struct lsample_exporter_node {
> +struct cmap_node node; /* In dpif_lsample->exporters. */
> +struct lsample_exporter exporter;
> +};
> +
> +static void
> +dpif_lsample_delete_exporter(struct dpif_lsample *lsample,
> + struct lsample_exporter_node *node)
> +{
> +ovs_mutex_lock(&lsample->mutex);
> +cmap_remove(&lsample->exporters, &node->node,
> +hash_int(node->exporter.options.collector_set_id, 0));
> +ovs_mutex_unlock(&lsample->mutex);
> +
> +ovsrcu_postpone(free, node);
> +}
> +
> +/* Adds an exporter with the provided options which are copied. */
> +static struct lsample_exporter_node *
> +dpif_lsample_add_exporter(struct dpif_lsample *lsample,
> + const struct ofproto_lsample_options *options)
> +{
> +struct lsample_exporter_node *node;
> +
> +node = xzalloc(sizeof *node);
> +node->exporter.options = *options;
> +
> +ovs_mutex_lock(&lsample->mutex);
> +cmap_insert(&lsample->exporters, &node->node,
> +hash_int(options->collector_set_id, 0));
> +ovs_mutex_unlock(&lsample->mutex);
> +
> +return node;
> +}
> +
> +static struct lsample_exporter_node *
> +dpif_lsample_find_exporter_node(const struct dpif_lsample *lsample,
> +const uint32_t collector_set_id)
> +{
> +struct lsample_exporter_node *node;
> +
> +CMAP_FOR_EACH_WITH_HASH (node, node,
> +hash_int(collector_set_id, 0),
> +&lsample->exporters) {
> +if (node->exporter.options.collector_set_id == collector_set_id) {
> +return node;
> +
Re: [ovs-dev] [PATCH v1 01/13] ofproto-dpif: Allow forcing dp features.
On 4 Jul 2024, at 9:52, Adrian Moreno wrote:
> Datapath features can be set with dpif/set-dp-features unixctl command.
> This command is not docummented and therefore not supported in
documented
> production but only useful for unit tests.
>
> A limitation was put in place originally to avoid enabling features at
> runtime that were disabled at boot time to avoid breaking the datapath
> in unexpected ways.
>
> But, considering users should not use this command and it should only be
> used for testing, we can assume whoever runs it knows what they are
> doing. Therefore, the limitation should be bypass-able.
>
> This patch adds a "--force" flag to the unixctl command to allow
> bypassing the mentioned limitation.
Small nit below, the rest looks good to me. If this is the only change in the
next series you can add my ACK.
Cheers,
Eelco
> Signed-off-by: Adrian Moreno
> ---
> ofproto/ofproto-dpif.c | 37 +++--
> 1 file changed, 27 insertions(+), 10 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index fcd7cd753..4712d10a8 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -6432,7 +6432,8 @@ display_support_field(const char *name,
> static bool
> dpif_set_support(struct dpif_backer_support *rt_support,
> struct dpif_backer_support *bt_support,
> - const char *name, const char *value, struct ds *ds)
> + const char *name, const char *value, bool force,
> + struct ds *ds)
> {
> struct shash all_fields = SHASH_INITIALIZER(&all_fields);
> struct dpif_support_field *field;
> @@ -6484,8 +6485,8 @@ dpif_set_support(struct dpif_backer_support *rt_support,
>
> if (field->type == DPIF_SUPPORT_FIELD_bool) {
> if (!strcasecmp(value, "true")) {
> -if (*(bool *)field->bt_ptr) {
> -*(bool *)field->rt_ptr = true;
> +if (*(bool *) field->bt_ptr || force) {
> +*(bool *) field->rt_ptr = true;
> changed = true;
> } else {
> ds_put_cstr(ds, "Can not enable features not supported by
> the datapth");
> @@ -6720,21 +6721,36 @@ ofproto_unixctl_dpif_set_dp_features(struct
> unixctl_conn *conn,
> void *aux OVS_UNUSED)
> {
> struct ds ds = DS_EMPTY_INITIALIZER;
> -const char *br = argv[1];
> +struct ofproto_dpif *ofproto;
> +bool changed, force = false;
> const char *name, *value;
> -struct ofproto_dpif *ofproto = ofproto_dpif_lookup_by_name(br);
> -bool changed;
> +const char *br = NULL;
Don't think we have to initialize br here.
> +
> +if (!strcmp(argv[1], "--force")) {
> +force = true;
> +if (argc > 2) {
> +br = argv[2];
> +name = argc > 3 ? argv[3] : NULL;
> +value = argc > 4 ? argv[4] : NULL;
> +} else {
> +unixctl_command_reply_error(conn, "bridge not specified");
> +return;
> +}
> +} else {
> +br = argv[1];
> +name = argc > 2 ? argv[2] : NULL;
> +value = argc > 3 ? argv[3] : NULL;
> +}
>
> +ofproto = ofproto_dpif_lookup_by_name(br);
> if (!ofproto) {
> unixctl_command_reply_error(conn, "no such bridge");
> return;
> }
>
> -name = argc > 2 ? argv[2] : NULL;
> -value = argc > 3 ? argv[3] : NULL;
> changed = dpif_set_support(&ofproto->backer->rt_support,
> &ofproto->backer->bt_support,
> - name, value, &ds);
> + name, value, force, &ds);
> if (changed) {
> xlate_set_support(ofproto, &ofproto->backer->rt_support);
> udpif_flush(ofproto->backer->udpif);
> @@ -6777,7 +6793,8 @@ ofproto_unixctl_init(void)
> unixctl_command_register("dpif/dump-flows",
> "[-m] [--names | --no-names] bridge", 1,
> INT_MAX,
> ofproto_unixctl_dpif_dump_flows, NULL);
> -unixctl_command_register("dpif/set-dp-features", "bridge", 1, 3 ,
> +unixctl_command_register("dpif/set-dp-features",
> + "[--force] bridge [feature] [value]", 1, 4 ,
> ofproto_unixctl_dpif_set_dp_features, NULL);
> }
>
> --
> 2.45.2
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1 03/13] ofproto_dpif: Check for psample support.
On 4 Jul 2024, at 9:52, Adrian Moreno wrote:
> Only kernel datapath supports this action so add a function in dpif.c
> that checks for that.
One small nit below, rest looks good. If this is the only change in the
next series you can add my ACK.
//Eelco
> Signed-off-by: Adrian Moreno
> ---
> lib/dpif.c | 7 +++
> lib/dpif.h | 1 +
> ofproto/ofproto-dpif.c | 43 ++
> ofproto/ofproto-dpif.h | 7 ++-
> 4 files changed, 57 insertions(+), 1 deletion(-)
>
> diff --git a/lib/dpif.c b/lib/dpif.c
> index 71728badc..0a964ba89 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -1953,6 +1953,13 @@ dpif_supports_lb_output_action(const struct dpif *dpif)
> return dpif_is_netdev(dpif);
> }
>
> +bool
> +dpif_may_support_psample(const struct dpif *dpif)
> +{
> +/* Userspace datapath does not support this action. */
> +return !dpif_is_netdev(dpif);
> +}
> +
> /* Meters */
> void
> dpif_meter_get_features(const struct dpif *dpif,
> diff --git a/lib/dpif.h b/lib/dpif.h
> index a764e8a59..6bef7d5b3 100644
> --- a/lib/dpif.h
> +++ b/lib/dpif.h
> @@ -941,6 +941,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_may_support_explicit_drop_action(const struct dpif *);
> +bool dpif_may_support_psample(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 4712d10a8..94c84d697 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -873,6 +873,12 @@ ovs_lb_output_action_supported(struct ofproto_dpif
> *ofproto)
> return ofproto->backer->rt_support.lb_output_action;
> }
>
> +bool
> +ovs_psample_supported(struct ofproto_dpif *ofproto)
> +{
> +return ofproto->backer->rt_support.psample;
> +}
> +
> /* Tests whether 'backer''s datapath supports recirculation. Only newer
> * datapaths support OVS_KEY_ATTR_RECIRC_ID in keys. We need to disable some
> * features on older datapaths that don't support this feature.
> @@ -1609,6 +1615,42 @@ check_add_mpls(struct dpif_backer *backer)
> return supported;
> }
>
> +/* Tests whether 'backer''s datapath supports the OVS_ACTION_ATTR_PSAMPLE
> + * action. */
> +static bool
> +check_psample(struct dpif_backer *backer)
> +{
> +uint8_t cookie[OVS_PSAMPLE_COOKIE_MAX_SIZE];
> +struct odputil_keybuf keybuf;
> +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_init(&actions, 64);
> +
> +/* Generate a random max-size cookie. */
> +random_bytes(cookie, sizeof(cookie));
> +
> +odp_put_psample_action(&actions, 10, cookie, sizeof cookie);
> +
> +supported = dpif_may_support_psample(backer->dpif) &&
> +dpif_probe_feature(backer->dpif, "psample", &key, &actions, NULL);
> +
> +ofpbuf_uninit(&actions);
> +VLOG_INFO("%s: Datapath %s psample", dpif_name(backer->dpif),
> + supported ? "supports" : "does not support");
> +return supported;
> +}
> +
> #define CHECK_FEATURE__(NAME, SUPPORT, FIELD, VALUE, ETHTYPE) \
> static bool \
> check_##NAME(struct dpif_backer *backer)\
> @@ -1698,6 +1740,7 @@ check_support(struct dpif_backer *backer)
> dpif_supports_lb_output_action(backer->dpif);
> backer->rt_support.ct_zero_snat = dpif_supports_ct_zero_snat(backer);
> backer->rt_support.add_mpls = check_add_mpls(backer);
> +backer->rt_support.psample = check_psample(backer);
>
> /* Flow fields. */
> backer->rt_support.odp.ct_state = check_ct_state(backer);
> diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
> index d33f73df8..bc7a19dab 100644
> --- a/ofproto/ofproto-dpif.h
> +++ b/ofproto/ofproto-dpif.h
> @@ -213,7 +213,10 @@ struct group_dpif *group_dpif_lookup(struct ofproto_dpif
> *,
> DPIF_SUPPORT_FIELD(bool, ct_zero_snat, "Conntrack all-zero IP SNAT")\
> \
> /* True if the datapath supports add_mpls action. */\
> -DPIF_SUPPORT_FIELD(bool, add_mpls, "MPLS Label add")
> +DPIF_SUPPORT_FIELD(bool, add_mpls, "MPLS Label add")\
> +\
> +/* True if the datapath supports psample action. */ \
> +DPIF_SUPPORT_FIELD(bool, psample, "psample")
>
>
> /* Stores the various features
Re: [ovs-dev] [PATCH v1 02/13] odp-util: Add support OVS_ACTION_ATTR_PSAMPLE.
On 4 Jul 2024, at 9:52, Adrian Moreno wrote:
> Add support for parsing and formatting the new action.
>
> Also, flag OVS_ACTION_ATTR_SAMPLE as requiring datapath assistance if it
> contains a nested OVS_ACTION_ATTR_PSAMPLE. The reason is that the
> sampling rate form the parent "sample" is made available to the nested
form -> from
> "psample" by the kernel.
Two small comments below, the rest looks good.
> Signed-off-by: Adrian Moreno
> ---
> include/linux/openvswitch.h | 28 +++
> lib/dpif-netdev.c| 1 +
> lib/dpif.c | 1 +
> lib/odp-execute.c| 25 +-
> lib/odp-util.c | 93
> lib/odp-util.h | 3 ++
> ofproto/ofproto-dpif-ipfix.c | 1 +
> ofproto/ofproto-dpif-sflow.c | 1 +
> python/ovs/flow/odp.py | 8
> tests/odp.at | 16 +++
> 10 files changed, 176 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
> index d9fb991ef..0023b65fb 100644
> --- a/include/linux/openvswitch.h
> +++ b/include/linux/openvswitch.h
> @@ -992,6 +992,31 @@ struct check_pkt_len_arg {
> };
> #endif
>
> +#define OVS_PSAMPLE_COOKIE_MAX_SIZE 16
> +/**
> + * enum ovs_pample_attr - Attributes for %OVS_ACTION_ATTR_PSAMPLE
> + * action.
> + *
> + * @OVS_PSAMPLE_ATTR_GROUP: 32-bit number to identify the source of the
> + * sample.
> + * @OVS_PSAMPLE_ATTR_COOKIE: An optional variable-length binary cookie that
> + * contains user-defined metadata. The maximum length is
> + * OVS_PSAMPLE_COOKIE_MAX_SIZE bytes.
> + *
> + * Sends the packet to the psample multicast group with the specified group
> and
> + * cookie. It is possible to combine this action with the
> + * %OVS_ACTION_ATTR_TRUNC action to limit the size of the sample.
> + */
> +enum ovs_psample_attr {
> +OVS_PSAMPLE_ATTR_GROUP = 1,/* u32 number. */
> +OVS_PSAMPLE_ATTR_COOKIE, /* Optional, user specified cookie. */
> +
> +/* private: */
None of the other definitions have this private marking, so I see no need to
start adding it here.
> +__OVS_PSAMPLE_ATTR_MAX
> +};
> +
> +#define OVS_PSAMPLE_ATTR_MAX (__OVS_PSAMPLE_ATTR_MAX - 1)
> +
> /**
> * enum ovs_action_attr - Action types.
> *
> @@ -1056,6 +1081,8 @@ struct check_pkt_len_arg {
> * of l3 tunnel flag in the tun_flags field of OVS_ACTION_ATTR_ADD_MPLS
> * argument.
> * @OVS_ACTION_ATTR_DROP: Explicit drop action.
> + * @OVS_ACTION_ATTR_PSAMPLE: Send a sample of the packet to external
> observers
> + * via psample.
> */
>
> enum ovs_action_attr {
> @@ -1087,6 +1114,7 @@ enum ovs_action_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. */
> + OVS_ACTION_ATTR_PSAMPLE, /* Nested OVS_PSAMPLE_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 c7f9e1490..f0594e5f5 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -9519,6 +9519,7 @@ dp_execute_cb(void *aux_, struct dp_packet_batch
> *packets_,
> case OVS_ACTION_ATTR_DROP:
> case OVS_ACTION_ATTR_ADD_MPLS:
> case OVS_ACTION_ATTR_DEC_TTL:
> +case OVS_ACTION_ATTR_PSAMPLE:
> case __OVS_ACTION_ATTR_MAX:
> OVS_NOT_REACHED();
> }
> diff --git a/lib/dpif.c b/lib/dpif.c
> index 23eb18495..71728badc 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -1192,6 +1192,7 @@ dpif_execute_helper_cb(void *aux_, struct
> dp_packet_batch *packets_,
> case OVS_ACTION_ATTR_TUNNEL_PUSH:
> case OVS_ACTION_ATTR_TUNNEL_POP:
> case OVS_ACTION_ATTR_USERSPACE:
> +case OVS_ACTION_ATTR_PSAMPLE:
> case OVS_ACTION_ATTR_RECIRC: {
> struct dpif_execute execute;
> struct ofpbuf execute_actions;
> diff --git a/lib/odp-execute.c b/lib/odp-execute.c
> index 081e4d432..15577d539 100644
> --- a/lib/odp-execute.c
> +++ b/lib/odp-execute.c
> @@ -818,13 +818,13 @@ requires_datapath_assistance(const struct nlattr *a)
> case OVS_ACTION_ATTR_RECIRC:
> case OVS_ACTION_ATTR_CT:
> case OVS_ACTION_ATTR_METER:
> +case OVS_ACTION_ATTR_PSAMPLE:
> return true;
>
> case OVS_ACTION_ATTR_SET:
> case OVS_ACTION_ATTR_SET_MASKED:
> case OVS_ACTION_ATTR_PUSH_VLAN:
> case OVS_ACTION_ATTR_POP_VLAN:
> -case OVS_ACTION_ATTR_SAMPLE:
> case OVS_ACTION_ATTR_HASH:
> case OVS_ACTION_ATTR_PUSH_MPLS:
> case OVS_ACTION_ATTR_POP_MPLS:
> @@ -841,6 +841,28 @@ requires_datapath_assistance(const struct nlattr *a)
> case OVS_ACTION_ATTR_DROP:
> return false;
>
> +case OVS_ACTION_ATTR_SAMPLE: {
> +/* Nested "psample" actions rely on the datapath executing the
> + * parent "sample", storing the probability and making it availabl
Re: [ovs-dev] [PATCH v14 3/6] appctl: Add option '--pretty' for pretty-printing JSON output.
Bleep bloop. Greetings Jakob Meng, 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 WARNING: Line lacks whitespace around operator #122 FILE: utilities/ovs-appctl.c:157: Requires: --format=json.\n\ Lines checked: 207, 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 v14 1/6] Add global option for JSON output to ovs-appctl.
Bleep bloop. Greetings Jakob Meng, 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 WARNING: Line lacks whitespace around operator #657 FILE: utilities/ovs-appctl.c:152: -f, --format=FMT Output format. One of: 'json', or 'text'\n\ Lines checked: 775, 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
[ovs-dev] [PATCH v14 4/6] python: Add option for pretty-printing JSON output to appctl.py.
From: Jakob Meng
With the '--pretty' option, appctl.py will now print JSON output in a
more readable fashion, i.e. with additional line breaks, spaces and
sorted dictionary keys. The pretty-printed output from appctl.py is not
strictly the same as with ovs-appctl because of both use different
pretty-printing implementations.
Signed-off-by: Jakob Meng
---
tests/appctl.py | 15 ---
tests/unixctl-py.at | 6 ++
2 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/tests/appctl.py b/tests/appctl.py
index 4aca7efbc..5f4b2754a 100644
--- a/tests/appctl.py
+++ b/tests/appctl.py
@@ -37,11 +37,12 @@ def connect_to_target(target):
return client
-def reply_to_string(reply, fmt=ovs.unixctl.UnixctlOutputFormat.TEXT):
+def reply_to_string(reply, fmt=ovs.unixctl.UnixctlOutputFormat.TEXT,
+fmt_flags={}):
if fmt == ovs.unixctl.UnixctlOutputFormat.TEXT:
body = str(reply)
else:
-body = ovs.json.to_string(reply)
+body = ovs.json.to_string(reply, **fmt_flags)
if body and not body.endswith("\n"):
body += "\n"
@@ -66,13 +67,21 @@ def main():
choices=[fmt.name.lower()
for fmt in ovs.unixctl.UnixctlOutputFormat],
type=str.lower)
+parser.add_argument("--pretty", action="store_true",
+help="Format the output in a more readable fashion."
+ " Requires: --format json.")
args = parser.parse_args()
+if (args.format != ovs.unixctl.UnixctlOutputFormat.JSON.name.lower()
+and args.pretty):
+ovs.util.ovs_fatal(0, "--pretty is supported with --format json only")
+
signal_alarm(int(args.timeout) if args.timeout else None)
ovs.vlog.Vlog.init()
target = args.target
format = ovs.unixctl.UnixctlOutputFormat[args.format.upper()]
+format_flags = dict(pretty=True) if args.pretty else {}
client = connect_to_target(target)
if format != ovs.unixctl.UnixctlOutputFormat.TEXT:
@@ -97,7 +106,7 @@ def main():
sys.exit(2)
else:
assert result is not None
-sys.stdout.write(reply_to_string(result, format))
+sys.stdout.write(reply_to_string(result, format, format_flags))
if __name__ == '__main__':
diff --git a/tests/unixctl-py.at b/tests/unixctl-py.at
index f4a664dc0..ae8bd5ad1 100644
--- a/tests/unixctl-py.at
+++ b/tests/unixctl-py.at
@@ -119,6 +119,12 @@ AT_CHECK_UNQUOTED([PYAPPCTL_PY -t test-unixctl.py --format
json version], [0], [
AT_CHECK_UNQUOTED([PYAPPCTL_PY -t test-unixctl.py --format JSON version], [0],
[dnl
{"reply":"$(cat expout)","reply-format":"plain"}
])
+AT_CHECK_UNQUOTED([PYAPPCTL_PY -t test-unixctl.py --format json --pretty
version], [0], [dnl
+{
+ "reply":"$(cat expout)",
+ "reply-format":"plain"
+}
+])
AT_CHECK([APPCTL -t test-unixctl.py echo robot ninja], [0], [stdout])
AT_CHECK([cat stdout | sed -e "s/u'/'/g"], [0], [dnl
--
2.39.2
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v14 6/6] ofproto: Add JSON output for 'dpif/show' command.
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
---
NEWS | 2 +-
ofproto/ofproto-dpif.c | 126 -
tests/ofproto-dpif.at | 40 +
3 files changed, 153 insertions(+), 15 deletions(-)
diff --git a/NEWS b/NEWS
index feebae86e..d18693315 100644
--- a/NEWS
+++ b/NEWS
@@ -6,7 +6,7 @@ Post-v3.3.0
* 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.
- * 'list-commands' now supports output in JSON format.
+ * 'dpif/show' and 'list-commands' now support output in JSON format.
- Userspace datapath:
* Conntrack now supports 'random' flag for selecting ports in a range
while natting and 'persistent' flag for selection of the IP address
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index fcd7cd753..87dfb0043 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -28,6 +28,7 @@
#include "fail-open.h"
#include "guarded-list.h"
#include "hmapx.h"
+#include "openvswitch/json.h"
#include "lacp.h"
#include "learn.h"
#include "mac-learning.h"
@@ -6519,19 +6520,108 @@ done:
return changed;
}
-static void
-dpif_show_backer(const struct dpif_backer *backer, struct ds *ds)
+static struct json *
+dpif_show_backer_json(struct json *backers, const struct dpif_backer *backer)
{
+struct json *json_backer = json_object_create();
+
+/* Add datapath as new JSON object using its name as key. */
+json_object_put(backers, dpif_name(backer->dpif), json_backer);
+
+/* Add datapath's stats under "stats" key. */
+struct json *json_dp_stats = json_object_create();
+struct dpif_dp_stats dp_stats;
+
+dpif_get_dp_stats(backer->dpif, &dp_stats);
+json_object_put_format(json_dp_stats, "hit", "%"PRIu64, dp_stats.n_hit);
+json_object_put_format(json_dp_stats, "missed", "%"PRIu64,
+ dp_stats.n_missed);
+json_object_put(json_backer, "stats", json_dp_stats);
+
+/* Add datapath's bridges under "bridges" key. */
+struct json *json_dp_bridges = json_object_create();
+
+struct shash ofproto_shash = SHASH_INITIALIZER(&ofproto_shash);
+free(get_ofprotos(&ofproto_shash));
+
+struct shash_node *node;
+SHASH_FOR_EACH (node, &ofproto_shash) {
+struct ofproto_dpif *ofproto = node->data;
+
+if (ofproto->backer != backer) {
+continue;
+}
+
+/* Add bridge to "bridges" dictionary using its name as key. */
+struct json *json_ofproto = json_object_create();
+
+/* Add bridge ports to the current bridge dictionary. */
+const struct shash_node *port;
+SHASH_FOR_EACH (port, &ofproto->up.port_by_name) {
+/* Add bridge port to a bridge's dict using port name as key. */
+struct json *json_ofproto_port = json_object_create();
+struct ofport *ofport = port->data;
+
+/* Add OpenFlow port associated with a bridge port. */
+json_object_put_format(json_ofproto_port, "ofport", "%"PRIu32,
+ ofport->ofp_port);
+
+/* Add bridge port number. */
+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, "port_no",
+ "%"PRIu32, odp_port);
+} else {
+json_object_put_string(json_ofproto_port, "port_no", "none");
+}
+
+/* Add type of a bridge port. */
+json_object_put_string(json_ofproto_port, "type",
+ netdev_get_type(ofport->netdev));
+
+/* Add config entries for a bridge port. */
+
+struct smap config = SMAP_INITIALIZER(&config);
+
+if (!netdev_get_config(ofport->netdev, &config)
+&& smap_count(&config)) {
+struct json *json_port_config = json_object_create();
+struct smap_node *cfg_node;
+
+SMAP_FOR_EACH (cfg_node, &config) {
+json_object_put_string(json_port_config, cfg_node->key,
+ cfg_node->value);
+}
+json_object_put(json_ofproto_port, "config", json_port_config);
+}
+smap_destroy(&config);
+
+json_object_put(json_ofproto, netdev_get_name(ofport->netdev),
+json_ofproto_port);
+} /* End of bridge port(s). */
+
+json_object_put(json_dp_bridges, ofpr
[ovs-dev] [PATCH v14 3/6] appctl: Add option '--pretty' for pretty-printing JSON output.
From: Jakob Meng
With the '--pretty' option, ovs-appctl will now print JSON output in a
more readable fashion, i.e. with additional line breaks, spaces and
sorted dictionary keys.
Signed-off-by: Jakob Meng
---
Documentation/ref/ovs-appctl.8.rst | 8 +++
NEWS | 1 +
tests/ovs-vswitchd.at | 6 ++
utilities/ovs-appctl.c | 34 --
4 files changed, 42 insertions(+), 7 deletions(-)
diff --git a/Documentation/ref/ovs-appctl.8.rst
b/Documentation/ref/ovs-appctl.8.rst
index 148cc7632..7054cf559 100644
--- a/Documentation/ref/ovs-appctl.8.rst
+++ b/Documentation/ref/ovs-appctl.8.rst
@@ -9,6 +9,7 @@ Synopsis
[``--target=`` | ``-t`` ]
[``--timeout=`` | ``-T`` ]
[``--format=`` | ``-f`` ]
+[``--pretty``]
[...]
``ovs-appctl --help``
@@ -79,6 +80,13 @@ In normal use only a single option is accepted:
{"reply-format":"plain","reply":"$PLAIN_TEXT_HERE"}
+* ``--pretty``
+
+ By default, JSON output is printed as compactly as possible. This option
+ causes JSON in output to be printed in a more readable fashion. For
+ example, members of objects and elements of arrays are printed one
+ per line, with indentation. Requires ``--format=json``.
+
Common Commands
===
diff --git a/NEWS b/NEWS
index c750ebae2..d903d2f74 100644
--- a/NEWS
+++ b/NEWS
@@ -5,6 +5,7 @@ Post-v3.3.0
- ovs-appctl:
* 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.
- Userspace datapath:
* Conntrack now supports 'random' flag for selecting ports in a range
while natting and 'persistent' flag for selection of the IP address
diff --git a/tests/ovs-vswitchd.at b/tests/ovs-vswitchd.at
index b1ae1ae1e..0f7a6085e 100644
--- a/tests/ovs-vswitchd.at
+++ b/tests/ovs-vswitchd.at
@@ -276,4 +276,10 @@ AT_CHECK_UNQUOTED([ovs-appctl --format json version], [0],
[dnl
{"reply":"$ovs_version","reply-format":"plain"}
])
+AT_CHECK_UNQUOTED([ovs-appctl --format json --pretty version], [0], [dnl
+{
+ "reply": "$ovs_version",
+ "reply-format": "plain"}
+])
+
AT_CLEANUP
diff --git a/utilities/ovs-appctl.c b/utilities/ovs-appctl.c
index 721698755..682ee100c 100644
--- a/utilities/ovs-appctl.c
+++ b/utilities/ovs-appctl.c
@@ -40,13 +40,15 @@ static void usage(void);
/* Parsed command line args. */
struct cmdl_args {
enum unixctl_output_fmt format;
+unsigned int format_flags;
char *target;
};
static struct cmdl_args *cmdl_args_create(void);
static struct cmdl_args *parse_command_line(int argc, char *argv[]);
static struct jsonrpc *connect_to_target(const char *target);
-static char *reply_to_string(struct json *reply, enum unixctl_output_fmt fmt);
+static char *reply_to_string(struct json *reply, enum unixctl_output_fmt fmt,
+ unsigned int fmt_flags);
int
main(int argc, char *argv[])
@@ -84,7 +86,7 @@ main(int argc, char *argv[])
if (cmd_error) {
jsonrpc_close(client);
-msg = reply_to_string(cmd_error, UNIXCTL_OUTPUT_FMT_TEXT);
+msg = reply_to_string(cmd_error, UNIXCTL_OUTPUT_FMT_TEXT, 0);
fputs(msg, stderr);
free(msg);
ovs_error(0, "%s: server returned an error", args->target);
@@ -108,13 +110,13 @@ main(int argc, char *argv[])
if (cmd_error) {
jsonrpc_close(client);
-msg = reply_to_string(cmd_error, UNIXCTL_OUTPUT_FMT_TEXT);
+msg = reply_to_string(cmd_error, UNIXCTL_OUTPUT_FMT_TEXT, 0);
fputs(msg, stderr);
free(msg);
ovs_error(0, "%s: server returned an error", args->target);
exit(2);
} else if (cmd_result) {
-msg = reply_to_string(cmd_result, args->format);
+msg = reply_to_string(cmd_result, args->format, args->format_flags);
fputs(msg, stdout);
free(msg);
} else {
@@ -151,6 +153,8 @@ Other options:\n\
--timeout=SECS wait at most SECS seconds for a response\n\
-f, --format=FMT Output format. One of: 'json', or 'text'\n\
(default: text)\n\
+ --pretty Format the output in a more readable fashion.\n\
+ Requires: --format=json.\n\
-h, --help Print this helpful information\n\
-V, --version Display ovs-appctl version information\n",
program_name, program_name);
@@ -163,6 +167,7 @@ cmdl_args_create(void)
struct cmdl_args *args = xmalloc(sizeof *args);
args->format = UNIXCTL_OUTPUT_FMT_TEXT;
+args->format_flags = 0;
args->target = NULL;
return args;
@@ -173,7 +178,8 @@ parse_command_line(int argc, char *argv[])
{
enum {
OPT_START = UCHAR_MAX + 1,
-VLOG_OPTION_ENUMS
+OPT_PRETTY,
+VLOG_OPTION_ENUMS,
};
static const struct option long_options[] = {
{"target", req
[ovs-dev] [PATCH v14 5/6] vswitchd: Add JSON output for 'list-commands' command.
From: Jakob Meng
The 'list-commands' command now supports machine-readable JSON output
in addition to the plain-text output for humans.
Reported-at: https://bugzilla.redhat.com/1824861
Signed-off-by: Jakob Meng
---
NEWS | 1 +
lib/unixctl.c | 44 +--
tests/ovs-vswitchd.at | 15 +++
3 files changed, 46 insertions(+), 14 deletions(-)
diff --git a/NEWS b/NEWS
index d903d2f74..feebae86e 100644
--- a/NEWS
+++ b/NEWS
@@ -6,6 +6,7 @@ Post-v3.3.0
* 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.
+ * 'list-commands' now supports output in JSON format.
- Userspace datapath:
* Conntrack now supports 'random' flag for selecting ports in a range
while natting and 'persistent' flag for selection of the IP address
diff --git a/lib/unixctl.c b/lib/unixctl.c
index e7ce77e2c..c060e8659 100644
--- a/lib/unixctl.c
+++ b/lib/unixctl.c
@@ -95,24 +95,40 @@ static void
unixctl_list_commands(struct unixctl_conn *conn, int argc OVS_UNUSED,
const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
{
-struct ds ds = DS_EMPTY_INITIALIZER;
-const struct shash_node **nodes = shash_sort(&commands);
-size_t i;
+if (unixctl_command_get_output_format(conn) == UNIXCTL_OUTPUT_FMT_JSON) {
+struct json *json_commands = json_object_create();
+const struct shash_node *node;
-ds_put_cstr(&ds, "The available commands are:\n");
+SHASH_FOR_EACH (node, &commands) {
+const struct unixctl_command *command = node->data;
-for (i = 0; i < shash_count(&commands); i++) {
-const struct shash_node *node = nodes[i];
-const struct unixctl_command *command = node->data;
-
-if (command->usage) {
-ds_put_format(&ds, " %-23s %s\n", node->name, command->usage);
+if (command->usage) {
+json_object_put_string(json_commands, node->name,
+ command->usage);
+}
}
-}
-free(nodes);
+unixctl_command_reply_json(conn, json_commands);
+} else {
+struct ds ds = DS_EMPTY_INITIALIZER;
+const struct shash_node **nodes = shash_sort(&commands);
+size_t i;
-unixctl_command_reply(conn, ds_cstr(&ds));
-ds_destroy(&ds);
+ds_put_cstr(&ds, "The available commands are:\n");
+
+for (i = 0; i < shash_count(&commands); ++i) {
+const struct shash_node *node = nodes[i];
+const struct unixctl_command *command = node->data;
+
+if (command->usage) {
+ds_put_format(&ds, " %-23s %s\n", node->name,
+ command->usage);
+}
+}
+free(nodes);
+
+unixctl_command_reply(conn, ds_cstr(&ds));
+ds_destroy(&ds);
+}
}
static void
diff --git a/tests/ovs-vswitchd.at b/tests/ovs-vswitchd.at
index 0f7a6085e..730363e83 100644
--- a/tests/ovs-vswitchd.at
+++ b/tests/ovs-vswitchd.at
@@ -283,3 +283,18 @@ AT_CHECK_UNQUOTED([ovs-appctl --format json --pretty
version], [0], [dnl
])
AT_CLEANUP
+
+AT_SETUP([ovs-vswitchd list-commands])
+OVS_VSWITCHD_START
+
+AT_CHECK([ovs-appctl list-commands], [0], [ignore])
+AT_CHECK([ovs-appctl --format json list-commands], [0], [stdout])
+
+# Check that ovs-appctl prints a single line with a trailing newline.
+AT_CHECK([wc -l stdout], [0], [1 stdout
+])
+
+# Check that ovs-appctl prints a JSON document.
+AT_CHECK([ovstest test-json stdout], [0], [ignore])
+
+AT_CLEANUP
--
2.39.2
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v14 1/6] Add global option for JSON output to ovs-appctl.
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-appctl. It gains a global option '-f,--format' which changes it to
print a JSON document instead of plain-text for humans. For example, a
later patch implements support for
'ovs-appctl --format json dpif/show'. By default, the output format
is plain-text as before.
A new 'set-options' command has been added to lib/unixctl.c which
allows to change the output format of the commands executed afterwards
on the same socket connection. It is supposed to be run by ovs-appctl
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 afterwards it will call 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.
Two access functions unixctl_command_{get,set}_output_format() and a
unixctl_command_reply_json function have been added to lib/unixctl.h:
unixctl_command_get_output_format() is supposed to be used in commands
like 'dpif/show' to query the requested output format. When JSON output
has been selected, the unixctl_command_reply_json() function can be
used to return JSON objects to the client (ovs-appctl) instead of
plain-text with the unixctl_command_reply{,_error}() functions.
When JSON has been requested but a command has not implemented JSON
output the plain-text output will be wrapped in a provisional JSON
document with the following structure:
{"reply":"$PLAIN_TEXT_HERE","reply-format":"plain"}
Thus commands which have been executed successfully will not fail when
they try to render the output at a later stage.
A test for the 'version' command has been implemented which shows how
the provisional JSON document looks like in practice. For a cleaner
JSON document, the trailing newline has been moved from the program
version string to function ovs_print_version(). This way, the
plain-text output of the 'version' command has not changed.
Output formatting has been moved from unixctl_client_transact() in
lib/unixctl.c to utilities/ovs-appctl.c. The former merely returns the
JSON objects returned from the server and the latter is now responsible
for printing it properly.
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 chosen name also better aligns with ovsdb-client
where '-f,--format' controls output formatting.
Reported-at: https://bugzilla.redhat.com/1824861
Signed-off-by: Jakob Meng
---
Documentation/ref/ovs-appctl.8.rst | 12 ++
NEWS | 3 +
lib/unixctl.c | 180 ++---
lib/unixctl.h | 17 ++-
lib/util.c | 6 +-
python/ovs/unixctl/server.py | 3 -
tests/appctl.py| 5 +
tests/ovs-vswitchd.at | 12 ++
utilities/ovs-appctl.c | 135 +++---
9 files changed, 305 insertions(+), 68 deletions(-)
diff --git a/Documentation/ref/ovs-appctl.8.rst
b/Documentation/ref/ovs-appctl.8.rst
index 3ce02e984..148cc7632 100644
--- a/Documentation/ref/ovs-appctl.8.rst
+++ b/Documentation/ref/ovs-appctl.8.rst
@@ -8,6 +8,7 @@ Synopsis
``ovs-appctl``
[``--target=`` | ``-t`` ]
[``--timeout=`` | ``-T`` ]
+[``--format=`` | ``-f`` ]
[...]
``ovs-appctl --help``
@@ -67,6 +68,17 @@ In normal use only a single option is accepted:
runtime to approximately seconds. If the timeout expires,
``ovs-appctl`` exits with a ``SIGALRM`` signal.
+* ``-f `` or ``--format=``
+
+ Tells ``ovs-appctl`` which output format to use. By default, or with a
+ of ``text``, ``ovs-appctl`` will print plain-text for humans.
+ When is ``json``, ``ovs-appctl`` will return a JSON document.
+ When ``json`` is requested, but a command has not implemented JSON
+ output, the plain-text output will be wrapped in a provisional JSON
+ document with the following structure::
+
+{"reply-format":"plain","reply":"$PLAIN_TEXT_HERE"}
+
Common Commands
===
diff --git a/NEWS b/NEWS
index e0359b759..f182647c7 100644
--- a/NEWS
+++ b/NEWS
@@ -2,6 +2,9 @@ Post-v3.3.0
- Option '--mlockall' now only locks memory pages on fault, if possible.
This also makes it compatible with vHost Post-copy Live Migration.
+ - ovs-appctl:
+ * Added new option [-f|--format] to choose the output format, e.g. 'json'
+ or 'text' (by defa
[ovs-dev] [PATCH v14 2/6] python: Add option for JSON output to unixctl classes and appctl.py.
From: Jakob Meng
This patch introduces support for different output formats to Python
Unixctl* classes and appctl.py, similar to what the previous commit did
for ovs-appctl.
In particular, tests/appctl.py gains a global option '-f,--format'
which allows users to request JSON instead of plain-text for humans.
Reported-at: https://bugzilla.redhat.com/1824861
Signed-off-by: Jakob Meng
---
NEWS | 2 ++
python/ovs/unixctl/__init__.py | 8 +
python/ovs/unixctl/client.py | 5 ++--
python/ovs/unixctl/server.py | 53 +-
tests/appctl.py| 39 -
tests/unixctl-py.at| 7 +
6 files changed, 98 insertions(+), 16 deletions(-)
diff --git a/NEWS b/NEWS
index f182647c7..c750ebae2 100644
--- a/NEWS
+++ b/NEWS
@@ -19,6 +19,8 @@ Post-v3.3.0
per interface 'options:dpdk-lsc-interrupt' to 'false'.
- Python:
* Added custom transaction support to the Idl via add_op().
+ * Added support for different output formats like 'json' to Python's
+ unixctl classes.
v3.3.0 - 16 Feb 2024
diff --git a/python/ovs/unixctl/__init__.py b/python/ovs/unixctl/__init__.py
index 8ee312943..b05f3df72 100644
--- a/python/ovs/unixctl/__init__.py
+++ b/python/ovs/unixctl/__init__.py
@@ -12,6 +12,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.
+import enum
import sys
import ovs.util
@@ -19,6 +20,13 @@ import ovs.util
commands = {}
+@enum.unique
+# FIXME: Use @enum.verify(enum.NAMED_FLAGS) from Python 3.11 when available.
+class UnixctlOutputFormat(enum.IntFlag):
+TEXT = 1 << 0
+JSON = 1 << 1
+
+
class _UnixctlCommand(object):
def __init__(self, usage, min_args, max_args, callback, aux):
self.usage = usage
diff --git a/python/ovs/unixctl/client.py b/python/ovs/unixctl/client.py
index 8283f99bb..8a6fcb1b9 100644
--- a/python/ovs/unixctl/client.py
+++ b/python/ovs/unixctl/client.py
@@ -14,6 +14,7 @@
import os
+import ovs.json
import ovs.jsonrpc
import ovs.stream
import ovs.util
@@ -41,10 +42,10 @@ class UnixctlClient(object):
return error, None, None
if reply.error is not None:
-return 0, str(reply.error), None
+return 0, reply.error, None
else:
assert reply.result is not None
-return 0, None, str(reply.result)
+return 0, None, reply.result
def close(self):
self._conn.close()
diff --git a/python/ovs/unixctl/server.py b/python/ovs/unixctl/server.py
index d24a7092c..9a58a38d5 100644
--- a/python/ovs/unixctl/server.py
+++ b/python/ovs/unixctl/server.py
@@ -12,6 +12,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.
+import argparse
import copy
import errno
import os
@@ -35,6 +36,7 @@ class UnixctlConnection(object):
assert isinstance(rpc, ovs.jsonrpc.Connection)
self._rpc = rpc
self._request_id = None
+self._fmt = ovs.unixctl.UnixctlOutputFormat.TEXT
def run(self):
self._rpc.run()
@@ -63,10 +65,29 @@ class UnixctlConnection(object):
return error
def reply(self, body):
-self._reply_impl(True, body)
+assert body is None or isinstance(body, str)
+
+if body is None:
+body = ""
+
+if self._fmt == ovs.unixctl.UnixctlOutputFormat.JSON:
+body = {
+"reply-format": "plain",
+"reply": body
+}
+
+return self._reply_impl_json(True, body)
+
+def reply_json(self, body):
+self._reply_impl_json(True, body)
def reply_error(self, body):
-self._reply_impl(False, body)
+assert body is None or isinstance(body, str)
+
+if body is None:
+body = ""
+
+return self._reply_impl_json(False, body)
# Called only by unixctl classes.
def _close(self):
@@ -78,15 +99,11 @@ class UnixctlConnection(object):
if not self._rpc.get_backlog():
self._rpc.recv_wait(poller)
-def _reply_impl(self, success, body):
+def _reply_impl_json(self, success, body):
assert isinstance(success, bool)
-assert body is None or isinstance(body, str)
assert self._request_id is not None
-if body is None:
-body = ""
-
if success:
reply = Message.create_reply(body, self._request_id)
else:
@@ -133,6 +150,25 @@ def _unixctl_version(conn, unused_argv, version):
conn.reply(version)
+def _unixctl_set_options(conn, argv, unused_aux):
+assert isinstance(conn, UnixctlConnection)
+
+parser = argparse.ArgumentParser()
+parser.add_argument("--format", default="text",
+choices=[fmt.name.lower()
+ for fmt in ovs.unixctl.UnixctlOutputFormat],
+type=str.l
[ovs-dev] [PATCH v14 0/6] Add global option to output JSON from ovs-appctl cmds.
From: Jakob Meng v14 addresses all comments from Ilya Maximets for v13 [0]. Aside from polishing, appctl.py's "--format" arg will be case-insensitive, allowing 'json' and 'JSON' etc. Kudos to Ilya for holding up code quality in Open vSwitch ❤️ Best regards, Jakob [0] https://patchwork.ozlabs.org/project/openvswitch/list/?series=413880&state=%2A&archive=both Jakob Meng (6): Add global option for JSON output to ovs-appctl. python: Add option for JSON output to unixctl classes and appctl.py. appctl: Add option '--pretty' for pretty-printing JSON output. python: Add option for pretty-printing JSON output to appctl.py. vswitchd: Add JSON output for 'list-commands' command. ofproto: Add JSON output for 'dpif/show' command. Documentation/ref/ovs-appctl.8.rst | 20 +++ NEWS | 7 + lib/unixctl.c | 224 + lib/unixctl.h | 17 ++- lib/util.c | 6 +- ofproto/ofproto-dpif.c | 126 ++-- python/ovs/unixctl/__init__.py | 8 ++ python/ovs/unixctl/client.py | 5 +- python/ovs/unixctl/server.py | 56 ++-- tests/appctl.py| 43 +- tests/ofproto-dpif.at | 40 ++ tests/ovs-vswitchd.at | 33 + tests/unixctl-py.at| 13 ++ utilities/ovs-appctl.c | 157 +--- 14 files changed, 647 insertions(+), 108 deletions(-) -- 2.39.2 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
