Re: [ovs-dev] [PATCH] cirrus: Update to FreeBSD 14.1.
On 2 Jul 2024, at 20:58, Ilya Maximets wrote: > 14.1 was released on June 4 and 14.0 will reach EoL in September. > Update now. > > Signed-off-by: Ilya Maximets Change looks good to me. Acked-by: Eelco Chaudron ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ovn-ctl: Fix incorrect use of `==` operator.
Bleep bloop. Greetings Frode Nordahl, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. git-am: error: sha1 information is lacking or useless (utilities/ovn-ctl). error: could not build fake ancestor hint: Use 'git am --show-current-patch=diff' to see the failed patch Patch failed at 0001 ovn-ctl: Fix incorrect use of `==` operator. When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". Patch skipped due to previous failure. Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn] ovn-ctl: Fix incorrect use of `==` operator.
The ovn-ctl script uses a POSIX shell in its shebang line,
however commit 12412b13c9e2 added two occurrences of the `==`
operator which is bash specific.
Symptoms of the issue are failure to start up and the follwing
messages logged:
ovn-ctl: 287: test: X: unexpected operator
ovn-ctl: 307: test: X: unexpected operator
Fixes: 12412b13c9e2 ("ovn-ctl: Support for --config-file ovsdb-server option.")
Reported-at: https://launchpad.net/bugs/2071767
Signed-off-by: Frode Nordahl
---
utilities/ovn-ctl | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/utilities/ovn-ctl b/utilities/ovn-ctl
index a4f410e4f..d7ca9e758 100755
--- a/utilities/ovn-ctl
+++ b/utilities/ovn-ctl
@@ -284,7 +284,7 @@ $cluster_remote_port
set ovsdb-server
set "$@" $log --log-file=$logfile
set "$@" --pidfile=$db_pid_file
-if test X"$config_file" == X; then
+if test X"$config_file" = X; then
set "$@" --remote=punix:$sock
else
set "$@" --config-file=$config_file
@@ -304,7 +304,7 @@ $cluster_remote_port
set exec "$@"
fi
-if test X"$use_remote_in_db" != Xno && test X"$config_file" == X; then
+if test X"$use_remote_in_db" != Xno && test X"$config_file" = X; then
set "$@" --remote=db:$schema_name,$table_name,connections
fi
--
2.45.2
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] ovn-ctl: Fix incorrect use of `==` operator.
The ovn-ctl script uses a POSIX shell in its shebang line,
however commit 12412b13c9e2 added two occurrences of the `==`
operator which is bash specific.
Symptoms of the issue are failure to start up and the follwing
messages logged:
ovn-ctl: 287: test: X: unexpected operator
ovn-ctl: 307: test: X: unexpected operator
Fixes: 12412b13c9e2 ("ovn-ctl: Support for --config-file ovsdb-server option.")
Reported-at: https://launchpad.net/bugs/2071767
Signed-off-by: Frode Nordahl
---
utilities/ovn-ctl | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/utilities/ovn-ctl b/utilities/ovn-ctl
index a4f410e4f..d7ca9e758 100755
--- a/utilities/ovn-ctl
+++ b/utilities/ovn-ctl
@@ -284,7 +284,7 @@ $cluster_remote_port
set ovsdb-server
set "$@" $log --log-file=$logfile
set "$@" --pidfile=$db_pid_file
-if test X"$config_file" == X; then
+if test X"$config_file" = X; then
set "$@" --remote=punix:$sock
else
set "$@" --config-file=$config_file
@@ -304,7 +304,7 @@ $cluster_remote_port
set exec "$@"
fi
-if test X"$use_remote_in_db" != Xno && test X"$config_file" == X; then
+if test X"$use_remote_in_db" != Xno && test X"$config_file" = X; then
set "$@" --remote=db:$schema_name,$table_name,connections
fi
--
2.45.2
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v1] netdev-native-tnl: Fix inner L2 pad loss causing checksum errors.
We encountered a scenario where, if the received packet contains
padding bytes, and we then add Geneve tunnel encapsulation without
carrying the padding bytes, it results in checksum errors when sending
out. Therefore, adding an inner_l2_pad is necessary.
For example, this type of packet format:
06 6c 6a 71 e1 d3 6c e2 d3 8b ea 24 81 00 03 e9
0010 08 00 45 00 00 28 9d e5 40 00 3b 06 6e 64 0a 6f
0020 05 14 0a fe 19 06 01 bb 22 ae 59 c0 8c 61 8e 26
0030 14 e3 50 10 00 7f ce 3a 00 00 00 00 9e 38 cf 64
Fixes: 084c8087292c ("userspace: Support VXLAN and GENEVE TSO.")
Signed-off-by: Jun Wang
---
lib/dp-packet.h | 21 -
lib/netdev-native-tnl.c | 3 +++
2 files changed, 23 insertions(+), 1 deletion(-)
diff --git a/lib/dp-packet.h b/lib/dp-packet.h
index a75b1c5..d583b28 100644
--- a/lib/dp-packet.h
+++ b/lib/dp-packet.h
@@ -176,6 +176,8 @@ struct dp_packet {
ovs_be32 packet_type; /* Packet type as defined in OpenFlow */
uint16_t csum_start; /* Position to start checksumming from. */
uint16_t csum_offset; /* Offset to place checksum. */
+uint16_t inner_l2_pad_size;/* Detected inner l2 padding size.
+* Padding is non-pullable. */
union {
struct pkt_metadata md;
uint64_t data[DP_PACKET_CONTEXT_SIZE / 8];
@@ -209,7 +211,10 @@ static inline void *dp_packet_eth(const struct dp_packet
*);
static inline void dp_packet_reset_offsets(struct dp_packet *);
static inline void dp_packet_reset_offload(struct dp_packet *);
static inline uint16_t dp_packet_l2_pad_size(const struct dp_packet *);
+static inline uint16_t dp_packet_inner_l2_pad_size(const struct dp_packet *);
static inline void dp_packet_set_l2_pad_size(struct dp_packet *, uint16_t);
+static inline void dp_packet_set_inner_l2_pad_size(struct dp_packet *,
+ uint16_t);
static inline void *dp_packet_l2_5(const struct dp_packet *);
static inline void dp_packet_set_l2_5(struct dp_packet *, void *);
static inline void *dp_packet_l3(const struct dp_packet *);
@@ -435,6 +440,7 @@ static inline void
dp_packet_reset_offsets(struct dp_packet *b)
{
b->l2_pad_size = 0;
+b->inner_l2_pad_size = 0;
b->l2_5_ofs = UINT16_MAX;
b->l3_ofs = UINT16_MAX;
b->l4_ofs = UINT16_MAX;
@@ -448,6 +454,12 @@ dp_packet_l2_pad_size(const struct dp_packet *b)
return b->l2_pad_size;
}
+static inline uint16_t
+dp_packet_inner_l2_pad_size(const struct dp_packet *b)
+{
+return b->inner_l2_pad_size;
+}
+
static inline void
dp_packet_set_l2_pad_size(struct dp_packet *b, uint16_t pad_size)
{
@@ -455,6 +467,13 @@ dp_packet_set_l2_pad_size(struct dp_packet *b, uint16_t
pad_size)
b->l2_pad_size = pad_size;
}
+static inline void
+dp_packet_set_inner_l2_pad_size(struct dp_packet *b, uint16_t pad_size)
+{
+ovs_assert(pad_size <= dp_packet_size(b));
+b->inner_l2_pad_size = pad_size;
+}
+
static inline void *
dp_packet_l2_5(const struct dp_packet *b)
{
@@ -543,7 +562,7 @@ dp_packet_inner_l4_size(const struct dp_packet *b)
return OVS_LIKELY(b->inner_l4_ofs != UINT16_MAX)
? (const char *) dp_packet_tail(b)
- (const char *) dp_packet_inner_l4(b)
- - dp_packet_l2_pad_size(b)
+ - dp_packet_inner_l2_pad_size(b)
: 0;
}
diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
index 0f9f07f..96ffdc1 100644
--- a/lib/netdev-native-tnl.c
+++ b/lib/netdev-native-tnl.c
@@ -156,6 +156,7 @@ netdev_tnl_push_ip_header(struct dp_packet *packet, const
void *header,
struct eth_header *eth;
struct ip_header *ip;
struct ovs_16aligned_ip6_hdr *ip6;
+uint16_t l2_pad_size;
eth = dp_packet_push_uninit(packet, size);
*ip_tot_size = dp_packet_size(packet) - sizeof (struct eth_header);
@@ -163,7 +164,9 @@ netdev_tnl_push_ip_header(struct dp_packet *packet, const
void *header,
memcpy(eth, header, size);
/* The encapsulated packet has type Ethernet. Adjust dp_packet. */
packet->packet_type = htonl(PT_ETH);
+l2_pad_size = dp_packet_l2_pad_size(packet);
dp_packet_reset_offsets(packet);
+dp_packet_set_inner_l2_pad_size(packet, l2_pad_size);
packet->l3_ofs = sizeof (struct eth_header);
if (netdev_tnl_is_header_ipv6(header)) {
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] Documentation: Update language about soft freeze requirements.
On Tue, Jul 2, 2024 at 1:51 PM Ilya Maximets wrote: > > On 7/2/24 19:38, Mark Michelson wrote: > > The documentation stated that for soft freeze, the development team > > would only consider merging new feature patches that had been "discussed > > and reviewed" during the soft freeze period. In reality, the OVN team > > has been willing to merge any new feature as long as it has been posted > > for review prior to the soft freeze date. The emails sent out by the OVN > > team prior to soft freeze have also stated that patches that were posted > > for review were under consideration for merging. > > > > To clear up confusion, the wording in the documentation has been updated > > as well now to indicate that patches simply need to be "posted" rather > > than being "discussed and reviewed" prior to the soft freeze date. > > > > Signed-off-by: Mark Michelson Acked-by: Numan Siddique Numan > > --- > > Documentation/internals/release-process.rst | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/Documentation/internals/release-process.rst > > b/Documentation/internals/release-process.rst > > index 988257975..2e9fa4f18 100644 > > --- a/Documentation/internals/release-process.rst > > +++ b/Documentation/internals/release-process.rst > > @@ -45,9 +45,9 @@ Scheduling`_ for the timing of each stage: > > 1. "Soft freeze" of the main branch. > > > > During the freeze, we ask committers to refrain from applying patches > > that > > - add new features unless those patches were already being publicly > > discussed > > - and reviewed before the freeze began. Bug fixes are welcome at any > > time. > > - Please propose and discuss exceptions on ovs-dev. > > + add new features unless those patches were already publicly posted > > before > > 'publicly posted' sounds a little strange... Maybe 'posted for review' or > 'posted for a public review'? > > > + the freeze began. Bug fixes are welcome at any time. Please propose > > Two spaces vs one space. We should be consistent at least within the same > paragraph. > > > + and discuss exceptions on ovs-dev. > > > > 2. Fork a release branch from main, named for the expected release number, > > e.g. "branch-25.09" for the branch that will yield OVN 25.09.x. > > ___ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ovs-lib: Recreate db when schema is corrupted.
On 7/2/24 21:27, Mike Pattrick wrote: > Previously ovs-lib would assume a database is valid if the file exists, > however, it is possible for the database file to exist but be unopenable > by ovsdb. > > Now existence checks are augmented with schema checksum validation. > Databases with an invalid schema are removed. Hi, Mike. The checksum is optional in the schema. Also, the db-cksum method only works on standalone databases. And clustered databases that didn't join the cluster yet have no schema at all. The test case in the patch seems insufficient. And having incorrect checksum in the schema is not a fatal failure, most of the schemas we're testing with have incorrect checksums and that's OK as long as the database record itself is correct. So, I don't think this validation method is a good solution. Also, we need to at least back up any non-empty files. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] ovs-lib: Recreate db when schema is corrupted.
Previously ovs-lib would assume a database is valid if the file exists,
however, it is possible for the database file to exist but be unopenable
by ovsdb.
Now existence checks are augmented with schema checksum validation.
Databases with an invalid schema are removed.
Reported-at: https://issues.redhat.com/browse/FDP-689
Reported-by: Ihar Hrachyshka
Signed-off-by: Mike Pattrick
---
tests/ovsdb-server.at | 50 +++
utilities/ovs-lib.in | 23
2 files changed, 69 insertions(+), 4 deletions(-)
diff --git a/tests/ovsdb-server.at b/tests/ovsdb-server.at
index ce6d32aee..54c1cda97 100644
--- a/tests/ovsdb-server.at
+++ b/tests/ovsdb-server.at
@@ -105,6 +105,56 @@ AT_CHECK([uuidfilt output], [0],
]], [])
AT_CLEANUP
+AT_SETUP([truncated database recreated])
+AT_KEYWORDS([ovsdb unix])
+AT_SKIP_IF([test "$IS_WIN32" = "yes"])
+ordinal_schema > schema
+. ovs-lib
+
+dnl Check that DB is recreated when schema corrupted.
+echo 'x' > db
+AT_CHECK([upgrade_db db schema], [0], [stdout], [ignore])
+AT_CHECK([grep -c "db does not exist\|Creating empty database db" stdout],
[0], [2
+])
+
+AT_DATA([txnfile], [[ovsdb-client transact unix:socket \
+'["ordinals",
+ {"op": "insert",
+ "table": "ordinals",
+ "row": {"number": 1, "name": "one"}}]'
+]])
+AT_CHECK([ovsdb-server --remote=punix:socket db --run="sh txnfile"], [0],
[stdout], [stderr])
+
+dnl Check that DB is not recreated on corrupted log. This is similar to the
previous test but
+dnl includes a mid-operation upgrade.
+echo 'xxx' >> db
+AT_CHECK([upgrade_db db schema], [0], [], [ignore])
+
+dnl Validate that the db can now be used.
+AT_DATA([txnfile], [[ovsdb-client transact unix:socket \
+'["ordinals",
+ {"op": "select",
+ "table": "ordinals",
+ "where": []}]'
+]])
+AT_CHECK([ovsdb-server --remote=punix:socket db --run="sh txnfile"], [0],
[stdout], [stderr])
+AT_CHECK([grep 'syntax error: db: parse error.* in header line "xxx"' stderr],
[0], [ignore])
+cat stdout >> output
+AT_CHECK([uuidfilt output], [0],
+
[[[{"rows":[{"_uuid":["uuid","<0>"],"_version":["uuid","<1>"],"name":"one","number":1}]}]
+]], [])
+
+dnl Validate then create and join cluster.
+echo 'x' > db
+AT_CHECK([create_cluster db schema tcp:1.1.1.1: 1000], [0], [stdout], [])
+AT_CHECK([grep -c 'Creating cluster database db' stdout], [0], [1
+])
+echo 'x' > db
+AT_CHECK([join_cluster db schema tcp:1.1.1.1: tcp:2.2.2.2:], [0],
[stdout], [])
+AT_CHECK([grep -c 'Joining db to cluster' stdout], [0], [1
+])
+AT_CLEANUP
+
AT_SETUP([truncating database log with bad transaction])
AT_KEYWORDS([ovsdb server positive unix])
AT_SKIP_IF([test "$IS_WIN32" = "yes"])
diff --git a/utilities/ovs-lib.in b/utilities/ovs-lib.in
index 7812a94ee..7e580d730 100644
--- a/utilities/ovs-lib.in
+++ b/utilities/ovs-lib.in
@@ -447,12 +447,27 @@ backup_db () {
action "Backing up database to $backup" cp "$DB_FILE" "$backup" || return 1
}
+validate_db () {
+# Returns 0 if $DB_FILE is present and has a valid schema.
+# Returns 1 if $DB_FILE is not present.
+DB_FILE="$1"
+
+if test ! -e "$DB_FILE"; then
+return 1
+elif ! ovsdb_tool db-cksum "$DB_FILE" >/dev/null 2>&1; then
+rm -f "$DB_FILE"
+return 1
+fi
+
+return 0
+}
+
upgrade_db () {
DB_FILE="$1"
DB_SCHEMA="$2"
schemaver=`ovsdb_tool schema-version "$DB_SCHEMA"`
-if test ! -e "$DB_FILE"; then
+if ! validate_db "$DB_FILE"; then
log_warning_msg "$DB_FILE does not exist"
install_dir `dirname $DB_FILE`
create_db "$DB_FILE" "$DB_SCHEMA"
@@ -513,7 +528,7 @@ create_cluster () {
election_timer_arg="--election-timer=$ELECTION_TIMER_MS"
fi
-if test ! -e "$DB_FILE"; then
+if ! validate_db "$DB_FILE"; then
action "Creating cluster database $DB_FILE" ovsdb_tool
$election_timer_arg create-cluster "$DB_FILE" "$DB_SCHEMA" "$LOCAL_ADDR"
elif ovsdb_tool db-is-standalone "$DB_FILE"; then
# Convert standalone database to clustered.
@@ -530,11 +545,11 @@ join_cluster() {
LOCAL_ADDR="$3"
REMOTE_ADDR="$4"
-if test -e "$DB_FILE" && ovsdb_tool db-is-standalone "$DB_FILE"; then
+if validate_db "$DB_FILE" && ovsdb_tool db-is-standalone "$DB_FILE"; then
backup_db || return 1
rm $DB_FILE
fi
-if test ! -e "$DB_FILE"; then
+if ! validate_db "$DB_FILE"; then
action "Joining $DB_FILE to cluster" \
ovsdb_tool join-cluster "$DB_FILE" "$SCHEMA_NAME" "$LOCAL_ADDR"
"$REMOTE_ADDR"
fi
--
2.39.3
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] cirrus: Update to FreeBSD 14.1.
14.1 was released on June 4 and 14.0 will reach EoL in September. Update now. Signed-off-by: Ilya Maximets --- .cirrus.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.cirrus.yml b/.cirrus.yml index 8db385f00..d73154a97 100644 --- a/.cirrus.yml +++ b/.cirrus.yml @@ -3,7 +3,7 @@ freebsd_build_task: freebsd_instance: matrix: image_family: freebsd-13-3-snap - image_family: freebsd-14-0-snap + image_family: freebsd-14-1-snap cpu: 4 memory: 4G -- 2.45.2 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next v7 05/10] net: openvswitch: add psample action
On 7/2/24 20:24, Jakub Kicinski wrote: > On Tue, 2 Jul 2024 20:16:51 +0200 Ilya Maximets wrote: >> On 7/2/24 20:06, Jakub Kicinski wrote: >>> On Tue, 2 Jul 2024 11:53:01 +0200 Ilya Maximets wrote: Or create a simple static function and mark all the arguments as unused, which kind of compliant to the coding style, but the least pretty. >>> >>> To be clear - kernel explicitly disables warnings about unused >>> arguments. Unused arguments are not a concern. >> >> OK. Good to know. >> >> Though I think, the '12) Macros, Enums and RTL' section of the >> coding style document needs some rephrasing in that case. > > Do you mean something like: > > diff --git a/Documentation/process/coding-style.rst > b/Documentation/process/coding-style.rst > index 7e768c65aa92..0516b7009688 100644 > --- a/Documentation/process/coding-style.rst > +++ b/Documentation/process/coding-style.rst > @@ -828,7 +828,7 @@ Generally, inline functions are preferable to macros > resembling functions. > } while (0) > > Function-like macros with unused parameters should be replaced by static > -inline functions to avoid the issue of unused variables: > +inline functions to avoid the issue of unused variables in the caller: > > .. code-block:: c > > ? Yes, that makes the logic behind the sentence way more clear. At least for me. Without this change it's hard to tell if the doc is talking about unused function arguments or unused variables in the caller. There is another issue though that this particular phrase directly leads a developer to declaring 'static inline' functions in .c files. And even '15) The inline disease' cites this use case as appropriate. And it is, with the exception for a macro that is a no-op stub version of some function. Having an example on how to write a stub version of the function in both .h and .c files might be a good addition to '21) Conditional Compilation' section breaking the tie for this particular use case. This section also discourages from conditional compilation in .c files, but it doesn't seem reasonable to export a static function outside for that reason. I suppose that section is mostly concerned about use of other modules. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next v7 05/10] net: openvswitch: add psample action
On Tue, Jul 02, 2024 at 01:33:01PM GMT, Simon Horman wrote:
> On Tue, Jul 02, 2024 at 11:53:01AM +0200, Ilya Maximets wrote:
> > On 7/2/24 11:37, Simon Horman wrote:
> > > On Tue, Jul 02, 2024 at 03:05:02AM -0400, Adrián Moreno wrote:
> > >> On Mon, Jul 01, 2024 at 02:23:12PM GMT, Aaron Conole wrote:
> > >>> Adrian Moreno writes:
> > >
> > > ...
> > >
> > diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> > >
> > > ...
> > >
> > @@ -1299,6 +1304,39 @@ static int execute_dec_ttl(struct sk_buff *skb,
> > struct sw_flow_key *key)
> > return 0;
> > }
> >
> > +#if IS_ENABLED(CONFIG_PSAMPLE)
> > +static void execute_psample(struct datapath *dp, struct sk_buff *skb,
> > + const struct nlattr *attr)
> > +{
> > + struct psample_group psample_group = {};
> > + struct psample_metadata md = {};
> > + const struct nlattr *a;
> > + int rem;
> > +
> > + nla_for_each_attr(a, nla_data(attr), nla_len(attr), rem) {
> > + switch (nla_type(a)) {
> > + case OVS_PSAMPLE_ATTR_GROUP:
> > + psample_group.group_num = nla_get_u32(a);
> > + break;
> > +
> > + case OVS_PSAMPLE_ATTR_COOKIE:
> > + md.user_cookie = nla_data(a);
> > + md.user_cookie_len = nla_len(a);
> > + break;
> > + }
> > + }
> > +
> > + psample_group.net = ovs_dp_get_net(dp);
> > + md.in_ifindex = OVS_CB(skb)->input_vport->dev->ifindex;
> > + md.trunc_size = skb->len - OVS_CB(skb)->cutlen;
> > +
> > + psample_sample_packet(&psample_group, skb, 0, &md);
> > +}
> > +#else
> > +static inline void execute_psample(struct datapath *dp, struct
> > sk_buff *skb,
> > + const struct nlattr *attr) {}
> > >>>
> > >>> I noticed that this got flagged in patchwork since it is 'static inline'
> > >>> while being part of a complete translation unit - but I also see some
> > >>> other places where that has been done. I guess it should be just
> > >>> 'static' though. I don't feel very strongly about it.
> > >>>
> > >>
> > >> We had a bit of discussion about this with Ilya. It seems "static
> > >> inline" is a common pattern around the kernel. The coding style
> > >> documentation says:
> > >> "Generally, inline functions are preferable to macros resembling
> > >> functions."
> > >>
> > >> So I think this "inline" is correct but I might be missing something.
> > >
> > > Hi Adrián,
> > >
> > > TL;DR: Please remove this inline keyword
> > >
> > > For Kernel networking code at least it is strongly preferred not
> > > to use inline in .c files unless there is a demonstrable - usually
> > > performance - reason to do so. Rather, it is preferred to let the
> > > compiler decide when to inline such functions. OTOH, the inline
> > > keyword in .h files is fine.
> >
> > FWIW, the main reason for 'inline' here is not performance, but silencing
> > compiler's potential 'maybe unused' warnings:
> >
> > Function-like macros with unused parameters should be replaced by static
> > inline functions to avoid the issue of unused variables
> >
> > I think, the rule for static inline functions in .c files is at odds with
> > the 'Conditional Compilation' section of coding style. The section does
> > recommend to avoid conditional function declaration in .c files, but I'm not
> > sure it is reasonable to export internal static functions for that reason.
> >
> > In this particular case we can either define a macro, which is discouraged
> > by the coding style:
> >
> > Generally, inline functions are preferable to macros resembling functions.
> >
> > Or create a static inline function, that is against rule of no static
> > inline functions in .c files.
> >
> > Or create a simple static function and mark all the arguments as unused,
> > which kind of compliant to the coding style, but the least pretty.
>
> Hi Ilya,
>
> I guess I would lean towards the last option.
> But in any case, thanks for pointing out that this is complex:
> I had not realised that when I wrote my previous email.
>
In that case this version (v7) should be good to go? Are there any other
comments? Or is v8 preferred (the only change is between them is
dropping the "inline")?
Thanks.
Adrián
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next v7 05/10] net: openvswitch: add psample action
On Tue, 2 Jul 2024 20:16:51 +0200 Ilya Maximets wrote: > On 7/2/24 20:06, Jakub Kicinski wrote: > > On Tue, 2 Jul 2024 11:53:01 +0200 Ilya Maximets wrote: > >> Or create a simple static function and mark all the arguments as unused, > >> which kind of compliant to the coding style, but the least pretty. > > > > To be clear - kernel explicitly disables warnings about unused > > arguments. Unused arguments are not a concern. > > OK. Good to know. > > Though I think, the '12) Macros, Enums and RTL' section of the > coding style document needs some rephrasing in that case. Do you mean something like: diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst index 7e768c65aa92..0516b7009688 100644 --- a/Documentation/process/coding-style.rst +++ b/Documentation/process/coding-style.rst @@ -828,7 +828,7 @@ Generally, inline functions are preferable to macros resembling functions. } while (0) Function-like macros with unused parameters should be replaced by static -inline functions to avoid the issue of unused variables: +inline functions to avoid the issue of unused variables in the caller: .. code-block:: c ? ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next v7 05/10] net: openvswitch: add psample action
On 7/2/24 20:06, Jakub Kicinski wrote: > On Tue, 2 Jul 2024 11:53:01 +0200 Ilya Maximets wrote: >> Or create a simple static function and mark all the arguments as unused, >> which kind of compliant to the coding style, but the least pretty. > > To be clear - kernel explicitly disables warnings about unused > arguments. Unused arguments are not a concern. OK. Good to know. Though I think, the '12) Macros, Enums and RTL' section of the coding style document needs some rephrasing in that case. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next v7 05/10] net: openvswitch: add psample action
On Tue, 2 Jul 2024 11:53:01 +0200 Ilya Maximets wrote: > Or create a simple static function and mark all the arguments as unused, > which kind of compliant to the coding style, but the least pretty. To be clear - kernel explicitly disables warnings about unused arguments. Unused arguments are not a concern. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3] python: ovsdb-idl: Add custom transaction operations.
On 6/28/24 21:18, Terry Wilson wrote: > It can be useful to be able to send raw transaction operations > through the Idl's connection. For example, to clean up MAC_Binding > entries for floating IPs without having to monitor the MAC_Binding > table which can be quite large. > > Signed-off-by: Terry Wilson > --- > NEWS | 2 ++ > python/ovs/db/idl.py | 21 - > tests/ovsdb-idl.at | 27 > tests/test-ovsdb.py | 73 > 4 files changed, 122 insertions(+), 1 deletion(-) Thanks, Terry! Applied. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] Documentation: Update language about soft freeze requirements.
On 7/2/24 19:38, Mark Michelson wrote: > The documentation stated that for soft freeze, the development team > would only consider merging new feature patches that had been "discussed > and reviewed" during the soft freeze period. In reality, the OVN team > has been willing to merge any new feature as long as it has been posted > for review prior to the soft freeze date. The emails sent out by the OVN > team prior to soft freeze have also stated that patches that were posted > for review were under consideration for merging. > > To clear up confusion, the wording in the documentation has been updated > as well now to indicate that patches simply need to be "posted" rather > than being "discussed and reviewed" prior to the soft freeze date. > > Signed-off-by: Mark Michelson > --- > Documentation/internals/release-process.rst | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/Documentation/internals/release-process.rst > b/Documentation/internals/release-process.rst > index 988257975..2e9fa4f18 100644 > --- a/Documentation/internals/release-process.rst > +++ b/Documentation/internals/release-process.rst > @@ -45,9 +45,9 @@ Scheduling`_ for the timing of each stage: > 1. "Soft freeze" of the main branch. > > During the freeze, we ask committers to refrain from applying patches that > - add new features unless those patches were already being publicly > discussed > - and reviewed before the freeze began. Bug fixes are welcome at any time. > - Please propose and discuss exceptions on ovs-dev. > + add new features unless those patches were already publicly posted before 'publicly posted' sounds a little strange... Maybe 'posted for review' or 'posted for a public review'? > + the freeze began. Bug fixes are welcome at any time. Please propose Two spaces vs one space. We should be consistent at least within the same paragraph. > + and discuss exceptions on ovs-dev. > > 2. Fork a release branch from main, named for the expected release number, > e.g. "branch-25.09" for the branch that will yield OVN 25.09.x. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3] netdev-offload-dpdk: Support offload of set dscp action.
On 7/1/24 13:52, Sunyang Wu wrote:
> Hi, Ilya, Thank you for your valuable advice!
> In lines 7390-7406 of the 'ofproto_dpif xlate.c' file, show below:
> case OFPACT_SET_IP_DSCP:
> if (is_ip_any(flow)) {
> WC_MASK_FIELD(wc, nw_proto);
> wc->masks.nw_tos |= IP_DSCP_MASK;
> flow->nw_tos &= ~IP_DSCP_MASK;
> flow->nw_tos |= ofpact_get_SET_IP_DSCP(a)->dscp;
> }
> break;
>
> case OFPACT_SET_IP_ECN:
> if (is_ip_any(flow)) {
> WC_MASK_FIELD(wc, nw_proto);
> wc->masks.nw_tos |= IP_ECN_MASK;
> flow->nw_tos &= ~IP_ECN_MASK;
> flow->nw_tos |= ofpact_get_SET_IP_ECN(a)->ecn;
> }
> break;
> when parsing and setting the DSCP and ECN actions, different masks are applied
> to the `nw_tos` field respectively, and a mask operation is performed on
> `nw_tos`.
True.
> Additionally, the same action identifier is used for offloading both DSCP
> and ECN, ensuring that setting DSCP and setting ECN do not interfere with
> each other.
I'm not sure I understand what you mean here.
Consider the following sequence of events:
1. OFPACT_SET_IP_ECN is executed.
wc->masks.nw_tos |= IP_ECN_MASK;
flow->nw_tos |= ofpact_get_SET_IP_ECN(a)->ecn;
2. flow and wc are translated into ovs_key_ipv4 elements in key and mask.
So, key->ipv4_tos == flow->nw_tos and mask->ipv4_tos == IP_ECN_MASK.
3. We call add_set_flow_action(ipv4_tos, RTE_FLOW_ACTION_TYPE_SET_IPV4_DSCP)
We check if mask->ipv4_tos is all-zero -- it is not.
We check if it is all-ones -- it is not.
We check if the action is RTE_FLOW_ACTION_TYPE_SET_IPV4_DSCP -- yes!
4. add_flow_action() is called with value flow->nw_tos, which is our
original ofpact_get_SET_IP_ECN(a)->ecn. Since ECN bits are higher
than DSCP, that is equivalent ot setting DSCP to zero.
5. mask->ipv4_tos is set to all zeroes.
6. Offload suceeds.
Original action was to set ECN to some value, what we offloaded is
seting DSCP to zero, which is a completely different action.
Offloading should have failed instead.
>
> Regarding the `add_set_flow_action__` function, the mask is cleared to zero.
> This clearing operation is intended for subsequent checks to determine if all
> the
> fields issued are supported. This mask will not be passed to the DPDK side,
> so it
> will not affect the offloading operation.
It will, because we should have failed offloading, since we can't
offload setting of ECN bits.
>
> On 6/28/24 23:08, Ilya Maximets wrote:
>> On 6/20/24 09:35, Sunyang Wu via dev wrote:
>>> Add the "set dscp action" parsing function, so that the "set dscp
>>> action" can be offloaded.
>>>
>>> Signed-off-by: Sunyang Wu
>>> ---
>>> lib/netdev-offload-dpdk.c | 19 ++-
>>> 1 file changed, 18 insertions(+), 1 deletion(-)
>>>
>>
>> Hi, Sunyang Wu. Thanks for the patch!
>> See some comments inline.
>>
>> Best regards, Ilya Maximets.
>>
>>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
>>> index 623005b1c..524942457 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;
>>> @@ -1835,7 +1846,10 @@ add_set_flow_action__(struct flow_actions *actions,
>>> if (is_all_zeros(mask, size)) {
>>> return 0;
>>> }
>>> -if (!is_all_ones(mask, size)) {
>>> +if (!is_all_ones(mask, size) &&
>>> +/* set dscp need patial mask */
>>> +attr != RTE_FLOW_ACTION_TYPE_SET_IPV4_DSCP &&
>>> +attr != RTE_FLOW_ACTION_TYPE_SET_IPV6_DSCP) {
>>
>> This doesn't seem right. We need to check that the mask contains all
>> the DSCP bits and that it doesn't have anything else inside.
>>
>> For example, if we try to set ECN bits, we'll have them in a mask.
>> In this case is_all_zeros() check will fail and then this check will
>> allow us to proceed as well. In the end, we'll end up setting DSCP
>> bits to all zeroes, or worse, to whatever was in the first six bits of
>> nw_tos.
>>
>
> Also, this function
[ovs-dev] [PATCH ovn] Documentation: Update language about soft freeze requirements.
The documentation stated that for soft freeze, the development team would only consider merging new feature patches that had been "discussed and reviewed" during the soft freeze period. In reality, the OVN team has been willing to merge any new feature as long as it has been posted for review prior to the soft freeze date. The emails sent out by the OVN team prior to soft freeze have also stated that patches that were posted for review were under consideration for merging. To clear up confusion, the wording in the documentation has been updated as well now to indicate that patches simply need to be "posted" rather than being "discussed and reviewed" prior to the soft freeze date. Signed-off-by: Mark Michelson --- Documentation/internals/release-process.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Documentation/internals/release-process.rst b/Documentation/internals/release-process.rst index 988257975..2e9fa4f18 100644 --- a/Documentation/internals/release-process.rst +++ b/Documentation/internals/release-process.rst @@ -45,9 +45,9 @@ Scheduling`_ for the timing of each stage: 1. "Soft freeze" of the main branch. During the freeze, we ask committers to refrain from applying patches that - add new features unless those patches were already being publicly discussed - and reviewed before the freeze began. Bug fixes are welcome at any time. - Please propose and discuss exceptions on ovs-dev. + add new features unless those patches were already publicly posted before + the freeze began. Bug fixes are welcome at any time. Please propose + and discuss exceptions on ovs-dev. 2. Fork a release branch from main, named for the expected release number, e.g. "branch-25.09" for the branch that will yield OVN 25.09.x. -- 2.45.2 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v29 0/8] Add offload support for sFlow
On 7/2/24 08:04, Chris Mi wrote: > On 6/27/2024 1:24 PM, Chris Mi wrote: >> On 6/18/2024 2:27 PM, Chris Mi wrote: >>> On 5/16/2024 5:09 AM, Ilya Maximets wrote: On 3/26/24 03:30, Chris Mi wrote: > This patch set adds offload support for sFlow. > > Psample is a genetlink channel for packet sampling. TC action > act_sample > uses psample to send sampled packets to userspace. > > When offloading sample action to TC, userspace creates a unique ID to > map sFlow action and tunnel info and passes this ID to kernel instead > of the sFlow info. psample will send this ID and sampled packet to > userspace. Using the ID, userspace can recover the sFlow info and send > sampled packet to the right sFlow monitoring host. Hi, Chris, others. I've been looking through the set and a psample in general for the past few days. There are few fairly minor issues in the set like UBsan crash because of incorrect OVS_RETURNS_NONNULL annotation and a few style and formatting issues, which are not hard to fix. However, I encountered an issue with a way tunnel metadata is recovered that I'm not sure we can actually fix. The algorithm of work in this patch set is described in the cover letter above. The key point is then packet-1 goes though MISS upcall we install a new datapath flow into TC and we remember the tunnel metadata of the packet-1 associated with a sample ID. When the packet-2 hits the datapath flow (tc actions), it gets sent to psample and ovs-vswitchd reads this packet from psample netlink socket and presents it as an ACTION upcall. Doing that it finds tunnel metadata using the sample ID and uses it as a tunnel metadata for packet-2. The key of the problem is that this is metadata from packet-1 and it can be different from metadata of packet-2. An example of this will be having two separate TCP streams going through the same VxLAN tunnel. For load balancing reasons most UDP tunnels choose source port of the outer UDP header based on RSS or 5-tuple hash of the inner packet. This means that two TCP streams going through the same tunnel will have different UDP source port in the outer header. When a packet from a first stream hits a MISS upcall, datapath flow will be installed and the sample ID will be associated with the metadata of the first stream. Chances are this datapath flow will not have exact match on the source port of the tunnel metadata. That means that a packet from the second stream will successfully match on the installed datapath flow and will go to psample with the ID of the first stream. OVS will read it from the psample socket and send to sFlow collector with metadata it found by the ID, i.e. with a tunnel metadata that contains tunnel source port of the first TCP stream, which is incorrect. The test below reproduces the issue. It's not a real test, it always fails on purpose and not very pretty, but what it does is that it creates a tunnel, then starts netcat server on one side and sends two files to it from the other side. These two transfers are separate TCP connections, so they use different source ports, that is causing the tunnel to choose different UDP source ports for them. After the test failure we can see in the sflow.log that all the values for 'tunnel4_in_src_port' are the same for both streams and some exchanges prior to that. However, if offload is disabled, the values will be different as they should. So, unless we can ensure that packets from different streams do not match on the same datapath flow, this schema doesn't work. The only way to ensure that packets from different streams within the same tunnel do not match on the same datapath flow is to always have an exact match on the whole tunnel metadata. But I don't think that is a good idea, because this way we'll have a separate datapath flow per TCP stream, which will be very bad for performance. And it will be bad for hardware offload since hardware NICs normally have a more limited amount of available resources. This leads us to conclusion that only non-tunneling traffic can be offloaded this way. For this to work we'll have to add an explicit match on tunnel metadata being empty (can that be expressed in TC?) But at this point a question arises if this even worth implementing, taking into account that it requires a lot of infrastructure changes throughout all the layers of OVS code just for sampling of non-tunnel packets, i.e. a very narrow use case. Also, my understanding was that there is a plan to offload other types of userspace()
[ovs-dev] [PATCH] bond: Fix inaccurate log info in bond_shift_load.
When the delta is less than 1024 in bond_shift_load, it print "shift 0kB of
load".
Like this:
bond dpdkbond0: shift 0kB of load (with hash 71) from nic1 to nic2 (now
carrying 20650165kB and 8311662kB load, respectively)
Signed-off-by: Han Ding
---
ofproto/bond.c | 24 +---
1 file changed, 17 insertions(+), 7 deletions(-)
diff --git a/ofproto/bond.c b/ofproto/bond.c
index c31869a..5b1975d 100644
--- a/ofproto/bond.c
+++ b/ofproto/bond.c
@@ -1192,13 +1192,23 @@ bond_shift_load(struct bond_entry *hash, struct
bond_member *to)
struct bond *bond = from->bond;
uint64_t delta = hash->tx_bytes;
-VLOG_INFO("bond %s: shift %"PRIu64"kB of load (with hash %"PRIdPTR") "
- "from %s to %s (now carrying %"PRIu64"kB and "
- "%"PRIu64"kB load, respectively)",
- bond->name, delta / 1024, hash - bond->hash,
- from->name, to->name,
- (from->tx_bytes - delta) / 1024,
- (to->tx_bytes + delta) / 1024);
+if (delta >= 1024) {
+VLOG_INFO("bond %s: shift %"PRIu64"kB of load (with hash %"PRIdPTR") "
+"from %s to %s (now carrying %"PRIu64"kB and "
+"%"PRIu64"kB load, respectively)",
+bond->name, delta / 1024, hash - bond->hash,
+from->name, to->name,
+(from->tx_bytes - delta) / 1024,
+(to->tx_bytes + delta) / 1024);
+} else {
+VLOG_INFO("bond %s: shift %"PRIu64"B of load (with hash %"PRIdPTR") "
+"from %s to %s (now carrying %"PRIu64"kB and "
+"%"PRIu64"kB load, respectively)",
+bond->name, delta, hash - bond->hash,
+from->name, to->name,
+(from->tx_bytes - delta) / 1024,
+(to->tx_bytes + delta) / 1024);
+}
/* Shift load away from 'from' to 'to'. */
from->tx_bytes -= delta;
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] netdev-tc-offloads: Don't offload header modification on ip fragments.
On 6/27/24 15:03, Eelco Chaudron wrote:
>
>
> On 4 Jun 2024, at 13:52, Ilya Maximets wrote:
>
>> On 6/4/24 13:42, Eelco Chaudron wrote:
>>>
>>>
>>> On 1 Jun 2024, at 0:08, Ilya Maximets wrote:
>>>
On 5/7/24 15:52, Eelco Chaudron wrote:
> While offloading header modifications to TC, OVS is using {TCA_PEDIT} +
> {TCA_CSUM} combination as that it the only way to represent header
> rewrite. However, {TCA_CSUM} is unable to calculate L4 checksums for
> IP fragments.
>
> Since TC already applies fragmentation bit masking, this patch simply
> needs to prevent these packets from being processed through TC.
>
> Signed-off-by: Eelco Chaudron
> ---
> v2: - Fixed and added some comments.
> - Use ovs-pcap to compare packets.
>
> NOTE: This patch needs an AVX512 fix before it can be applied.
> Intel is working on this.
> ---
> lib/netdev-offload-tc.c | 32 ++
> lib/tc.c| 5 ++-
> tests/system-traffic.at | 93 +
> 3 files changed, 129 insertions(+), 1 deletion(-)
>
> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> index 921d52317..bdd307933 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
> @@ -1488,6 +1488,30 @@ parse_put_flow_ct_action(struct tc_flower *flower,
> return 0;
> }
>
> +static bool
> +will_tc_add_l4_checksum(struct tc_flower *flower, int type)
> +{
> +/* This function returns true if the tc layer will add a l4 checksum
> action
> + * for this set action. Refer to the csum_update_flag() function for
> + * detailed logic. Note that even the kernel only supports updating
> TCP,
> + * UDP and ICMPv6. */
This comment should be outside of the function, I think. It's strange
to have it here. I see csum_update_flag() has a comment inside, but
that's strange as well. That function has other style issues as well,
there is no need to copy them.
>>>
>>> ACK, will clean this up in the next rev.
>>>
> +switch (type) {
> +case OVS_KEY_ATTR_IPV4:
> +case OVS_KEY_ATTR_IPV6:
> +case OVS_KEY_ATTR_TCP:
> +case OVS_KEY_ATTR_UDP:
> +switch (flower->key.ip_proto) {
> +case IPPROTO_TCP:
> +case IPPROTO_UDP:
> +case IPPROTO_ICMPV6:
> +case IPPROTO_UDPLITE:
> +return true;
> +}
> +break;
> +}
> +return false;
> +}
> +
> static int
> parse_put_flow_set_masked_action(struct tc_flower *flower,
> struct tc_action *action,
> @@ -1520,6 +1544,14 @@ parse_put_flow_set_masked_action(struct tc_flower
> *flower,
> return EOPNOTSUPP;
> }
>
> +if (flower->key.flags & TCA_FLOWER_KEY_FLAGS_IS_FRAGMENT
We're using this field to make an offloading decision. We must
also set in the mask. If for some reason we're not matching on
the fragment bits, we may first receive a non-fragmented packet
and offload it, then fragmented traffic may match and fail the
checksumming. So, we need to enable the bit in the mask.
>>>
>>> The dp always matches on the fragment bit for IPv4 packets, I did some
>>> tests with this.
>>> So if we sent a non-fragment packet first the dp rule will match on
>>> fragment = 0. Or
>>> did I miss something?
>>
>> It is true today, but nothing ensures that on the netdev-offload-tc level.
>> Moreover, the netdev-offload-tc is written in a way that it expects the
>> frag bits to potentially not be in the mask:
>>
>> https://github.com/openvswitch/ovs/blob/1d681ffe3b208a0db4945b6389142ab18404a4d1/lib/netdev-offload-tc.c#L2432-L2448
>> So, it is going to be internally inconsistent if we do not set the bit
>> explicitly.
>>
>> And if someday we'll stop adding the frag bit, we'll never know if we
>> forget to set it in netdev-offload-tc. At the very least we'll need an
>> assertion that it is set. But having an assertion will still be internally
>> inconsistent with the code linked above. So, it may be better to just fix
>> it instead anyway.
>
> I finally got some time to look at the code, and I can add the below abort()
> in case OVS decides to no longer mask out the frag bits by default.
>
> However, I have no clear idea how to make this work if we ever stop adding
> this frag mask. I don't think we can report back to request a wider mask or
> track what exists and force a revalidate.
>
> @@ -2447,6 +2479,8 @@ netdev_tc_flow_put(struct netdev *netdev, struct match
> *match,
> }
>
> mask->nw_frag = 0;
> +} else {
> +ovs_abort(0, "TC offload assumes nw_frag is always masked");
> }
We don't need to abort. We should just narrow down the match by adding
frag b
[ovs-dev] [PATCH net-next 3/3] selftests: openvswitch: Be more verbose with selftest debugging.
The openvswitch selftest is difficult to debug for anyone that isn't
directly familiar with the openvswitch module and the specifics of the
test cases. Many times when something fails, the debug log will be
sparsely populated and it takes some time to understand where a failure
occured.
Increase the amount of details logged to the debug log by trapping all
'info' logs, and all 'ovs_sbx' commands.
Signed-off-by: Aaron Conole
---
NOTE: There is a conflict here with a patch on list that adds psample
support, but it should be simple to resolve, since the conflict
would be due to a context change in tests="". I can also respin
if the patches collide.
tools/testing/selftests/net/openvswitch/openvswitch.sh | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh
b/tools/testing/selftests/net/openvswitch/openvswitch.sh
index 0bd0425848d9..531951086d9c 100755
--- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
+++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
@@ -23,7 +23,9 @@ tests="
drop_reason drop: test drop reasons are
emitted"
info() {
-[ $VERBOSE = 0 ] || echo $*
+ [ "${ovs_dir}" != "" ] &&
+ echo "`date +"[%m-%d %H:%M:%S]"` $*" >> ${ovs_dir}/debug.log
+ [ $VERBOSE = 0 ] || echo $*
}
ovs_base=`pwd`
@@ -65,7 +67,8 @@ ovs_setenv() {
ovs_sbx() {
if test "X$2" != X; then
- (ovs_setenv $1; shift; "$@" >> ${ovs_dir}/debug.log)
+ (ovs_setenv $1; shift;
+info "run cmd: $@"; "$@" >> ${ovs_dir}/debug.log)
else
ovs_setenv $1
fi
@@ -139,7 +142,7 @@ ovs_add_flow () {
info "Adding flow to DP: sbx:$1 br:$2 flow:$3 act:$4"
ovs_sbx "$1" python3 $ovs_base/ovs-dpctl.py add-flow "$2" "$3" "$4"
if [ $? -ne 0 ]; then
- echo "Flow [ $3 : $4 ] failed" >> ${ovs_dir}/debug.log
+ info "Flow [ $3 : $4 ] failed"
return 1
fi
return 0
--
2.45.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH net-next 2/3] selftests: openvswitch: Attempt to autoload module.
Previously, the openvswitch.sh test suites would not attempt to autoload
the openvswitch module. The idea was that a user who is manually running
tests might not even have the OVS module loaded or configured for their
own development. However, if the kernel module is configured, and the
module can be autoloaded then we should just attempt to load it and run
the tests. This is especially true in the CI environments, where the CI
tests should be able to rely on auto loading to get the test suite running.
Signed-off-by: Aaron Conole
---
.../selftests/net/openvswitch/openvswitch.sh | 14 +-
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh
b/tools/testing/selftests/net/openvswitch/openvswitch.sh
index 15bca0708717..0bd0425848d9 100755
--- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
+++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
@@ -613,16 +613,20 @@ run_test() {
tname="$1"
tdesc="$2"
- if ! lsmod | grep openvswitch >/dev/null 2>&1; then
- stdbuf -o0 printf "TEST: %-60s [NOMOD]\n" "${tdesc}"
- return $ksft_skip
- fi
-
if python3 ovs-dpctl.py -h 2>&1 | \
grep -E "Need to (install|upgrade) the python" >/dev/null 2>&1;
then
stdbuf -o0 printf "TEST: %-60s [PYLIB]\n" "${tdesc}"
return $ksft_skip
fi
+
+ python3 ovs-dpctl.py show >/dev/null 2>&1 || \
+ echo "[DPCTL] show exception."
+
+ if ! lsmod | grep openvswitch >/dev/null 2>&1; then
+ stdbuf -o0 printf "TEST: %-60s [NOMOD]\n" "${tdesc}"
+ return $ksft_skip
+ fi
+
printf "TEST: %-60s [START]\n" "${tname}"
unset IFS
--
2.45.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH net-next 1/3] selftests: openvswitch: Bump timeout to 15 minutes.
We found that since some tests rely on the TCP SYN timeouts to cause flow misses, the default test suite timeout of 45 seconds is quick to be exceeded. Bump the timeout to 15 minutes. Signed-off-by: Aaron Conole --- tools/testing/selftests/net/openvswitch/settings | 1 + 1 file changed, 1 insertion(+) create mode 100644 tools/testing/selftests/net/openvswitch/settings diff --git a/tools/testing/selftests/net/openvswitch/settings b/tools/testing/selftests/net/openvswitch/settings new file mode 100644 index ..e2206265f67c --- /dev/null +++ b/tools/testing/selftests/net/openvswitch/settings @@ -0,0 +1 @@ +timeout=900 -- 2.45.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH net-next 0/3] selftests: openvswitch: Address some flakes in the CI environment
These patches aim to make using the openvswitch testsuite more reliable. These should address the major sources of flakiness in the openvswitch test suite allowing the CI infrastructure to exercise the openvswitch module for patch series. There should be no change for users who simply run the tests (except that patch 3/3 does make some of the debugging a bit easier by making some output more verbose). Aaron Conole (3): selftests: openvswitch: Bump timeout to 15 minutes. selftests: openvswitch: Attempt to autoload module. selftests: openvswitch: Be more verbose with selftest debugging. .../selftests/net/openvswitch/openvswitch.sh | 23 --- .../selftests/net/openvswitch/settings| 1 + 2 files changed, 16 insertions(+), 8 deletions(-) create mode 100644 tools/testing/selftests/net/openvswitch/settings -- 2.45.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next v7 05/10] net: openvswitch: add psample action
On Tue, Jul 02, 2024 at 11:53:01AM +0200, Ilya Maximets wrote:
> On 7/2/24 11:37, Simon Horman wrote:
> > On Tue, Jul 02, 2024 at 03:05:02AM -0400, Adrián Moreno wrote:
> >> On Mon, Jul 01, 2024 at 02:23:12PM GMT, Aaron Conole wrote:
> >>> Adrian Moreno writes:
> >
> > ...
> >
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> >
> > ...
> >
> @@ -1299,6 +1304,39 @@ static int execute_dec_ttl(struct sk_buff *skb,
> struct sw_flow_key *key)
> return 0;
> }
>
> +#if IS_ENABLED(CONFIG_PSAMPLE)
> +static void execute_psample(struct datapath *dp, struct sk_buff *skb,
> +const struct nlattr *attr)
> +{
> +struct psample_group psample_group = {};
> +struct psample_metadata md = {};
> +const struct nlattr *a;
> +int rem;
> +
> +nla_for_each_attr(a, nla_data(attr), nla_len(attr), rem) {
> +switch (nla_type(a)) {
> +case OVS_PSAMPLE_ATTR_GROUP:
> +psample_group.group_num = nla_get_u32(a);
> +break;
> +
> +case OVS_PSAMPLE_ATTR_COOKIE:
> +md.user_cookie = nla_data(a);
> +md.user_cookie_len = nla_len(a);
> +break;
> +}
> +}
> +
> +psample_group.net = ovs_dp_get_net(dp);
> +md.in_ifindex = OVS_CB(skb)->input_vport->dev->ifindex;
> +md.trunc_size = skb->len - OVS_CB(skb)->cutlen;
> +
> +psample_sample_packet(&psample_group, skb, 0, &md);
> +}
> +#else
> +static inline void execute_psample(struct datapath *dp, struct sk_buff
> *skb,
> + const struct nlattr *attr) {}
> >>>
> >>> I noticed that this got flagged in patchwork since it is 'static inline'
> >>> while being part of a complete translation unit - but I also see some
> >>> other places where that has been done. I guess it should be just
> >>> 'static' though. I don't feel very strongly about it.
> >>>
> >>
> >> We had a bit of discussion about this with Ilya. It seems "static
> >> inline" is a common pattern around the kernel. The coding style
> >> documentation says:
> >> "Generally, inline functions are preferable to macros resembling
> >> functions."
> >>
> >> So I think this "inline" is correct but I might be missing something.
> >
> > Hi Adrián,
> >
> > TL;DR: Please remove this inline keyword
> >
> > For Kernel networking code at least it is strongly preferred not
> > to use inline in .c files unless there is a demonstrable - usually
> > performance - reason to do so. Rather, it is preferred to let the
> > compiler decide when to inline such functions. OTOH, the inline
> > keyword in .h files is fine.
>
> FWIW, the main reason for 'inline' here is not performance, but silencing
> compiler's potential 'maybe unused' warnings:
>
> Function-like macros with unused parameters should be replaced by static
> inline functions to avoid the issue of unused variables
>
> I think, the rule for static inline functions in .c files is at odds with
> the 'Conditional Compilation' section of coding style. The section does
> recommend to avoid conditional function declaration in .c files, but I'm not
> sure it is reasonable to export internal static functions for that reason.
>
> In this particular case we can either define a macro, which is discouraged
> by the coding style:
>
> Generally, inline functions are preferable to macros resembling functions.
>
> Or create a static inline function, that is against rule of no static
> inline functions in .c files.
>
> Or create a simple static function and mark all the arguments as unused,
> which kind of compliant to the coding style, but the least pretty.
Hi Ilya,
I guess I would lean towards the last option.
But in any case, thanks for pointing out that this is complex:
I had not realised that when I wrote my previous email.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next v8 05/10] net: openvswitch: add psample action
Adrian Moreno writes: > Add support for a new action: psample. > > This action accepts a u32 group id and a variable-length cookie and uses > the psample multicast group to make the packet available for > observability. > > The maximum length of the user-defined cookie is set to 16, same as > tc_cookie, to discourage using cookies that will not be offloadable. > > Reviewed-by: Ilya Maximets > Acked-by: Eelco Chaudron > Signed-off-by: Adrian Moreno > --- Reviewed-by: Aaron Conole ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next v7 10/10] selftests: openvswitch: add psample test
Adrián Moreno writes:
> On Mon, Jul 01, 2024 at 02:38:44PM GMT, Aaron Conole wrote:
>> Adrian Moreno writes:
>>
>> > Add a test to verify sampling packets via psample works.
>> >
>> > In order to do that, create a subcommand in ovs-dpctl.py to listen to
>> > on the psample multicast group and print samples.
>> >
>> > Signed-off-by: Adrian Moreno
>> > ---
>> > .../selftests/net/openvswitch/openvswitch.sh | 115 +-
>> > .../selftests/net/openvswitch/ovs-dpctl.py| 73 ++-
>> > 2 files changed, 182 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh
>> > b/tools/testing/selftests/net/openvswitch/openvswitch.sh
>> > index 15bca0708717..02a366e01004 100755
>> > --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
>> > +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
>> > @@ -20,7 +20,8 @@ tests="
>> >nat_related_v4 ip4-nat-related: ICMP related
>> > matches work with SNAT
>> >netlink_checks ovsnl: validate netlink attrs
>> > and settings
>> >upcall_interfaces ovs: test the upcall interfaces
>> > - drop_reason drop: test drop reasons are
>> > emitted"
>> > + drop_reason drop: test drop reasons are
>> > emitted
>> > + psample psample: Sampling packets with
>> > psample"
>> >
>> > info() {
>> > [ $VERBOSE = 0 ] || echo $*
>> > @@ -102,12 +103,21 @@ ovs_netns_spawn_daemon() {
>> >shift
>> >netns=$1
>> >shift
>> > - info "spawning cmd: $*"
>> > - ip netns exec $netns $* >> $ovs_dir/stdout 2>> $ovs_dir/stderr &
>> > + if [ "$netns" == "_default" ]; then
>> > + $* >> $ovs_dir/stdout 2>> $ovs_dir/stderr &
>> > + else
>> > + ip netns exec $netns $* >> $ovs_dir/stdout 2>>
>> > $ovs_dir/stderr &
>> > + fi
>> >pid=$!
>> >ovs_sbx "$sbx" on_exit "kill -TERM $pid 2>/dev/null"
>> > }
>> >
>> > +ovs_spawn_daemon() {
>> > + sbx=$1
>> > + shift
>> > + ovs_netns_spawn_daemon $sbx "_default" $*
>> > +}
>> > +
>> > ovs_add_netns_and_veths () {
>> >info "Adding netns attached: sbx:$1 dp:$2 {$3, $4, $5}"
>> >ovs_sbx "$1" ip netns add "$3" || return 1
>> > @@ -170,6 +180,19 @@ ovs_drop_reason_count()
>> >return `echo "$perf_output" | grep "$pattern" | wc -l`
>> > }
>> >
>> > +ovs_test_flow_fails () {
>> > + ERR_MSG="Flow actions may not be safe on all matching packets"
>> > +
>> > + PRE_TEST=$(dmesg | grep -c "${ERR_MSG}")
>> > + ovs_add_flow $@ &> /dev/null $@ && return 1
>> > + POST_TEST=$(dmesg | grep -c "${ERR_MSG}")
>> > +
>> > + if [ "$PRE_TEST" == "$POST_TEST" ]; then
>> > + return 1
>> > + fi
>> > + return 0
>> > +}
>> > +
>> > usage() {
>> >echo
>> >echo "$0 [OPTIONS] [TEST]..."
>> > @@ -184,6 +207,92 @@ usage() {
>> >exit 1
>> > }
>> >
>> > +
>> > +# psample test
>> > +# - use psample to observe packets
>> > +test_psample() {
>> > + sbx_add "test_psample" || return $?
>> > +
>> > + # Add a datapath with per-vport dispatching.
>> > + ovs_add_dp "test_psample" psample -V 2:1 || return 1
>> > +
>> > + info "create namespaces"
>> > + ovs_add_netns_and_veths "test_psample" "psample" \
>> > + client c0 c1 172.31.110.10/24 -u || return 1
>> > + ovs_add_netns_and_veths "test_psample" "psample" \
>> > + server s0 s1 172.31.110.20/24 -u || return 1
>> > +
>> > + # Check if psample actions can be configured.
>> > + ovs_add_flow "test_psample" psample \
>> > + 'in_port(1),eth(),eth_type(0x0806),arp()' 'psample(group=1)'
>>
>> Might be good to redirect this stdout/stderr line to /dev/null -
>> otherwise on an unsupported system there will be the following extra
>> splat:
>>
>> Traceback (most recent call last):
>> File
>> "/home/aconole/git/linux/tools/testing/selftests/net/openvswitch/ovs-dpctl.py",
>> line 2774, in
>> sys.exit(main(sys.argv))
>>...
>> File "/usr/lib/python3.12/site-packages/pyroute2/netlink/nlsocket.py",
>> line 489, in get
>> raise msg['header']['error']
>> pyroute2.netlink.exceptions.NetlinkError: (22, 'Invalid argument')
>>
>
> I thought knowing the return value was kind of useful but sure, we can
> redirect it to /dev/null.
>
>> > + if [ $? == 1 ]; then
>> > + info "no support for psample - skipping"
>> > + ovs_exit_sig
>> > + return $ksft_skip
>> > + fi
>> > +
>> > + ovs_del_flows "test_psample" psample
>> > +
>> > + # Test action verification.
>> > + OLDIFS=$IFS
>> > + IFS='*'
>> > + min_key='in_port(1),eth(),eth_type(0x0800),ipv4()'
>> > + for testcase in \
>> > + "cookie to
>> > large"*"psample(group=1,cookie=1615141312111009080706050403020100)"
>> > \
>> > + "no group with cookie"*"psample(cookie=abcd)" \
>> > + "no group"*"psample()";
>> > + do
>> > + set -- $testcase;
>> > + ovs_test_flow_fail
Re: [ovs-dev] [PATCH net-next v8 02/10] net: sched: act_sample: add action cookie to sample
On Tue, Jul 02, 2024 at 11:53:19AM +0200, Adrian Moreno wrote:
> If the action has a user_cookie, pass it along to the sample so it can
> be easily identified.
>
> Reviewed-by: Aaron Conole
> Acked-by: Eelco Chaudron
> Reviewed-by: Ido Schimmel
> Signed-off-by: Adrian Moreno
> ---
> net/sched/act_sample.c | 12
> 1 file changed, 12 insertions(+)
>
> diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c
> index a69b53d54039..2ceb4d141b71 100644
> --- a/net/sched/act_sample.c
> +++ b/net/sched/act_sample.c
> @@ -167,7 +167,9 @@ TC_INDIRECT_SCOPE int tcf_sample_act(struct sk_buff *skb,
> {
> struct tcf_sample *s = to_sample(a);
> struct psample_group *psample_group;
> + u8 cookie_data[TC_COOKIE_MAX_SIZE];
> struct psample_metadata md = {};
> + struct tc_cookie *user_cookie;
> int retval;
>
> tcf_lastuse_update(&s->tcf_tm);
> @@ -189,6 +191,16 @@ TC_INDIRECT_SCOPE int tcf_sample_act(struct sk_buff *skb,
> if (skb_at_tc_ingress(skb) && tcf_sample_dev_ok_push(skb->dev))
> skb_push(skb, skb->mac_len);
>
> + rcu_read_lock();
> + user_cookie = rcu_dereference(a->user_cookie);
> + if (user_cookie) {
> + memcpy(cookie_data, user_cookie->data,
> +user_cookie->len);
> + md.user_cookie = cookie_data;
> + md.user_cookie_len = user_cookie->len;
> + }
> + rcu_read_unlock();
> +
> md.trunc_size = s->truncate ? s->trunc_size : skb->len;
> psample_sample_packet(psample_group, skb, s->rate, &md);
>
> --
> 2.45.2
>
>
Reviewed-by: Michal Kubiak
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next v8 05/10] net: openvswitch: add psample action
On Tue, Jul 02, 2024 at 11:53:22AM +0200, Adrian Moreno wrote: > Add support for a new action: psample. > > This action accepts a u32 group id and a variable-length cookie and uses > the psample multicast group to make the packet available for > observability. > > The maximum length of the user-defined cookie is set to 16, same as > tc_cookie, to discourage using cookies that will not be offloadable. > > Reviewed-by: Ilya Maximets > Acked-by: Eelco Chaudron > Signed-off-by: Adrian Moreno > --- Reviewed-by: Michal Kubiak ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next v7 05/10] net: openvswitch: add psample action
On Mon, Jul 01, 2024 at 12:56:43PM +, Adrián Moreno wrote:
> On Mon, Jul 01, 2024 at 01:40:39PM GMT, Michal Kubiak wrote:
> > On Sun, Jun 30, 2024 at 09:57:26PM +0200, Adrian Moreno wrote:
> > > Add support for a new action: psample.
> > >
> > > This action accepts a u32 group id and a variable-length cookie and uses
> > > the psample multicast group to make the packet available for
> > > observability.
> > >
> > > The maximum length of the user-defined cookie is set to 16, same as
> > > tc_cookie, to discourage using cookies that will not be offloadable.
> > >
> > > Acked-by: Eelco Chaudron
> > > Signed-off-by: Adrian Moreno
> > > ---
> > > Documentation/netlink/specs/ovs_flow.yaml | 17
> > > include/uapi/linux/openvswitch.h | 28 ++
> > > net/openvswitch/Kconfig | 1 +
> > > net/openvswitch/actions.c | 47 +++
> > > net/openvswitch/flow_netlink.c| 32 ++-
> > > 5 files changed, 124 insertions(+), 1 deletion(-)
> > >
[...]
> > > @@ -914,6 +914,31 @@ struct check_pkt_len_arg {
> > > };
> > > #endif
> > >
> > > +#define OVS_PSAMPLE_COOKIE_MAX_SIZE 16
> >
> > In your patch #2 you use "TC_COOKIE_MAX_SIZE" as an array size for your
> > cookie. I know that now OVS_PSAMPLE_COOKIE_MAX_SIZE == TC_COOKIE_MAX_SIZE,
> > so this size will be validated correctly.
> > But how likely is that those 2 constants will have different values in the
> > future?
> > Would it be reasonable to create more strict dependency between those
> > macros, e.g.:
> >
> > #define OVS_PSAMPLE_COOKIE_MAX_SIZE TC_COOKIE_MAX_SIZE
> >
> > or, at least, add a comment that the size shouldn't be bigger than
> > TC_COOKIE_MAX_SIZE?
> > I'm just considering the risk of exceeding the array from the patch #2 when
> > somebody increases OVS_PSAMPLE_COOKIE_MAX_SIZE in the future.
> >
> > Thanks,
> > Michal
> >
>
> Hi Michal,
>
> Thanks for sharing your thoughts.
>
> I tried to keep the dependency between both cookie sizes loose.
>
> I don't want a change in TC_COOKIE_MAX_SIZE to inadvertently modify
> OVS_PSAMPLE_COOKIE_MAX_SIZE because OVS might not need a bigger cookie
> and even if it does, backwards compatibility needs to be guaranteed:
> meaning OVS userspace will have to detect the new size and use it or
> fall back to a smaller cookie for older kernels. All this needs to be
> known and worked on in userspace.
>
> On the other hand, I intentionally made OVS's "psample" action as
> similar as possible to act_psample, including setting the same cookie
> size to begin with. The reason is that I think we should try to implement
> tc-flower offloading of this action using act_sample, plus 16 seemed a
> very reasonable max value.
>
> When we decide to support offloading the "psample" action, this must
> be done entirely in userspace. OVS must create a act_sample action
> (instead of the OVS "psample" one) via netlink. In no circumstances the
> openvswitch kmod interacts with tc directly.
>
> Now, back to your concern. If OVS_PSAMPLE_COOKIE_MAX_SIZE grows and
> TC_COOKIE_MAX_SIZE does not *and* we already support offloading this
> action to tc, the only consequence is that OVS userspace has a
> problem because the tc's netlink interface will reject cookies larger
> than TC_COOKIE_MAX_SIZE [1].
> This guarantees that the array in patch #2 is never overflown.
>
> OVS will have to deal with the different sizes and try to squeeze the
> data into TC_COOKIE_MAX_SIZE or fail to offload the action altogether.
>
> Psample does not have a size limit so different parts of the kernel can
> use psample with different internal max-sizes without any restriction.
>
> I hope this clears your concerns.
>
> [1] https://github.com/torvalds/linux/blob/master/net/sched/act_api.c#L1299
>
> Thanks.
> Adrián
>
Thank you, Adrian, for your detailed explanation. I wasn't aware of the
internal validation of that parameter using the mechanism from [1].
Sorry for asking the questions I should have answered by studying the
code more carefully.
I have no concerns about it now.
Thanks,
Reviewed-by: Michal Kubiak
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH]bond:Fixinaccurateloginfoinbond_shift_load.
Bleep bloop. Greetings Han Ding, 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 start with a capital. Subject: bond:Fixinaccurateloginfoinbond_shift_load. Lines checked: 53, 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]bond:Fixinaccurateloginfoinbond_shift_load.
When the delta is less than 1024 in bond_shift_load, it print "shift 0kB of
load".
Like this:
bond dpdkbond0: shift 0kB of load (with hash 71) from nic1 to nic2 (now
carrying 20650165kB and 8311662kB load, respectively)
Signed-off-by: Han Ding
---
ofproto/bond.c | 24 +---
1 file changed, 17 insertions(+), 7 deletions(-)
diff --git a/ofproto/bond.c b/ofproto/bond.c
index c31869a..5b1975d 100644
--- a/ofproto/bond.c
+++ b/ofproto/bond.c
@@ -1192,13 +1192,23 @@ bond_shift_load(struct bond_entry *hash, struct
bond_member *to)
struct bond *bond = from->bond;
uint64_t delta = hash->tx_bytes;
-VLOG_INFO("bond %s: shift %"PRIu64"kB of load (with hash %"PRIdPTR") "
- "from %s to %s (now carrying %"PRIu64"kB and "
- "%"PRIu64"kB load, respectively)",
- bond->name, delta / 1024, hash - bond->hash,
- from->name, to->name,
- (from->tx_bytes - delta) / 1024,
- (to->tx_bytes + delta) / 1024);
+if (delta >= 1024) {
+VLOG_INFO("bond %s: shift %"PRIu64"kB of load (with hash %"PRIdPTR") "
+"from %s to %s (now carrying %"PRIu64"kB and "
+"%"PRIu64"kB load, respectively)",
+bond->name, delta / 1024, hash - bond->hash,
+from->name, to->name,
+(from->tx_bytes - delta) / 1024,
+(to->tx_bytes + delta) / 1024);
+} else {
+VLOG_INFO("bond %s: shift %"PRIu64"B of load (with hash %"PRIdPTR") "
+"from %s to %s (now carrying %"PRIu64"kB and "
+"%"PRIu64"kB load, respectively)",
+bond->name, delta, hash - bond->hash,
+from->name, to->name,
+(from->tx_bytes - delta) / 1024,
+(to->tx_bytes + delta) / 1024);
+}
/* Shift load away from 'from' to 'to'. */
from->tx_bytes -= delta;
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH net-next v8 10/10] selftests: openvswitch: add psample test
Add a test to verify sampling packets via psample works.
In order to do that, create a subcommand in ovs-dpctl.py to listen to
on the psample multicast group and print samples.
Reviewed-by: Aaron Conole
Tested-by: Ilya Maximets
Signed-off-by: Adrian Moreno
---
.../selftests/net/openvswitch/openvswitch.sh | 115 +-
.../selftests/net/openvswitch/ovs-dpctl.py| 73 ++-
2 files changed, 182 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh
b/tools/testing/selftests/net/openvswitch/openvswitch.sh
index 15bca0708717..a33b63c6ef8f 100755
--- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
+++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
@@ -20,7 +20,8 @@ tests="
nat_related_v4 ip4-nat-related: ICMP related
matches work with SNAT
netlink_checks ovsnl: validate netlink attrs
and settings
upcall_interfaces ovs: test the upcall interfaces
- drop_reason drop: test drop reasons are
emitted"
+ drop_reason drop: test drop reasons are
emitted
+ psample psample: Sampling packets with
psample"
info() {
[ $VERBOSE = 0 ] || echo $*
@@ -102,12 +103,21 @@ ovs_netns_spawn_daemon() {
shift
netns=$1
shift
- info "spawning cmd: $*"
- ip netns exec $netns $* >> $ovs_dir/stdout 2>> $ovs_dir/stderr &
+ if [ "$netns" == "_default" ]; then
+ $* >> $ovs_dir/stdout 2>> $ovs_dir/stderr &
+ else
+ ip netns exec $netns $* >> $ovs_dir/stdout 2>>
$ovs_dir/stderr &
+ fi
pid=$!
ovs_sbx "$sbx" on_exit "kill -TERM $pid 2>/dev/null"
}
+ovs_spawn_daemon() {
+ sbx=$1
+ shift
+ ovs_netns_spawn_daemon $sbx "_default" $*
+}
+
ovs_add_netns_and_veths () {
info "Adding netns attached: sbx:$1 dp:$2 {$3, $4, $5}"
ovs_sbx "$1" ip netns add "$3" || return 1
@@ -170,6 +180,19 @@ ovs_drop_reason_count()
return `echo "$perf_output" | grep "$pattern" | wc -l`
}
+ovs_test_flow_fails () {
+ ERR_MSG="Flow actions may not be safe on all matching packets"
+
+ PRE_TEST=$(dmesg | grep -c "${ERR_MSG}")
+ ovs_add_flow $@ &> /dev/null $@ && return 1
+ POST_TEST=$(dmesg | grep -c "${ERR_MSG}")
+
+ if [ "$PRE_TEST" == "$POST_TEST" ]; then
+ return 1
+ fi
+ return 0
+}
+
usage() {
echo
echo "$0 [OPTIONS] [TEST]..."
@@ -184,6 +207,92 @@ usage() {
exit 1
}
+
+# psample test
+# - use psample to observe packets
+test_psample() {
+ sbx_add "test_psample" || return $?
+
+ # Add a datapath with per-vport dispatching.
+ ovs_add_dp "test_psample" psample -V 2:1 || return 1
+
+ info "create namespaces"
+ ovs_add_netns_and_veths "test_psample" "psample" \
+ client c0 c1 172.31.110.10/24 -u || return 1
+ ovs_add_netns_and_veths "test_psample" "psample" \
+ server s0 s1 172.31.110.20/24 -u || return 1
+
+ # Check if psample actions can be configured.
+ ovs_add_flow "test_psample" psample \
+ 'in_port(1),eth(),eth_type(0x0806),arp()' 'psample(group=1)' &>
/dev/null
+ if [ $? == 1 ]; then
+ info "no support for psample - skipping"
+ ovs_exit_sig
+ return $ksft_skip
+ fi
+
+ ovs_del_flows "test_psample" psample
+
+ # Test action verification.
+ OLDIFS=$IFS
+ IFS='*'
+ min_key='in_port(1),eth(),eth_type(0x0800),ipv4()'
+ for testcase in \
+ "cookie to
large"*"psample(group=1,cookie=1615141312111009080706050403020100)" \
+ "no group with cookie"*"psample(cookie=abcd)" \
+ "no group"*"psample()";
+ do
+ set -- $testcase;
+ ovs_test_flow_fails "test_psample" psample $min_key $2
+ if [ $? == 1 ]; then
+ info "failed - $1"
+ return 1
+ fi
+ done
+ IFS=$OLDIFS
+
+ ovs_del_flows "test_psample" psample
+ # Allow ARP
+ ovs_add_flow "test_psample" psample \
+ 'in_port(1),eth(),eth_type(0x0806),arp()' '2' || return 1
+ ovs_add_flow "test_psample" psample \
+ 'in_port(2),eth(),eth_type(0x0806),arp()' '1' || return 1
+
+ # Sample first 14 bytes of all traffic.
+ ovs_add_flow "test_psample" psample \
+ "in_port(1),eth(),eth_type(0x0800),ipv4()" \
+"trunc(14),psample(group=1,cookie=c0ffee),2"
+
+ # Sample all traffic. In this case, use a sample() action with both
+ # psample and an upcall emulating simultaneous local sampling and
+ # sFlow / IPFIX.
+ nlpid=$(grep -E "listening on upcall packet handler" \
+$ovs_d
[ovs-dev] [PATCH net-next v8 09/10] selftests: openvswitch: parse trunc action
The trunc action was supported decode-able but not parse-able. Add
support for parsing the action string.
Reviewed-by: Aaron Conole
Signed-off-by: Adrian Moreno
---
.../testing/selftests/net/openvswitch/ovs-dpctl.py | 13 +
1 file changed, 13 insertions(+)
diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
index 4ccf26f96327..e8dc9af10d4d 100644
--- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
+++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
@@ -863,6 +863,19 @@ class ovsactions(nla):
self["attrs"].append(["OVS_ACTION_ATTR_USERSPACE", uact])
parsed = True
+elif parse_starts_block(actstr, "trunc(", False):
+parencount += 1
+actstr, val = parse_extract_field(
+actstr,
+"trunc(",
+r"([0-9]+)",
+int,
+False,
+None,
+)
+self["attrs"].append(["OVS_ACTION_ATTR_TRUNC", val])
+parsed = True
+
actstr = actstr[strspn(actstr, ", ") :]
while parencount > 0:
parencount -= 1
--
2.45.2
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH net-next v8 08/10] selftests: openvswitch: add userspace parsing
The userspace action lacks parsing support plus it contains a bug in the
name of one of its attributes.
This patch makes userspace action work.
Reviewed-by: Aaron Conole
Signed-off-by: Adrian Moreno
---
.../selftests/net/openvswitch/ovs-dpctl.py| 24 +--
1 file changed, 22 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
index dcc400a21a22..4ccf26f96327 100644
--- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
+++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
@@ -589,13 +589,27 @@ class ovsactions(nla):
print_str += "userdata="
for f in self.get_attr("OVS_USERSPACE_ATTR_USERDATA"):
print_str += "%x." % f
-if self.get_attr("OVS_USERSPACE_ATTR_TUN_PORT") is not None:
+if self.get_attr("OVS_USERSPACE_ATTR_EGRESS_TUN_PORT") is not None:
print_str += "egress_tun_port=%d" % self.get_attr(
-"OVS_USERSPACE_ATTR_TUN_PORT"
+"OVS_USERSPACE_ATTR_EGRESS_TUN_PORT"
)
print_str += ")"
return print_str
+def parse(self, actstr):
+attrs_desc = (
+("pid", "OVS_USERSPACE_ATTR_PID", int),
+("userdata", "OVS_USERSPACE_ATTR_USERDATA",
+lambda x: list(bytearray.fromhex(x))),
+("egress_tun_port", "OVS_USERSPACE_ATTR_EGRESS_TUN_PORT", int)
+)
+
+attrs, actstr = parse_attrs(actstr, attrs_desc)
+for attr in attrs:
+self["attrs"].append(attr)
+
+return actstr
+
def dpstr(self, more=False):
print_str = ""
@@ -843,6 +857,12 @@ class ovsactions(nla):
self["attrs"].append(["OVS_ACTION_ATTR_PSAMPLE", psampleact])
parsed = True
+elif parse_starts_block(actstr, "userspace(", False):
+uact = self.userspace()
+actstr = uact.parse(actstr[len("userspace(") : ])
+self["attrs"].append(["OVS_ACTION_ATTR_USERSPACE", uact])
+parsed = True
+
actstr = actstr[strspn(actstr, ", ") :]
while parencount > 0:
parencount -= 1
--
2.45.2
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH net-next v8 07/10] selftests: openvswitch: add psample action
Add sample and psample action support to ovs-dpctl.py.
Refactor common attribute parsing logic into an external function.
Reviewed-by: Aaron Conole
Signed-off-by: Adrian Moreno
---
.../selftests/net/openvswitch/ovs-dpctl.py| 162 +-
1 file changed, 161 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
index 182a09975975..dcc400a21a22 100644
--- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
+++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
@@ -8,6 +8,7 @@ import argparse
import errno
import ipaddress
import logging
+import math
import multiprocessing
import re
import socket
@@ -60,6 +61,7 @@ OVS_FLOW_CMD_DEL = 2
OVS_FLOW_CMD_GET = 3
OVS_FLOW_CMD_SET = 4
+UINT32_MAX = 0x
def macstr(mac):
outstr = ":".join(["%02X" % i for i in mac])
@@ -281,6 +283,75 @@ def parse_extract_field(
return str_skipped, data
+def parse_attrs(actstr, attr_desc):
+"""Parses the given action string and returns a list of netlink
+attributes based on a list of attribute descriptions.
+
+Each element in the attribute description list is a tuple such as:
+(name, attr_name, parse_func)
+where:
+name: is the string representing the attribute
+attr_name: is the name of the attribute as defined in the uAPI.
+parse_func: is a callable accepting a string and returning either
+a single object (the parsed attribute value) or a tuple of
+two values (the parsed attribute value and the remaining string)
+
+Returns a list of attributes and the remaining string.
+"""
+def parse_attr(actstr, key, func):
+actstr = actstr[len(key) :]
+
+if not func:
+return None, actstr
+
+delim = actstr[0]
+actstr = actstr[1:]
+
+if delim == "=":
+pos = strcspn(actstr, ",)")
+ret = func(actstr[:pos])
+else:
+ret = func(actstr)
+
+if isinstance(ret, tuple):
+(datum, actstr) = ret
+else:
+datum = ret
+actstr = actstr[strcspn(actstr, ",)"):]
+
+if delim == "(":
+if not actstr or actstr[0] != ")":
+raise ValueError("Action contains unbalanced parentheses")
+
+actstr = actstr[1:]
+
+actstr = actstr[strspn(actstr, ", ") :]
+
+return datum, actstr
+
+attrs = []
+attr_desc = list(attr_desc)
+while actstr and actstr[0] != ")" and attr_desc:
+found = False
+for i, (key, attr, func) in enumerate(attr_desc):
+if actstr.startswith(key):
+datum, actstr = parse_attr(actstr, key, func)
+attrs.append([attr, datum])
+found = True
+del attr_desc[i]
+
+if not found:
+raise ValueError("Unknown attribute: '%s'" % actstr)
+
+actstr = actstr[strspn(actstr, ", ") :]
+
+if actstr[0] != ")":
+raise ValueError("Action string contains extra garbage or has "
+ "unbalanced parenthesis: '%s'" % actstr)
+
+return attrs, actstr[1:]
+
+
class ovs_dp_msg(genlmsg):
# include the OVS version
# We need a custom header rather than just being able to rely on
@@ -299,7 +370,7 @@ class ovsactions(nla):
("OVS_ACTION_ATTR_SET", "ovskey"),
("OVS_ACTION_ATTR_PUSH_VLAN", "none"),
("OVS_ACTION_ATTR_POP_VLAN", "flag"),
-("OVS_ACTION_ATTR_SAMPLE", "none"),
+("OVS_ACTION_ATTR_SAMPLE", "sample"),
("OVS_ACTION_ATTR_RECIRC", "uint32"),
("OVS_ACTION_ATTR_HASH", "none"),
("OVS_ACTION_ATTR_PUSH_MPLS", "none"),
@@ -318,8 +389,85 @@ class ovsactions(nla):
("OVS_ACTION_ATTR_ADD_MPLS", "none"),
("OVS_ACTION_ATTR_DEC_TTL", "none"),
("OVS_ACTION_ATTR_DROP", "uint32"),
+("OVS_ACTION_ATTR_PSAMPLE", "psample"),
)
+class psample(nla):
+nla_flags = NLA_F_NESTED
+
+nla_map = (
+("OVS_PSAMPLE_ATTR_UNSPEC", "none"),
+("OVS_PSAMPLE_ATTR_GROUP", "uint32"),
+("OVS_PSAMPLE_ATTR_COOKIE", "array(uint8)"),
+)
+
+def dpstr(self, more=False):
+args = "group=%d" % self.get_attr("OVS_PSAMPLE_ATTR_GROUP")
+
+cookie = self.get_attr("OVS_PSAMPLE_ATTR_COOKIE")
+if cookie:
+args += ",cookie(%s)" % \
+"".join(format(x, "02x") for x in cookie)
+
+return "psample(%s)" % args
+
+def parse(self, actstr):
+desc = (
+("group", "OVS_PSAMPLE_ATTR_GROUP", int),
+("cookie", "OVS_PSAMPLE_ATTR_COOKIE",
+lambda x: list(bytearray.fromhex(x)))
+)
+
+attrs, actstr = parse_attrs(actstr, desc)
+
+for attr in attrs:
+self["attrs"].append(att
[ovs-dev] [PATCH net-next v8 06/10] net: openvswitch: store sampling probability in cb.
When a packet sample is observed, the sampling rate that was used is
important to estimate the real frequency of such event.
Store the probability of the parent sample action in the skb's cb area
and use it in psample action to pass it down to psample module.
Reviewed-by: Aaron Conole
Acked-by: Eelco Chaudron
Reviewed-by: Ilya Maximets
Signed-off-by: Adrian Moreno
---
include/uapi/linux/openvswitch.h | 3 ++-
net/openvswitch/actions.c| 20 +---
net/openvswitch/datapath.h | 3 +++
net/openvswitch/vport.c | 1 +
4 files changed, 23 insertions(+), 4 deletions(-)
diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index 3dd653748725..3a701bd1f31b 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -649,7 +649,8 @@ enum ovs_flow_attr {
* Actions are passed as nested attributes.
*
* Executes the specified actions with the given probability on a per-packet
- * basis.
+ * basis. Nested actions will be able to access the probability value of the
+ * parent @OVS_ACTION_ATTR_SAMPLE.
*/
enum ovs_sample_attr {
OVS_SAMPLE_ATTR_UNSPEC,
diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 892d7e48fc5b..101f9a23792c 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -1048,12 +1048,15 @@ static int sample(struct datapath *dp, struct sk_buff
*skb,
struct nlattr *sample_arg;
int rem = nla_len(attr);
const struct sample_arg *arg;
+ u32 init_probability;
bool clone_flow_key;
+ int err;
/* The first action is always 'OVS_SAMPLE_ATTR_ARG'. */
sample_arg = nla_data(attr);
arg = nla_data(sample_arg);
actions = nla_next(sample_arg, &rem);
+ init_probability = OVS_CB(skb)->probability;
if ((arg->probability != U32_MAX) &&
(!arg->probability || get_random_u32() > arg->probability)) {
@@ -1062,9 +1065,16 @@ static int sample(struct datapath *dp, struct sk_buff
*skb,
return 0;
}
+ OVS_CB(skb)->probability = arg->probability;
+
clone_flow_key = !arg->exec;
- return clone_execute(dp, skb, key, 0, actions, rem, last,
-clone_flow_key);
+ err = clone_execute(dp, skb, key, 0, actions, rem, last,
+ clone_flow_key);
+
+ if (!last)
+ OVS_CB(skb)->probability = init_probability;
+
+ return err;
}
/* When 'last' is true, clone() should always consume the 'skb'.
@@ -1311,6 +1321,7 @@ static void execute_psample(struct datapath *dp, struct
sk_buff *skb,
struct psample_group psample_group = {};
struct psample_metadata md = {};
const struct nlattr *a;
+ u32 rate;
int rem;
nla_for_each_attr(a, nla_data(attr), nla_len(attr), rem) {
@@ -1329,8 +1340,11 @@ static void execute_psample(struct datapath *dp, struct
sk_buff *skb,
psample_group.net = ovs_dp_get_net(dp);
md.in_ifindex = OVS_CB(skb)->input_vport->dev->ifindex;
md.trunc_size = skb->len - OVS_CB(skb)->cutlen;
+ md.rate_as_probability = 1;
+
+ rate = OVS_CB(skb)->probability ? OVS_CB(skb)->probability : U32_MAX;
- psample_sample_packet(&psample_group, skb, 0, &md);
+ psample_sample_packet(&psample_group, skb, rate, &md);
}
#else
static void execute_psample(struct datapath *dp, struct sk_buff *skb,
diff --git a/net/openvswitch/datapath.h b/net/openvswitch/datapath.h
index 0cd29971a907..9ca6231ea647 100644
--- a/net/openvswitch/datapath.h
+++ b/net/openvswitch/datapath.h
@@ -115,12 +115,15 @@ struct datapath {
* fragmented.
* @acts_origlen: The netlink size of the flow actions applied to this skb.
* @cutlen: The number of bytes from the packet end to be removed.
+ * @probability: The sampling probability that was applied to this skb; 0 means
+ * no sampling has occurred; U32_MAX means 100% probability.
*/
struct ovs_skb_cb {
struct vport*input_vport;
u16 mru;
u16 acts_origlen;
u32 cutlen;
+ u32 probability;
};
#define OVS_CB(skb) ((struct ovs_skb_cb *)(skb)->cb)
diff --git a/net/openvswitch/vport.c b/net/openvswitch/vport.c
index 972ae01a70f7..8732f6e51ae5 100644
--- a/net/openvswitch/vport.c
+++ b/net/openvswitch/vport.c
@@ -500,6 +500,7 @@ int ovs_vport_receive(struct vport *vport, struct sk_buff
*skb,
OVS_CB(skb)->input_vport = vport;
OVS_CB(skb)->mru = 0;
OVS_CB(skb)->cutlen = 0;
+ OVS_CB(skb)->probability = 0;
if (unlikely(dev_net(skb->dev) != ovs_dp_get_net(vport->dp))) {
u32 mark;
--
2.45.2
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH net-next v8 05/10] net: openvswitch: add psample action
Add support for a new action: psample.
This action accepts a u32 group id and a variable-length cookie and uses
the psample multicast group to make the packet available for
observability.
The maximum length of the user-defined cookie is set to 16, same as
tc_cookie, to discourage using cookies that will not be offloadable.
Reviewed-by: Ilya Maximets
Acked-by: Eelco Chaudron
Signed-off-by: Adrian Moreno
---
Documentation/netlink/specs/ovs_flow.yaml | 17
include/uapi/linux/openvswitch.h | 28 +
net/openvswitch/Kconfig | 1 +
net/openvswitch/actions.c | 48 +++
net/openvswitch/flow_netlink.c| 32 ++-
5 files changed, 125 insertions(+), 1 deletion(-)
diff --git a/Documentation/netlink/specs/ovs_flow.yaml
b/Documentation/netlink/specs/ovs_flow.yaml
index 4fdfc6b5cae9..46f5d1cd8a5f 100644
--- a/Documentation/netlink/specs/ovs_flow.yaml
+++ b/Documentation/netlink/specs/ovs_flow.yaml
@@ -727,6 +727,12 @@ attribute-sets:
name: dec-ttl
type: nest
nested-attributes: dec-ttl-attrs
+ -
+name: psample
+type: nest
+nested-attributes: psample-attrs
+doc: |
+ Sends a packet sample to psample for external observation.
-
name: tunnel-key-attrs
enum-name: ovs-tunnel-key-attr
@@ -938,6 +944,17 @@ attribute-sets:
-
name: gbp
type: u32
+ -
+name: psample-attrs
+enum-name: ovs-psample-attr
+name-prefix: ovs-psample-attr-
+attributes:
+ -
+name: group
+type: u32
+ -
+name: cookie
+type: binary
operations:
name-prefix: ovs-flow-cmd-
diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index efc82c318fa2..3dd653748725 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -914,6 +914,31 @@ struct check_pkt_len_arg {
};
#endif
+#define OVS_PSAMPLE_COOKIE_MAX_SIZE 16
+/**
+ * enum ovs_psample_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: */
+ __OVS_PSAMPLE_ATTR_MAX
+};
+
+#define OVS_PSAMPLE_ATTR_MAX (__OVS_PSAMPLE_ATTR_MAX - 1)
+
/**
* enum ovs_action_attr - Action types.
*
@@ -966,6 +991,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.
*
* Only a single header can be set with a single %OVS_ACTION_ATTR_SET. Not all
* fields within a header are modifiable, e.g. the IPv4 protocol and fragment
@@ -1004,6 +1031,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 error code. */
+ OVS_ACTION_ATTR_PSAMPLE, /* Nested OVS_PSAMPLE_ATTR_*. */
__OVS_ACTION_ATTR_MAX,/* Nothing past this will be accepted
* from userspace. */
diff --git a/net/openvswitch/Kconfig b/net/openvswitch/Kconfig
index 29a7081858cd..2535f3f9f462 100644
--- a/net/openvswitch/Kconfig
+++ b/net/openvswitch/Kconfig
@@ -10,6 +10,7 @@ config OPENVSWITCH
(NF_CONNTRACK && ((!NF_DEFRAG_IPV6 || NF_DEFRAG_IPV6) && \
(!NF_NAT || NF_NAT) && \
(!NETFILTER_CONNCOUNT ||
NETFILTER_CONNCOUNT)))
+ depends on PSAMPLE || !PSAMPLE
select LIBCRC32C
select MPLS
select NET_MPLS_GSO
diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 964225580824..892d7e48fc5b 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -24,6 +24,11 @@
#include
#include
#include
+
+#if IS_ENABLED(CONFIG_PSAMPLE)
+#include
+#endif
+
#include
#include "datapath.h"
@@ -1299,6 +1304,40 @@ static int execute_dec_ttl(struct sk_buff *skb, struct
sw_flow_key *key)
return 0;
}
+#if IS_ENABLED(CONFIG_PSAMPLE)
+static void execute_psample(struct datapath *dp, struct sk_buff *skb,
+ const struct nlattr *attr)
+{
+ struct psample_group psample_g
[ovs-dev] [PATCH net-next v8 04/10] net: psample: allow using rate as probability
Although not explicitly documented in the psample module itself, the
definition of PSAMPLE_ATTR_SAMPLE_RATE seems inherited from act_sample.
Quoting tc-sample(8):
"RATE of 100 will lead to an average of one sampled packet out of every
100 observed."
With this semantics, the rates that we can express with an unsigned
32-bits number are very unevenly distributed and concentrated towards
"sampling few packets".
For example, we can express a probability of 2.32E-8% but we
cannot express anything between 100% and 50%.
For sampling applications that are capable of sampling a decent
amount of packets, this sampling rate semantics is not very useful.
Add a new flag to the uAPI that indicates that the sampling rate is
expressed in scaled probability, this is:
- 0 is 0% probability, no packets get sampled.
- U32_MAX is 100% probability, all packets get sampled.
Reviewed-by: Aaron Conole
Acked-by: Eelco Chaudron
Reviewed-by: Ido Schimmel
Signed-off-by: Adrian Moreno
---
include/net/psample.h| 3 ++-
include/uapi/linux/psample.h | 10 +-
net/psample/psample.c| 3 +++
3 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/include/net/psample.h b/include/net/psample.h
index 2ac71260a546..c52e9ebd88dd 100644
--- a/include/net/psample.h
+++ b/include/net/psample.h
@@ -24,7 +24,8 @@ struct psample_metadata {
u8 out_tc_valid:1,
out_tc_occ_valid:1,
latency_valid:1,
- unused:5;
+ rate_as_probability:1,
+ unused:4;
const u8 *user_cookie;
u32 user_cookie_len;
};
diff --git a/include/uapi/linux/psample.h b/include/uapi/linux/psample.h
index e80637e1d97b..b765f0e81f20 100644
--- a/include/uapi/linux/psample.h
+++ b/include/uapi/linux/psample.h
@@ -8,7 +8,11 @@ enum {
PSAMPLE_ATTR_ORIGSIZE,
PSAMPLE_ATTR_SAMPLE_GROUP,
PSAMPLE_ATTR_GROUP_SEQ,
- PSAMPLE_ATTR_SAMPLE_RATE,
+ PSAMPLE_ATTR_SAMPLE_RATE, /* u32, ratio between observed and
+* sampled packets or scaled probability
+* if PSAMPLE_ATTR_SAMPLE_PROBABILITY
+* is set.
+*/
PSAMPLE_ATTR_DATA,
PSAMPLE_ATTR_GROUP_REFCOUNT,
PSAMPLE_ATTR_TUNNEL,
@@ -20,6 +24,10 @@ enum {
PSAMPLE_ATTR_TIMESTAMP, /* u64, nanoseconds */
PSAMPLE_ATTR_PROTO, /* u16 */
PSAMPLE_ATTR_USER_COOKIE, /* binary, user provided data */
+ PSAMPLE_ATTR_SAMPLE_PROBABILITY,/* no argument, interpret rate in
+* PSAMPLE_ATTR_SAMPLE_RATE as a
+* probability scaled 0 - U32_MAX.
+*/
__PSAMPLE_ATTR_MAX
};
diff --git a/net/psample/psample.c b/net/psample/psample.c
index 1c76f3e48dcd..f48b5b9cd409 100644
--- a/net/psample/psample.c
+++ b/net/psample/psample.c
@@ -497,6 +497,9 @@ void psample_sample_packet(struct psample_group *group,
struct sk_buff *skb,
md->user_cookie))
goto error;
+ if (md->rate_as_probability)
+ nla_put_flag(skb, PSAMPLE_ATTR_SAMPLE_PROBABILITY);
+
genlmsg_end(nl_skb, data);
genlmsg_multicast_netns(&psample_nl_family, group->net, nl_skb, 0,
PSAMPLE_NL_MCGRP_SAMPLE, GFP_ATOMIC);
--
2.45.2
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH net-next v8 03/10] net: psample: skip packet copy if no listeners
If nobody is listening on the multicast group, generating the sample, which involves copying packet data, seems completely unnecessary. Return fast in this case. Reviewed-by: Aaron Conole Acked-by: Eelco Chaudron Reviewed-by: Ido Schimmel Reviewed-by: Simon Horman Signed-off-by: Adrian Moreno --- net/psample/psample.c | 4 1 file changed, 4 insertions(+) diff --git a/net/psample/psample.c b/net/psample/psample.c index b37488f426bc..1c76f3e48dcd 100644 --- a/net/psample/psample.c +++ b/net/psample/psample.c @@ -376,6 +376,10 @@ void psample_sample_packet(struct psample_group *group, struct sk_buff *skb, void *data; int ret; + if (!genl_has_listeners(&psample_nl_family, group->net, + PSAMPLE_NL_MCGRP_SAMPLE)) + return; + meta_len = (in_ifindex ? nla_total_size(sizeof(u16)) : 0) + (out_ifindex ? nla_total_size(sizeof(u16)) : 0) + (md->out_tc_valid ? nla_total_size(sizeof(u16)) : 0) + -- 2.45.2 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH net-next v8 01/10] net: psample: add user cookie
Add a user cookie to the sample metadata so that sample emitters can
provide more contextual information to samples.
If present, send the user cookie in a new attribute:
PSAMPLE_ATTR_USER_COOKIE.
Reviewed-by: Michal Kubiak
Acked-by: Eelco Chaudron
Reviewed-by: Simon Horman
Reviewed-by: Ido Schimmel
Signed-off-by: Adrian Moreno
---
include/net/psample.h| 2 ++
include/uapi/linux/psample.h | 1 +
net/psample/psample.c| 9 -
3 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/include/net/psample.h b/include/net/psample.h
index 0509d2d6be67..2ac71260a546 100644
--- a/include/net/psample.h
+++ b/include/net/psample.h
@@ -25,6 +25,8 @@ struct psample_metadata {
out_tc_occ_valid:1,
latency_valid:1,
unused:5;
+ const u8 *user_cookie;
+ u32 user_cookie_len;
};
struct psample_group *psample_group_get(struct net *net, u32 group_num);
diff --git a/include/uapi/linux/psample.h b/include/uapi/linux/psample.h
index e585db5bf2d2..e80637e1d97b 100644
--- a/include/uapi/linux/psample.h
+++ b/include/uapi/linux/psample.h
@@ -19,6 +19,7 @@ enum {
PSAMPLE_ATTR_LATENCY, /* u64, nanoseconds */
PSAMPLE_ATTR_TIMESTAMP, /* u64, nanoseconds */
PSAMPLE_ATTR_PROTO, /* u16 */
+ PSAMPLE_ATTR_USER_COOKIE, /* binary, user provided data */
__PSAMPLE_ATTR_MAX
};
diff --git a/net/psample/psample.c b/net/psample/psample.c
index a5d9b8446f77..b37488f426bc 100644
--- a/net/psample/psample.c
+++ b/net/psample/psample.c
@@ -386,7 +386,9 @@ void psample_sample_packet(struct psample_group *group,
struct sk_buff *skb,
nla_total_size(sizeof(u32)) +/* group_num */
nla_total_size(sizeof(u32)) +/* seq */
nla_total_size_64bit(sizeof(u64)) + /* timestamp */
- nla_total_size(sizeof(u16)); /* protocol */
+ nla_total_size(sizeof(u16)) +/* protocol */
+ (md->user_cookie_len ?
+ nla_total_size(md->user_cookie_len) : 0); /* user cookie */
#ifdef CONFIG_INET
tun_info = skb_tunnel_info(skb);
@@ -486,6 +488,11 @@ void psample_sample_packet(struct psample_group *group,
struct sk_buff *skb,
}
#endif
+ if (md->user_cookie && md->user_cookie_len &&
+ nla_put(nl_skb, PSAMPLE_ATTR_USER_COOKIE, md->user_cookie_len,
+ md->user_cookie))
+ goto error;
+
genlmsg_end(nl_skb, data);
genlmsg_multicast_netns(&psample_nl_family, group->net, nl_skb, 0,
PSAMPLE_NL_MCGRP_SAMPLE, GFP_ATOMIC);
--
2.45.2
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH net-next v8 02/10] net: sched: act_sample: add action cookie to sample
If the action has a user_cookie, pass it along to the sample so it can
be easily identified.
Reviewed-by: Aaron Conole
Acked-by: Eelco Chaudron
Reviewed-by: Ido Schimmel
Signed-off-by: Adrian Moreno
---
net/sched/act_sample.c | 12
1 file changed, 12 insertions(+)
diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c
index a69b53d54039..2ceb4d141b71 100644
--- a/net/sched/act_sample.c
+++ b/net/sched/act_sample.c
@@ -167,7 +167,9 @@ TC_INDIRECT_SCOPE int tcf_sample_act(struct sk_buff *skb,
{
struct tcf_sample *s = to_sample(a);
struct psample_group *psample_group;
+ u8 cookie_data[TC_COOKIE_MAX_SIZE];
struct psample_metadata md = {};
+ struct tc_cookie *user_cookie;
int retval;
tcf_lastuse_update(&s->tcf_tm);
@@ -189,6 +191,16 @@ TC_INDIRECT_SCOPE int tcf_sample_act(struct sk_buff *skb,
if (skb_at_tc_ingress(skb) && tcf_sample_dev_ok_push(skb->dev))
skb_push(skb, skb->mac_len);
+ rcu_read_lock();
+ user_cookie = rcu_dereference(a->user_cookie);
+ if (user_cookie) {
+ memcpy(cookie_data, user_cookie->data,
+ user_cookie->len);
+ md.user_cookie = cookie_data;
+ md.user_cookie_len = user_cookie->len;
+ }
+ rcu_read_unlock();
+
md.trunc_size = s->truncate ? s->trunc_size : skb->len;
psample_sample_packet(psample_group, skb, s->rate, &md);
--
2.45.2
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH net-next v8 00/10] net: openvswitch: Add sample multicasting.
** Background **
Currently, OVS supports several packet sampling mechanisms (sFlow,
per-bridge IPFIX, per-flow IPFIX). These end up being translated into a
userspace action that needs to be handled by ovs-vswitchd's handler
threads only to be forwarded to some third party application that
will somehow process the sample and provide observability on the
datapath.
A particularly interesting use-case is controller-driven
per-flow IPFIX sampling where the OpenFlow controller can add metadata
to samples (via two 32bit integers) and this metadata is then available
to the sample-collecting system for correlation.
** Problem **
The fact that sampled traffic share netlink sockets and handler thread
time with upcalls, apart from being a performance bottleneck in the
sample extraction itself, can severely compromise the datapath,
yielding this solution unfit for highly loaded production systems.
Users are left with little options other than guessing what sampling
rate will be OK for their traffic pattern and system load and dealing
with the lost accuracy.
Looking at available infrastructure, an obvious candidated would be
to use psample. However, it's current state does not help with the
use-case at stake because sampled packets do not contain user-defined
metadata.
** Proposal **
This series is an attempt to fix this situation by extending the
existing psample infrastructure to carry a variable length
user-defined cookie.
The main existing user of psample is tc's act_sample. It is also
extended to forward the action's cookie to psample.
Finally, a new OVS action (OVS_SAMPLE_ATTR_PSAMPLE) is created.
It accepts a group and an optional cookie and uses psample to
multicast the packet and the metadata.
--
v7 -> v8:
- Rebased
- Redirect flow insertion to /dev/null to avoid spat in test.
- Removed inline keyword in stub execute_psample_action function.
v6 -> v7:
- Rebased
- Fixed typo in comment.
v5 -> v6:
- Renamed emit_sample -> psample
- Addressed unused variable and conditionally compilation of function.
v4 -> v5:
- Rebased.
- Removed lefover enum value and wrapped some long lines in selftests.
v3 -> v4:
- Rebased.
- Addressed Jakub's comment on private and unused nla attributes.
v2 -> v3:
- Addressed comments from Simon, Aaron and Ilya.
- Dropped probability propagation in nested sample actions.
- Dropped patch v2's 7/9 in favor of a userspace implementation and
consume skb if emit_sample is the last action, same as we do with
userspace.
- Split ovs-dpctl.py features in independent patches.
v1 -> v2:
- Create a new action ("emit_sample") rather than reuse existing
"sample" one.
- Add probability semantics to psample's sampling rate.
- Store sampling probability in skb's cb area and use it in emit_sample.
- Test combining "emit_sample" with "trunc"
- Drop group_id filtering and tracepoint in psample.
rfc_v2 -> v1:
- Accommodate Ilya's comments.
- Split OVS's attribute in two attributes and simplify internal
handling of psample arguments.
- Extend psample and tc with a user-defined cookie.
- Add a tracepoint to psample to facilitate troubleshooting.
rfc_v1 -> rfc_v2:
- Use psample instead of a new OVS-only multicast group.
- Extend psample and tc with a user-defined cookie.
Adrian Moreno (10):
net: psample: add user cookie
net: sched: act_sample: add action cookie to sample
net: psample: skip packet copy if no listeners
net: psample: allow using rate as probability
net: openvswitch: add psample action
net: openvswitch: store sampling probability in cb.
selftests: openvswitch: add psample action
selftests: openvswitch: add userspace parsing
selftests: openvswitch: parse trunc action
selftests: openvswitch: add psample test
Documentation/netlink/specs/ovs_flow.yaml | 17 ++
include/net/psample.h | 5 +-
include/uapi/linux/openvswitch.h | 31 +-
include/uapi/linux/psample.h | 11 +-
net/openvswitch/Kconfig | 1 +
net/openvswitch/actions.c | 66 -
net/openvswitch/datapath.h| 3 +
net/openvswitch/flow_netlink.c| 32 ++-
net/openvswitch/vport.c | 1 +
net/psample/psample.c | 16 +-
net/sched/act_sample.c| 12 +
.../selftests/net/openvswitch/openvswitch.sh | 115 +++-
.../selftests/net/openvswitch/ovs-dpctl.py| 272 +-
13 files changed, 566 insertions(+), 16 deletions(-)
--
2.45.2
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next v7 05/10] net: openvswitch: add psample action
On 7/2/24 11:37, Simon Horman wrote:
> On Tue, Jul 02, 2024 at 03:05:02AM -0400, Adrián Moreno wrote:
>> On Mon, Jul 01, 2024 at 02:23:12PM GMT, Aaron Conole wrote:
>>> Adrian Moreno writes:
>
> ...
>
diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
>
> ...
>
@@ -1299,6 +1304,39 @@ static int execute_dec_ttl(struct sk_buff *skb,
struct sw_flow_key *key)
return 0;
}
+#if IS_ENABLED(CONFIG_PSAMPLE)
+static void execute_psample(struct datapath *dp, struct sk_buff *skb,
+ const struct nlattr *attr)
+{
+ struct psample_group psample_group = {};
+ struct psample_metadata md = {};
+ const struct nlattr *a;
+ int rem;
+
+ nla_for_each_attr(a, nla_data(attr), nla_len(attr), rem) {
+ switch (nla_type(a)) {
+ case OVS_PSAMPLE_ATTR_GROUP:
+ psample_group.group_num = nla_get_u32(a);
+ break;
+
+ case OVS_PSAMPLE_ATTR_COOKIE:
+ md.user_cookie = nla_data(a);
+ md.user_cookie_len = nla_len(a);
+ break;
+ }
+ }
+
+ psample_group.net = ovs_dp_get_net(dp);
+ md.in_ifindex = OVS_CB(skb)->input_vport->dev->ifindex;
+ md.trunc_size = skb->len - OVS_CB(skb)->cutlen;
+
+ psample_sample_packet(&psample_group, skb, 0, &md);
+}
+#else
+static inline void execute_psample(struct datapath *dp, struct sk_buff
*skb,
+ const struct nlattr *attr) {}
>>>
>>> I noticed that this got flagged in patchwork since it is 'static inline'
>>> while being part of a complete translation unit - but I also see some
>>> other places where that has been done. I guess it should be just
>>> 'static' though. I don't feel very strongly about it.
>>>
>>
>> We had a bit of discussion about this with Ilya. It seems "static
>> inline" is a common pattern around the kernel. The coding style
>> documentation says:
>> "Generally, inline functions are preferable to macros resembling functions."
>>
>> So I think this "inline" is correct but I might be missing something.
>
> Hi Adrián,
>
> TL;DR: Please remove this inline keyword
>
> For Kernel networking code at least it is strongly preferred not
> to use inline in .c files unless there is a demonstrable - usually
> performance - reason to do so. Rather, it is preferred to let the
> compiler decide when to inline such functions. OTOH, the inline
> keyword in .h files is fine.
FWIW, the main reason for 'inline' here is not performance, but silencing
compiler's potential 'maybe unused' warnings:
Function-like macros with unused parameters should be replaced by static
inline functions to avoid the issue of unused variables
I think, the rule for static inline functions in .c files is at odds with
the 'Conditional Compilation' section of coding style. The section does
recommend to avoid conditional function declaration in .c files, but I'm not
sure it is reasonable to export internal static functions for that reason.
In this particular case we can either define a macro, which is discouraged
by the coding style:
Generally, inline functions are preferable to macros resembling functions.
Or create a static inline function, that is against rule of no static
inline functions in .c files.
Or create a simple static function and mark all the arguments as unused,
which kind of compliant to the coding style, but the least pretty.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next v7 05/10] net: openvswitch: add psample action
On Tue, Jul 02, 2024 at 10:37:26AM GMT, Simon Horman wrote:
> On Tue, Jul 02, 2024 at 03:05:02AM -0400, Adrián Moreno wrote:
> > On Mon, Jul 01, 2024 at 02:23:12PM GMT, Aaron Conole wrote:
> > > Adrian Moreno writes:
>
> ...
>
> > > > diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
>
> ...
>
> > > > @@ -1299,6 +1304,39 @@ static int execute_dec_ttl(struct sk_buff *skb,
> > > > struct sw_flow_key *key)
> > > > return 0;
> > > > }
> > > >
> > > > +#if IS_ENABLED(CONFIG_PSAMPLE)
> > > > +static void execute_psample(struct datapath *dp, struct sk_buff *skb,
> > > > + const struct nlattr *attr)
> > > > +{
> > > > + struct psample_group psample_group = {};
> > > > + struct psample_metadata md = {};
> > > > + const struct nlattr *a;
> > > > + int rem;
> > > > +
> > > > + nla_for_each_attr(a, nla_data(attr), nla_len(attr), rem) {
> > > > + switch (nla_type(a)) {
> > > > + case OVS_PSAMPLE_ATTR_GROUP:
> > > > + psample_group.group_num = nla_get_u32(a);
> > > > + break;
> > > > +
> > > > + case OVS_PSAMPLE_ATTR_COOKIE:
> > > > + md.user_cookie = nla_data(a);
> > > > + md.user_cookie_len = nla_len(a);
> > > > + break;
> > > > + }
> > > > + }
> > > > +
> > > > + psample_group.net = ovs_dp_get_net(dp);
> > > > + md.in_ifindex = OVS_CB(skb)->input_vport->dev->ifindex;
> > > > + md.trunc_size = skb->len - OVS_CB(skb)->cutlen;
> > > > +
> > > > + psample_sample_packet(&psample_group, skb, 0, &md);
> > > > +}
> > > > +#else
> > > > +static inline void execute_psample(struct datapath *dp, struct sk_buff
> > > > *skb,
> > > > + const struct nlattr *attr) {}
> > >
> > > I noticed that this got flagged in patchwork since it is 'static inline'
> > > while being part of a complete translation unit - but I also see some
> > > other places where that has been done. I guess it should be just
> > > 'static' though. I don't feel very strongly about it.
> > >
> >
> > We had a bit of discussion about this with Ilya. It seems "static
> > inline" is a common pattern around the kernel. The coding style
> > documentation says:
> > "Generally, inline functions are preferable to macros resembling functions."
> >
> > So I think this "inline" is correct but I might be missing something.
>
> Hi Adrián,
>
> TL;DR: Please remove this inline keyword
>
> For Kernel networking code at least it is strongly preferred not
> to use inline in .c files unless there is a demonstrable - usually
> performance - reason to do so. Rather, it is preferred to let the
> compiler decide when to inline such functions. OTOH, the inline
> keyword in .h files is fine.
>
Ok. I'll send a new version.
Thanks.
Adrián
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next v7 05/10] net: openvswitch: add psample action
On Tue, Jul 02, 2024 at 03:05:02AM -0400, Adrián Moreno wrote:
> On Mon, Jul 01, 2024 at 02:23:12PM GMT, Aaron Conole wrote:
> > Adrian Moreno writes:
...
> > > diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
...
> > > @@ -1299,6 +1304,39 @@ static int execute_dec_ttl(struct sk_buff *skb,
> > > struct sw_flow_key *key)
> > > return 0;
> > > }
> > >
> > > +#if IS_ENABLED(CONFIG_PSAMPLE)
> > > +static void execute_psample(struct datapath *dp, struct sk_buff *skb,
> > > + const struct nlattr *attr)
> > > +{
> > > + struct psample_group psample_group = {};
> > > + struct psample_metadata md = {};
> > > + const struct nlattr *a;
> > > + int rem;
> > > +
> > > + nla_for_each_attr(a, nla_data(attr), nla_len(attr), rem) {
> > > + switch (nla_type(a)) {
> > > + case OVS_PSAMPLE_ATTR_GROUP:
> > > + psample_group.group_num = nla_get_u32(a);
> > > + break;
> > > +
> > > + case OVS_PSAMPLE_ATTR_COOKIE:
> > > + md.user_cookie = nla_data(a);
> > > + md.user_cookie_len = nla_len(a);
> > > + break;
> > > + }
> > > + }
> > > +
> > > + psample_group.net = ovs_dp_get_net(dp);
> > > + md.in_ifindex = OVS_CB(skb)->input_vport->dev->ifindex;
> > > + md.trunc_size = skb->len - OVS_CB(skb)->cutlen;
> > > +
> > > + psample_sample_packet(&psample_group, skb, 0, &md);
> > > +}
> > > +#else
> > > +static inline void execute_psample(struct datapath *dp, struct sk_buff
> > > *skb,
> > > +const struct nlattr *attr) {}
> >
> > I noticed that this got flagged in patchwork since it is 'static inline'
> > while being part of a complete translation unit - but I also see some
> > other places where that has been done. I guess it should be just
> > 'static' though. I don't feel very strongly about it.
> >
>
> We had a bit of discussion about this with Ilya. It seems "static
> inline" is a common pattern around the kernel. The coding style
> documentation says:
> "Generally, inline functions are preferable to macros resembling functions."
>
> So I think this "inline" is correct but I might be missing something.
Hi Adrián,
TL;DR: Please remove this inline keyword
For Kernel networking code at least it is strongly preferred not
to use inline in .c files unless there is a demonstrable - usually
performance - reason to do so. Rather, it is preferred to let the
compiler decide when to inline such functions. OTOH, the inline
keyword in .h files is fine.
...
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next v7 05/10] net: openvswitch: add psample action
On Tue, Jul 02, 2024 at 03:05:02AM GMT, Adrián Moreno wrote:
> On Mon, Jul 01, 2024 at 02:23:12PM GMT, Aaron Conole wrote:
> > Adrian Moreno writes:
> >
> > > Add support for a new action: psample.
> > >
> > > This action accepts a u32 group id and a variable-length cookie and uses
> > > the psample multicast group to make the packet available for
> > > observability.
> > >
> > > The maximum length of the user-defined cookie is set to 16, same as
> > > tc_cookie, to discourage using cookies that will not be offloadable.
> > >
> > > Acked-by: Eelco Chaudron
> > > Signed-off-by: Adrian Moreno
> > > ---
> >
> > Hi Adrian,
> >
> > Just some nits below.
> >
> > > Documentation/netlink/specs/ovs_flow.yaml | 17
> > > include/uapi/linux/openvswitch.h | 28 ++
> > > net/openvswitch/Kconfig | 1 +
> > > net/openvswitch/actions.c | 47 +++
> > > net/openvswitch/flow_netlink.c| 32 ++-
> > > 5 files changed, 124 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Documentation/netlink/specs/ovs_flow.yaml
> > > b/Documentation/netlink/specs/ovs_flow.yaml
> > > index 4fdfc6b5cae9..46f5d1cd8a5f 100644
> > > --- a/Documentation/netlink/specs/ovs_flow.yaml
> > > +++ b/Documentation/netlink/specs/ovs_flow.yaml
> > > @@ -727,6 +727,12 @@ attribute-sets:
> > > name: dec-ttl
> > > type: nest
> > > nested-attributes: dec-ttl-attrs
> > > + -
> > > +name: psample
> > > +type: nest
> > > +nested-attributes: psample-attrs
> > > +doc: |
> > > + Sends a packet sample to psample for external observation.
> > >-
> > > name: tunnel-key-attrs
> > > enum-name: ovs-tunnel-key-attr
> > > @@ -938,6 +944,17 @@ attribute-sets:
> > >-
> > > name: gbp
> > > type: u32
> > > + -
> > > +name: psample-attrs
> > > +enum-name: ovs-psample-attr
> > > +name-prefix: ovs-psample-attr-
> > > +attributes:
> > > + -
> > > +name: group
> > > +type: u32
> > > + -
> > > +name: cookie
> > > +type: binary
> > >
> > > operations:
> > >name-prefix: ovs-flow-cmd-
> > > diff --git a/include/uapi/linux/openvswitch.h
> > > b/include/uapi/linux/openvswitch.h
> > > index efc82c318fa2..3dd653748725 100644
> > > --- a/include/uapi/linux/openvswitch.h
> > > +++ b/include/uapi/linux/openvswitch.h
> > > @@ -914,6 +914,31 @@ struct check_pkt_len_arg {
> > > };
> > > #endif
> > >
> > > +#define OVS_PSAMPLE_COOKIE_MAX_SIZE 16
> > > +/**
> > > + * enum ovs_psample_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: */
> > > + __OVS_PSAMPLE_ATTR_MAX
> > > +};
> > > +
> > > +#define OVS_PSAMPLE_ATTR_MAX (__OVS_PSAMPLE_ATTR_MAX - 1)
> > > +
> > > /**
> > > * enum ovs_action_attr - Action types.
> > > *
> > > @@ -966,6 +991,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.
> > > *
> > > * Only a single header can be set with a single %OVS_ACTION_ATTR_SET.
> > > Not all
> > > * fields within a header are modifiable, e.g. the IPv4 protocol and
> > > fragment
> > > @@ -1004,6 +1031,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 error code. */
> > > + OVS_ACTION_ATTR_PSAMPLE, /* Nested OVS_PSAMPLE_ATTR_*. */
> > >
> > > __OVS_ACTION_ATTR_MAX,/* Nothing past this will be accepted
> > > * from userspace. */
> > > diff --git a/net/openvswitch/Kconfig b/net/openvswitch/Kconfig
> > > index 29a7081858cd..2535f3f9f462 100644
> > > --- a/net/openvswitch/Kconfig
> > > +++ b/net/openvswitch/Kconfig
> > > @@ -10,6 +10,7 @@ config OPENVSWITCH
> > > (NF_CONNTRACK && ((!NF_DEFRAG_IPV6 || NF_DEFRAG_IPV6) && \
> > >(!NF_NAT || NF_NAT) && \
>
Re: [ovs-dev] [PATCH net-next v7 10/10] selftests: openvswitch: add psample test
On Mon, Jul 01, 2024 at 02:38:44PM GMT, Aaron Conole wrote:
> Adrian Moreno writes:
>
> > Add a test to verify sampling packets via psample works.
> >
> > In order to do that, create a subcommand in ovs-dpctl.py to listen to
> > on the psample multicast group and print samples.
> >
> > Signed-off-by: Adrian Moreno
> > ---
> > .../selftests/net/openvswitch/openvswitch.sh | 115 +-
> > .../selftests/net/openvswitch/ovs-dpctl.py| 73 ++-
> > 2 files changed, 182 insertions(+), 6 deletions(-)
> >
> > diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh
> > b/tools/testing/selftests/net/openvswitch/openvswitch.sh
> > index 15bca0708717..02a366e01004 100755
> > --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
> > +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
> > @@ -20,7 +20,8 @@ tests="
> > nat_related_v4 ip4-nat-related: ICMP related
> > matches work with SNAT
> > netlink_checks ovsnl: validate netlink attrs
> > and settings
> > upcall_interfaces ovs: test the upcall interfaces
> > - drop_reason drop: test drop reasons are
> > emitted"
> > + drop_reason drop: test drop reasons are
> > emitted
> > + psample psample: Sampling packets with
> > psample"
> >
> > info() {
> > [ $VERBOSE = 0 ] || echo $*
> > @@ -102,12 +103,21 @@ ovs_netns_spawn_daemon() {
> > shift
> > netns=$1
> > shift
> > - info "spawning cmd: $*"
> > - ip netns exec $netns $* >> $ovs_dir/stdout 2>> $ovs_dir/stderr &
> > + if [ "$netns" == "_default" ]; then
> > + $* >> $ovs_dir/stdout 2>> $ovs_dir/stderr &
> > + else
> > + ip netns exec $netns $* >> $ovs_dir/stdout 2>>
> > $ovs_dir/stderr &
> > + fi
> > pid=$!
> > ovs_sbx "$sbx" on_exit "kill -TERM $pid 2>/dev/null"
> > }
> >
> > +ovs_spawn_daemon() {
> > + sbx=$1
> > + shift
> > + ovs_netns_spawn_daemon $sbx "_default" $*
> > +}
> > +
> > ovs_add_netns_and_veths () {
> > info "Adding netns attached: sbx:$1 dp:$2 {$3, $4, $5}"
> > ovs_sbx "$1" ip netns add "$3" || return 1
> > @@ -170,6 +180,19 @@ ovs_drop_reason_count()
> > return `echo "$perf_output" | grep "$pattern" | wc -l`
> > }
> >
> > +ovs_test_flow_fails () {
> > + ERR_MSG="Flow actions may not be safe on all matching packets"
> > +
> > + PRE_TEST=$(dmesg | grep -c "${ERR_MSG}")
> > + ovs_add_flow $@ &> /dev/null $@ && return 1
> > + POST_TEST=$(dmesg | grep -c "${ERR_MSG}")
> > +
> > + if [ "$PRE_TEST" == "$POST_TEST" ]; then
> > + return 1
> > + fi
> > + return 0
> > +}
> > +
> > usage() {
> > echo
> > echo "$0 [OPTIONS] [TEST]..."
> > @@ -184,6 +207,92 @@ usage() {
> > exit 1
> > }
> >
> > +
> > +# psample test
> > +# - use psample to observe packets
> > +test_psample() {
> > + sbx_add "test_psample" || return $?
> > +
> > + # Add a datapath with per-vport dispatching.
> > + ovs_add_dp "test_psample" psample -V 2:1 || return 1
> > +
> > + info "create namespaces"
> > + ovs_add_netns_and_veths "test_psample" "psample" \
> > + client c0 c1 172.31.110.10/24 -u || return 1
> > + ovs_add_netns_and_veths "test_psample" "psample" \
> > + server s0 s1 172.31.110.20/24 -u || return 1
> > +
> > + # Check if psample actions can be configured.
> > + ovs_add_flow "test_psample" psample \
> > + 'in_port(1),eth(),eth_type(0x0806),arp()' 'psample(group=1)'
>
> Might be good to redirect this stdout/stderr line to /dev/null -
> otherwise on an unsupported system there will be the following extra
> splat:
>
> Traceback (most recent call last):
> File
> "/home/aconole/git/linux/tools/testing/selftests/net/openvswitch/ovs-dpctl.py",
> line 2774, in
> sys.exit(main(sys.argv))
>...
> File "/usr/lib/python3.12/site-packages/pyroute2/netlink/nlsocket.py",
> line 489, in get
> raise msg['header']['error']
> pyroute2.netlink.exceptions.NetlinkError: (22, 'Invalid argument')
>
I thought knowing the return value was kind of useful but sure, we can
redirect it to /dev/null.
> > + if [ $? == 1 ]; then
> > + info "no support for psample - skipping"
> > + ovs_exit_sig
> > + return $ksft_skip
> > + fi
> > +
> > + ovs_del_flows "test_psample" psample
> > +
> > + # Test action verification.
> > + OLDIFS=$IFS
> > + IFS='*'
> > + min_key='in_port(1),eth(),eth_type(0x0800),ipv4()'
> > + for testcase in \
> > + "cookie to
> > large"*"psample(group=1,cookie=1615141312111009080706050403020100)" \
> > + "no group with cookie"*"psample(cookie=abcd)" \
> > + "no group"*"psample()";
> > + do
> > + set -- $testcase;
> > + ovs_test_flow_fails "test_psample" psample $min_key $2
> > + if [ $? == 1 ]; then
> > + in
Re: [ovs-dev] [PATCH net-next v7 05/10] net: openvswitch: add psample action
On Mon, Jul 01, 2024 at 02:23:12PM GMT, Aaron Conole wrote:
> Adrian Moreno writes:
>
> > Add support for a new action: psample.
> >
> > This action accepts a u32 group id and a variable-length cookie and uses
> > the psample multicast group to make the packet available for
> > observability.
> >
> > The maximum length of the user-defined cookie is set to 16, same as
> > tc_cookie, to discourage using cookies that will not be offloadable.
> >
> > Acked-by: Eelco Chaudron
> > Signed-off-by: Adrian Moreno
> > ---
>
> Hi Adrian,
>
> Just some nits below.
>
> > Documentation/netlink/specs/ovs_flow.yaml | 17
> > include/uapi/linux/openvswitch.h | 28 ++
> > net/openvswitch/Kconfig | 1 +
> > net/openvswitch/actions.c | 47 +++
> > net/openvswitch/flow_netlink.c| 32 ++-
> > 5 files changed, 124 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/netlink/specs/ovs_flow.yaml
> > b/Documentation/netlink/specs/ovs_flow.yaml
> > index 4fdfc6b5cae9..46f5d1cd8a5f 100644
> > --- a/Documentation/netlink/specs/ovs_flow.yaml
> > +++ b/Documentation/netlink/specs/ovs_flow.yaml
> > @@ -727,6 +727,12 @@ attribute-sets:
> > name: dec-ttl
> > type: nest
> > nested-attributes: dec-ttl-attrs
> > + -
> > +name: psample
> > +type: nest
> > +nested-attributes: psample-attrs
> > +doc: |
> > + Sends a packet sample to psample for external observation.
> >-
> > name: tunnel-key-attrs
> > enum-name: ovs-tunnel-key-attr
> > @@ -938,6 +944,17 @@ attribute-sets:
> >-
> > name: gbp
> > type: u32
> > + -
> > +name: psample-attrs
> > +enum-name: ovs-psample-attr
> > +name-prefix: ovs-psample-attr-
> > +attributes:
> > + -
> > +name: group
> > +type: u32
> > + -
> > +name: cookie
> > +type: binary
> >
> > operations:
> >name-prefix: ovs-flow-cmd-
> > diff --git a/include/uapi/linux/openvswitch.h
> > b/include/uapi/linux/openvswitch.h
> > index efc82c318fa2..3dd653748725 100644
> > --- a/include/uapi/linux/openvswitch.h
> > +++ b/include/uapi/linux/openvswitch.h
> > @@ -914,6 +914,31 @@ struct check_pkt_len_arg {
> > };
> > #endif
> >
> > +#define OVS_PSAMPLE_COOKIE_MAX_SIZE 16
> > +/**
> > + * enum ovs_psample_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: */
> > + __OVS_PSAMPLE_ATTR_MAX
> > +};
> > +
> > +#define OVS_PSAMPLE_ATTR_MAX (__OVS_PSAMPLE_ATTR_MAX - 1)
> > +
> > /**
> > * enum ovs_action_attr - Action types.
> > *
> > @@ -966,6 +991,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.
> > *
> > * Only a single header can be set with a single %OVS_ACTION_ATTR_SET.
> > Not all
> > * fields within a header are modifiable, e.g. the IPv4 protocol and
> > fragment
> > @@ -1004,6 +1031,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 error code. */
> > + OVS_ACTION_ATTR_PSAMPLE, /* Nested OVS_PSAMPLE_ATTR_*. */
> >
> > __OVS_ACTION_ATTR_MAX,/* Nothing past this will be accepted
> >* from userspace. */
> > diff --git a/net/openvswitch/Kconfig b/net/openvswitch/Kconfig
> > index 29a7081858cd..2535f3f9f462 100644
> > --- a/net/openvswitch/Kconfig
> > +++ b/net/openvswitch/Kconfig
> > @@ -10,6 +10,7 @@ config OPENVSWITCH
> >(NF_CONNTRACK && ((!NF_DEFRAG_IPV6 || NF_DEFRAG_IPV6) && \
> > (!NF_NAT || NF_NAT) && \
> > (!NETFILTER_CONNCOUNT ||
> > NETFILTER_CONNCOUNT)))
> > + depends on PSAMPLE || !PSAMPLE
> > select LIBCRC32C
> > select MPLS
> > select NET_MPLS_GSO
> > diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> > index 964225580824..a035
