Re: [ovs-dev] [PATCH v29 0/8] Add offload support for sFlow
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() action in the future, such as controller() action using the same psample infrastructure. But that will not be possible for the same reason. In addition to tunnel metadata we will also have to provide connection tracking information, which is even harder, because conntrack state is more dynamic and is only actually known to the datapath. psample can't supply this information to the userspace. Thoughts? Hi Ilya, Sorry for the late reply. I applied your following diff and reproduced the issue you found. This seems like a limitation we can't
[ovs-dev] [PATCH] ovsdb: raft: Don't forward more than one command to the leader.
Every transaction has RAFT log prerequisites. Even if transactions
are not related (because RAFT doesn't actually know what data it is
handling). When leader writes a new record to a RAFT storage, it is
getting appended to the log right away and changes current 'eid',
i.e., changes prerequisites. The leader will not try to write new
records until the current one is committed, because until then the
pre-check will be failing.
However, that is different for the follower. Followers do not add
records to the RAFT log until the leader sends an append request back.
So, if there are multiple transactions pending on a follower, it will
create a command for each of them and prerequisites will be set to the
same values. All these commands will be sent to the leader, but only
one can succeed at a time, because accepting one command immediately
changes prerequisites and all other commands become non-applicable.
So, out of N commands, 1 will succeed and N - 1 will fail. The cluster
failure is a transient failure, so the follower will re-process all the
failed transactions and send them again. 1 will succeed and N - 2 will
fail. And so on, until there are no more transactions. In the end,
instead of processing N transactions, the follower is performing
N * (N - 1) / 2 transaction processing iterations. That is consuming
a huge amount of CPU resources completely unnecessarily.
Since there is no real chance for multiple transactions from the same
follower to succeed, it's better to not send them in the first place.
This also eliminates prerequisite mismatch messages on a leader in
this particular case.
In a test with 30 parallel shell threads executing 12K transactions
total with separate ovsdb-client calls through the same follower there
is about 60% performance improvement. The test takes ~100 seconds to
complete without this change and ~40 seconds with this change applied.
The new time is very close to what it takes to execute the same test
through the cluster leader.
Note: prerequisite failures on a leader are still possible, but mostly
in a case of simultaneous transactions from different followers. It's
a normal thing for a distributed database due to its nature.
Signed-off-by: Ilya Maximets
---
ovsdb/raft.c| 45 -
ovsdb/raft.h| 2 +-
ovsdb/storage.c | 9 +
ovsdb/storage.h | 5 -
ovsdb/transaction.c | 6 +-
5 files changed, 55 insertions(+), 12 deletions(-)
diff --git a/ovsdb/raft.c b/ovsdb/raft.c
index ac3d37ac4..116924058 100644
--- a/ovsdb/raft.c
+++ b/ovsdb/raft.c
@@ -2307,12 +2307,55 @@ raft_get_eid(const struct raft *raft, uint64_t index)
return &raft->snap.eid;
}
-const struct uuid *
+static const struct uuid *
raft_current_eid(const struct raft *raft)
{
return raft_get_eid(raft, raft->log_end - 1);
}
+bool
+raft_precheck_prereq(const struct raft *raft, const struct uuid *prereq)
+{
+if (!uuid_equals(raft_current_eid(raft), prereq)) {
+VLOG_DBG("%s: prerequisites (" UUID_FMT ") "
+ "do not match current eid (" UUID_FMT ")",
+ __func__, UUID_ARGS(prereq),
+ UUID_ARGS(raft_current_eid(raft)));
+return false;
+}
+
+/* Having incomplete commands on a follower means that the leader has
+ * these commands and they will change the prerequisites once added to
+ * the leader's log.
+ *
+ * There is a chance that all these commands will actually fail and the
+ * record with current prerequisites will in fact succeed, but, since
+ * these are our own commands, the chances are low.
+ *
+ * Incomplete commands on a leader will not change the leader's current
+ * 'eid' on commit as they are already part of the leader's log. */
+if (raft->role != RAFT_LEADER && hmap_count(&raft->commands)) {
+struct raft_command *cmd;
+
+HMAP_FOR_EACH (cmd, hmap_node, &raft->commands) {
+/* Skip commands that are already part of the log (have non-zero
+ * index) and ones that do not carry any data (have zero 'eid'),
+ * as they can't change prerequisites.
+ *
+ * Database will not re-run triggers unless the data changes or
+ * one of the data-carrying triggers completes. So, pre-check must
+ * not fail if there are no outstanding data-carrying commands. */
+if (!cmd->index && !uuid_is_zero(&cmd->eid)) {
+VLOG_DBG("%s: follower still has an incomplete command "
+ UUID_FMT, __func__, UUID_ARGS(&cmd->eid));
+return false;
+}
+}
+}
+
+return true;
+}
+
static struct raft_command *
raft_command_create_completed(enum raft_command_status status)
{
diff --git a/ovsdb/raft.h b/ovsdb/raft.h
index a5b55d9bf..5833aaf23 100644
--- a/ovsdb/raft.h
+++ b/ovsdb/raft.h
@@ -189,5 +189,5 @@ struct ovsdb_error *raft_store_snaps
Re: [ovs-dev] [PATCH net-next v5 05/10] net: openvswitch: add emit_sample action
On Wed, Jun 26, 2024 at 04:28:17PM GMT, Eelco Chaudron wrote:
>
>
> On 25 Jun 2024, at 22:51, Adrian Moreno wrote:
>
> > Add support for a new action: emit_sample.
> >
> > 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.
>
> I’ll add the same comment as I had in the user space part, and that
> is that I feel from an OVS perspective this action should be called
> emit_local() instead of emit_sample() to make it Datapath independent.
> Or quoting the earlier comment:
>
>
> “I’ll start the discussion again on the naming. The name "emit_sample()"
> does not seem appropriate. This function's primary role is to copy the
> packet and send it to a local collector, which varies depending on the
> datapath. For the kernel datapath, this collector is psample, while for
> userspace, it will likely be some kind of probe. This action is distinct
> from the sample() action by design; it is a standalone action that can
> be combined with others.
>
> Furthermore, the action itself does not involve taking a sample; it
> consistently pushes the packet to the local collector. Therefore, I
> suggest renaming "emit_sample()" to "emit_local()". This same goes for
> all the derivative ATTR naming.”
>
This is a blurry semantic area.
IMO, "sample" is the act of extracting (potentially a piece of)
someting, in this case, a packet. It is common to only take some packets
as samples, so this action usually comes with some kind of "rate", but
even if the rate is 1, it's still sampling in this context.
OTOH, OVS kernel design tries to be super-modular and define small
combinable actions, so the rate or probability generation is done with
another action which is (IMHO unfortunately) named "sample".
With that interpretation of the term it would actually make more sense
to rename "sample" to something like "random" (of course I'm not
suggestion we do it). "sample" without any nested action that actually
sends the packet somewhere is not sampling, it's just doing something or
not based on a probability. Where as "emit_sample" is sampling even if
it's not nested inside a "sample".
Having said that, I don't have a super strong favor for "emit_sample". I'm
OK with "emit_local" or "emit_packet" or even just "emit".
I don't think any term will fully satisfy everyone so I hope we can find
a reasonable compromise.
> > 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 | 45 +++
> > net/openvswitch/flow_netlink.c| 33 -
> > 5 files changed, 123 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/netlink/specs/ovs_flow.yaml
> > b/Documentation/netlink/specs/ovs_flow.yaml
> > index 4fdfc6b5cae9..a7ab5593a24f 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: emit-sample
> > +type: nest
> > +nested-attributes: emit-sample-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: emit-sample-attrs
> > +enum-name: ovs-emit-sample-attr
> > +name-prefix: ovs-emit-sample-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..8cfa1b3f6b06 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_EMIT_SAMPLE_COOKIE_MAX_SIZE 16
> > +/**
> > + * enum ovs_emit_sample_attr - Attributes for %OVS_ACTION_ATTR_EMIT_SAMPLE
> > + * action.
> > + *
> > + * @OVS_EMIT_SAMPLE_ATTR_GROUP: 32-bit number to identify the source of the
> > + * sample.
> > + * @OVS_EMIT_SAMPLE_ATTR_COOKIE: A variable-length binary cookie that
> > contains
> > + * user-defined metadata. The maximum length is
> > OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE
> > + * bytes.
> > + *
> > + * Sends the packet to the psample multicast group with the specified
> > group and
> > + * cookie. It is possible
Re: [ovs-dev] [PATCH net-next v5 05/10] net: openvswitch: add emit_sample action
On Wed, Jun 26, 2024 at 12:51:19PM GMT, Ilya Maximets wrote:
> On 6/25/24 22:51, Adrian Moreno wrote:
> > Add support for a new action: emit_sample.
> >
> > 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.
> >
> > 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 | 45 +++
> > net/openvswitch/flow_netlink.c| 33 -
> > 5 files changed, 123 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/netlink/specs/ovs_flow.yaml
> > b/Documentation/netlink/specs/ovs_flow.yaml
> > index 4fdfc6b5cae9..a7ab5593a24f 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: emit-sample
> > +type: nest
> > +nested-attributes: emit-sample-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: emit-sample-attrs
> > +enum-name: ovs-emit-sample-attr
> > +name-prefix: ovs-emit-sample-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..8cfa1b3f6b06 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_EMIT_SAMPLE_COOKIE_MAX_SIZE 16
> > +/**
> > + * enum ovs_emit_sample_attr - Attributes for %OVS_ACTION_ATTR_EMIT_SAMPLE
> > + * action.
> > + *
> > + * @OVS_EMIT_SAMPLE_ATTR_GROUP: 32-bit number to identify the source of the
> > + * sample.
> > + * @OVS_EMIT_SAMPLE_ATTR_COOKIE: A variable-length binary cookie that
> > contains
> > + * user-defined metadata. The maximum length is
> > OVS_EMIT_SAMPLE_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 packet being
> > emitted.
> > + */
> > +enum ovs_emit_sample_attr {
> > + OVS_EMIT_SAMPLE_ATTR_GROUP = 1, /* u32 number. */
> > + OVS_EMIT_SAMPLE_ATTR_COOKIE,/* Optional, user specified cookie. */
> > +
> > + /* private: */
> > + __OVS_EMIT_SAMPLE_ATTR_MAX
> > +};
> > +
> > +#define OVS_EMIT_SAMPLE_ATTR_MAX (__OVS_EMIT_SAMPLE_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_EMIT_SAMPLE: 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_EMIT_SAMPLE, /* Nested OVS_EMIT_SAMPLE_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
Re: [ovs-dev] [PATCH net-next v5 02/10] net: sched: act_sample: add action cookie to sample
On Wed, Jun 26, 2024 at 04:28:01PM GMT, Eelco Chaudron wrote:
>
>
> On 25 Jun 2024, at 22:51, Adrian Moreno wrote:
>
> > If the action has a user_cookie, pass it along to the sample so it can
> > be easily identified.
> >
> > 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);
>
> Maybe I’m over paranoid, but can we assume user_cookie->len, will not be
> larger than TC_COOKIE_MAX_SIZE?
> Or should we do something like min(user_cookie->len, sizeof(cookie_data))
>
I think it's good to be paranoid with this kind of things. I do,
however, think it should be safe to use. The cookie is extracted from
the netlink attribute directly and its length is verified with the
nla_policy [1]. So nothing that comes into the kernel should be larger
than TC_COOKIE_MAX_SIZE.
I guess if there is some previous bug that allows for the size to get
corrupted, then this might happen but doing those kind of checks in the
fast path seems a bit excessive. For example, Ilya argued in v2 [2] that
we should avoid zeroing "u8 cookie_data[TC_COOKIE_MAX_SIZE]" to safe the
unneeded cycles.
[1]
https://github.com/torvalds/linux/blob/55027e689933ba2e64f3d245fb1ff185b3e7fc81/net/sched/act_api.c#L1299
[2]
https://patchwork.kernel.org/project/netdevbpf/patch/20240603185647.2310748-3-amore...@redhat.com/
Thanks.
Adrián
> > + 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.1
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4 ovn 1/3] northd: Introduce ECMP_Nexthop table in SB db.
Bleep bloop. Greetings Lorenzo Bianconi, 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 (northd/northd.c). error: could not build fake ancestor hint: Use 'git am --show-current-patch=diff' to see the failed patch Patch failed at 0001 northd: Introduce ECMP_Nexthop table in SB db. When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next v3 4/7] selftests: openvswitch: Add support for tunnel() key.
On Tue, Jun 25, 2024 at 01:22:42PM -0400, Aaron Conole wrote: > This will be used when setting details about the tunnel to use as > transport. There is a difference between the ODP format between tunnel(): > the 'key' flag is not actually a flag field, so we don't support it in the > same way that the vswitchd userspace supports displaying it. > > Signed-off-by: Aaron Conole Reviewed-by: Simon Horman Tested-by: Simon Horman ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next v3 6/7] selftests: net: Use the provided dpctl rather than the vswitchd for tests.
On Tue, Jun 25, 2024 at 01:22:44PM -0400, Aaron Conole wrote: > The current pmtu test infrastucture requires an installed copy of the > ovs-vswitchd userspace. This means that any automated or constrained > environments may not have the requisite tools to run the tests. However, > the pmtu tests don't require any special classifier processing. Indeed > they are only using the vswitchd in the most basic mode - as a NORMAL > switch. > > However, the ovs-dpctl kernel utility can now program all the needed basic > flows to allow traffic to traverse the tunnels and provide support for at > least testing some basic pmtu scenarios. More complicated flow pipelines > can be added to the internal ovs test infrastructure, but that is work for > the future. For now, enable the most common cases - wide mega flows with > no other prerequisites. > > Enhance the pmtu testing to try testing using the internal utility, first. > As a fallback, if the internal utility isn't running, then try with the > ovs-vswitchd userspace tools. > > Additionally, make sure that when the pyroute2 package is not available > the ovs-dpctl utility will error out to properly signal an error has > occurred and skip using the internal utility. Hi Aaron, I don't feel strongly about this, but it does feel like the change to ovs-dpctl.py could live in a separate patch. > Reviewed-by: Stefano Brivio > Signed-off-by: Aaron Conole The above not withstanding, Reviewed-by: Simon Horman I have tested pmtu.sh with this change on Fedora 40 both with python3-pyroute2 installed, which uses ovs-dpctl, and without, which uses ovs-vswitchd userspace tools. Tested-by: Simon Horman ... ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v4 ovn 2/3] northd: Add nexhop id in ct_label.label.
Introduce the nexthop identifier in the ct_label.label field for
ecmp-symmetric replies connections. This field will be used by
ovn-controller to track ct entries and to flush them if requested by the
CMS (e.g. removing the related static routes).
Signed-off-by: Lorenzo Bianconi
---
northd/en-lflow.c| 3 +++
northd/inc-proc-northd.c | 1 +
northd/northd.c | 34 ---
northd/northd.h | 1 +
tests/ovn.at | 4 +--
tests/system-ovn.at | 58 +++-
6 files changed, 65 insertions(+), 36 deletions(-)
diff --git a/northd/en-lflow.c b/northd/en-lflow.c
index 3dba5034b..b4df49076 100644
--- a/northd/en-lflow.c
+++ b/northd/en-lflow.c
@@ -54,6 +54,8 @@ lflow_get_input_data(struct engine_node *node,
engine_get_input_data("lr_stateful", node);
struct ed_type_ls_stateful *ls_stateful_data =
engine_get_input_data("ls_stateful", node);
+struct ecmp_nexthop_data *nexthop_data =
+engine_get_input_data("ecmp_nexthop", node);
lflow_input->sbrec_logical_flow_table =
EN_OVSDB_GET(engine_get_input("SB_logical_flow", node));
@@ -83,6 +85,7 @@ lflow_get_input_data(struct engine_node *node,
lflow_input->parsed_routes = &static_routes_data->parsed_routes;
lflow_input->route_tables = &static_routes_data->route_tables;
lflow_input->route_policies = &route_policies_data->route_policies;
+lflow_input->nexthops_table = &nexthop_data->nexthops;
struct ed_type_global_config *global_config =
engine_get_input_data("global_config", node);
diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
index c4e5b9bf6..3d4bfa175 100644
--- a/northd/inc-proc-northd.c
+++ b/northd/inc-proc-northd.c
@@ -281,6 +281,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
engine_add_input(&en_lflow, &en_route_policies, NULL);
engine_add_input(&en_lflow, &en_static_routes, NULL);
engine_add_input(&en_lflow, &en_bfd, NULL);
+engine_add_input(&en_lflow, &en_ecmp_nexthop, NULL);
engine_add_input(&en_lflow, &en_northd, lflow_northd_handler);
engine_add_input(&en_lflow, &en_port_group, lflow_port_group_handler);
engine_add_input(&en_lflow, &en_lr_stateful, lflow_lr_stateful_handler);
diff --git a/northd/northd.c b/northd/northd.c
index 861f979c3..f6f0568f6 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -10849,7 +10849,8 @@ add_ecmp_symmetric_reply_flows(struct lflow_table
*lflows,
struct ovn_port *out_port,
const struct parsed_route *route,
struct ds *route_match,
- struct lflow_ref *lflow_ref)
+ struct lflow_ref *lflow_ref,
+ struct simap *nexthops_table)
{
const struct nbrec_logical_router_static_route *st_route = route->route;
struct ds match = DS_EMPTY_INITIALIZER;
@@ -10887,9 +10888,15 @@ add_ecmp_symmetric_reply_flows(struct lflow_table
*lflows,
ds_put_cstr(&match, " && !ct.rpl && (ct.new || ct.est)");
ds_put_format(&actions,
"ct_commit { ct_label.ecmp_reply_eth = eth.src; "
-" %s = %" PRId64 ";}; "
-"next;",
+" %s = %" PRId64 ";",
ct_ecmp_reply_port_match, out_port->sb->tunnel_key);
+
+struct simap_node *n = simap_find(nexthops_table, st_route->nexthop);
+if (n) {
+ds_put_format(&actions, " ct_label.label = %d;", n->data);
+}
+ds_put_cstr(&actions, " }; next;");
+
ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_ECMP_STATEFUL, 100,
ds_cstr(&match), ds_cstr(&actions),
&st_route->header_,
@@ -10947,7 +10954,8 @@ static void
build_ecmp_route_flow(struct lflow_table *lflows, struct ovn_datapath *od,
bool ct_masked_mark, const struct hmap *lr_ports,
struct ecmp_groups_node *eg,
- struct lflow_ref *lflow_ref)
+ struct lflow_ref *lflow_ref,
+ struct simap *nexthops_table)
{
bool is_ipv4 = IN6_IS_ADDR_V4MAPPED(&eg->prefix);
@@ -11005,7 +11013,7 @@ build_ecmp_route_flow(struct lflow_table *lflows,
struct ovn_datapath *od,
add_ecmp_symmetric_reply_flows(lflows, od, ct_masked_mark,
lrp_addr_s, out_port,
route_, &route_match,
- lflow_ref);
+ lflow_ref, nexthops_table);
}
ds_clear(&match);
ds_put_format(&match, REG_ECMP_GROUP_ID" == %"PRIu16" && "
@@ -12899,7 +12907,8 @@ build_static_route_flows_for_lrouter(
struct lflow_table *lflows, const struct hmap *lr_ports,
struct hmap *parsed_routes,
struct simap *route_tables,
-str
[ovs-dev] [PATCH v4 ovn 1/3] northd: Introduce ECMP_Nexthop table in SB db.
Introduce ECMP_Nexthop table in the SB db in order to track active
ecmp-symmetric-reply connections and flush stale ones.
Signed-off-by: Lorenzo Bianconi
---
northd/en-northd.c | 33 +++
northd/en-northd.h | 4 ++
northd/inc-proc-northd.c | 7 +++-
northd/northd.c | 89
northd/northd.h | 10 +
ovn-sb.ovsschema | 18 +++-
ovn-sb.xml | 31 ++
tests/ovn-northd.at | 4 ++
8 files changed, 193 insertions(+), 3 deletions(-)
diff --git a/northd/en-northd.c b/northd/en-northd.c
index a4de71ba1..a2823ab2b 100644
--- a/northd/en-northd.c
+++ b/northd/en-northd.c
@@ -380,6 +380,23 @@ en_bfd_run(struct engine_node *node, void *data)
engine_set_node_state(node, EN_UPDATED);
}
+void
+en_ecmp_nexthop_run(struct engine_node *node, void *data)
+{
+const struct engine_context *eng_ctx = engine_get_context();
+struct static_routes_data *static_routes_data =
+engine_get_input_data("static_routes", node);
+struct ecmp_nexthop_data *enh_data = data;
+const struct sbrec_ecmp_nexthop_table *sbrec_ecmp_nexthop_table =
+EN_OVSDB_GET(engine_get_input("SB_ecmp_nexthop", node));
+
+build_ecmp_nexthop_table(eng_ctx->ovnsb_idl_txn,
+ &static_routes_data->parsed_routes,
+ &enh_data->nexthops,
+ sbrec_ecmp_nexthop_table);
+engine_set_node_state(node, EN_UPDATED);
+}
+
void
*en_northd_init(struct engine_node *node OVS_UNUSED,
struct engine_arg *arg OVS_UNUSED)
@@ -421,6 +438,16 @@ void
return data;
}
+void
+*en_ecmp_nexthop_init(struct engine_node *node OVS_UNUSED,
+ struct engine_arg *arg OVS_UNUSED)
+{
+struct ecmp_nexthop_data *data = xzalloc(sizeof *data);
+
+ecmp_nexthop_init(data);
+return data;
+}
+
void
en_northd_cleanup(void *data)
{
@@ -451,3 +478,9 @@ en_bfd_cleanup(void *data)
{
bfd_destroy(data);
}
+
+void
+en_ecmp_nexthop_cleanup(void *data)
+{
+ecmp_nexthop_destroy(data);
+}
diff --git a/northd/en-northd.h b/northd/en-northd.h
index 424209c2f..c6d520f71 100644
--- a/northd/en-northd.h
+++ b/northd/en-northd.h
@@ -34,5 +34,9 @@ void *en_bfd_init(struct engine_node *node OVS_UNUSED,
void en_bfd_cleanup(void *data);
bool bfd_change_handler(struct engine_node *node, void *data);
void en_bfd_run(struct engine_node *node, void *data);
+void en_ecmp_nexthop_run(struct engine_node *node, void *data);
+void *en_ecmp_nexthop_init(struct engine_node *node OVS_UNUSED,
+ struct engine_arg *arg OVS_UNUSED);
+void en_ecmp_nexthop_cleanup(void *data);
#endif /* EN_NORTHD_H */
diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
index d907da14d..c4e5b9bf6 100644
--- a/northd/inc-proc-northd.c
+++ b/northd/inc-proc-northd.c
@@ -103,7 +103,8 @@ static unixctl_cb_func chassis_features_list;
SB_NODE(fdb, "fdb") \
SB_NODE(static_mac_binding, "static_mac_binding") \
SB_NODE(chassis_template_var, "chassis_template_var") \
-SB_NODE(logical_dp_group, "logical_dp_group")
+SB_NODE(logical_dp_group, "logical_dp_group") \
+SB_NODE(ecmp_nexthop, "ecmp_nexthop")
enum sb_engine_node {
#define SB_NODE(NAME, NAME_STR) SB_##NAME,
@@ -160,6 +161,7 @@ static ENGINE_NODE_WITH_CLEAR_TRACK_DATA(ls_stateful,
"ls_stateful");
static ENGINE_NODE(route_policies, "route_policies");
static ENGINE_NODE(static_routes, "static_routes");
static ENGINE_NODE(bfd, "bfd");
+static ENGINE_NODE(ecmp_nexthop, "ecmp_nexthop");
void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
struct ovsdb_idl_loop *sb)
@@ -261,6 +263,9 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
engine_add_input(&en_static_routes,
&en_nb_logical_router_static_route, NULL);
+engine_add_input(&en_ecmp_nexthop, &en_sb_ecmp_nexthop, NULL);
+engine_add_input(&en_ecmp_nexthop, &en_static_routes, NULL);
+
engine_add_input(&en_sync_meters, &en_nb_acl, NULL);
engine_add_input(&en_sync_meters, &en_nb_meter, NULL);
engine_add_input(&en_sync_meters, &en_sb_meter, NULL);
diff --git a/northd/northd.c b/northd/northd.c
index 7906ca61c..861f979c3 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -10007,6 +10007,83 @@ build_bfd_table(
return ret;
}
+#define NEXTHOP_IDS_LEN65535
+bool
+build_ecmp_nexthop_table(
+struct ovsdb_idl_txn *ovnsb_txn,
+struct hmap *routes,
+struct simap *nexthops,
+const struct sbrec_ecmp_nexthop_table *sbrec_ecmp_nexthop_table)
+{
+unsigned long *nexthop_ids = bitmap_allocate(NEXTHOP_IDS_LEN);
+bool ret = false;
+
+if (!ovnsb_txn) {
+return false;
+}
+
+const struct sbrec_ecmp_nexthop *sb_ecmp_nexthop;
+struct simap_node *n;
+SIMAP_FOR_EACH (n, nexthops) {
+bitmap_set1(nexthop_ids, n->data);
+}
+
+
[ovs-dev] [PATCH v4 ovn 3/3] ofctrl: Introduce ecmp_nexthop_monitor.
Introduce ecmp_nexthop_monitor in ovn-controller in order to track and
flush ecmp-symmetric reply ct entires when requested by the CMS (e.g
removing the related static routes).
Signed-off-by: Lorenzo Bianconi
---
controller/ofctrl.c | 54 +
controller/ofctrl.h | 2 ++
controller/ovn-controller.c | 2 ++
tests/system-ovn.at | 8 ++
4 files changed, 66 insertions(+)
diff --git a/controller/ofctrl.c b/controller/ofctrl.c
index 9d181a782..2c891c049 100644
--- a/controller/ofctrl.c
+++ b/controller/ofctrl.c
@@ -388,9 +388,17 @@ struct meter_band_entry {
static struct shash meter_bands;
+#define ECMP_NEXTHOP_IDS_LEN65535
+static unsigned long *ecmp_nexthop_ids;
+
static void ofctrl_meter_bands_destroy(void);
static void ofctrl_meter_bands_clear(void);
+static void ecmp_nexthop_monitor_run(
+const struct sbrec_ecmp_nexthop_table *enh_table,
+struct ovs_list *msgs);
+
+
/* MFF_* field ID for our Geneve option. In S_TLV_TABLE_MOD_SENT, this is
* the option we requested (we don't know whether we obtained it yet). In
* S_CLEAR_FLOWS or S_UPDATE_FLOWS, this is really the option we have. */
@@ -429,6 +437,7 @@ ofctrl_init(struct ovn_extend_table *group_table,
groups = group_table;
meters = meter_table;
shash_init(&meter_bands);
+ecmp_nexthop_ids = bitmap_allocate(ECMP_NEXTHOP_IDS_LEN);
}
/* S_NEW, for a new connection.
@@ -876,6 +885,7 @@ ofctrl_destroy(void)
expr_symtab_destroy(&symtab);
shash_destroy(&symtab);
ofctrl_meter_bands_destroy();
+bitmap_free(ecmp_nexthop_ids);
}
uint64_t
@@ -2305,6 +2315,47 @@ add_meter(struct ovn_extend_table_info *m_desired,
ofctrl_meter_bands_alloc(sb_meter, m_desired, msgs);
}
+static void
+ecmp_nexthop_monitor_flush_ct_entry(uint64_t id, struct ovs_list *msgs)
+{
+ovs_u128 mask = {
+/* ct_labels.label BITS[96-127] */
+.u64.hi = 0x,
+};
+ovs_u128 nexthop = {
+.u64.hi = id << 32,
+};
+struct ofp_ct_match match = {
+.labels = nexthop,
+.labels_mask = mask,
+};
+struct ofpbuf *msg = ofp_ct_match_encode(&match, NULL,
+ rconn_get_version(swconn));
+ovs_list_push_back(msgs, &msg->list_node);
+}
+
+static void
+ecmp_nexthop_monitor_run(const struct sbrec_ecmp_nexthop_table *enh_table,
+ struct ovs_list *msgs)
+{
+unsigned long *ids = bitmap_allocate(ECMP_NEXTHOP_IDS_LEN);
+
+const struct sbrec_ecmp_nexthop *sbrec_ecmp_nexthop;
+SBREC_ECMP_NEXTHOP_TABLE_FOR_EACH (sbrec_ecmp_nexthop, enh_table) {
+bitmap_set1(ids, sbrec_ecmp_nexthop->id);
+}
+
+int id;
+BITMAP_FOR_EACH_1 (id, ECMP_NEXTHOP_IDS_LEN, ecmp_nexthop_ids) {
+if (!bitmap_is_set(ids, id)) {
+ecmp_nexthop_monitor_flush_ct_entry(id, msgs);
+}
+}
+
+bitmap_free(ecmp_nexthop_ids);
+ecmp_nexthop_ids = ids;
+}
+
static void
installed_flow_add(struct ovn_flow *d,
struct ofputil_bundle_ctrl_msg *bc,
@@ -2663,6 +2714,7 @@ ofctrl_put(struct ovn_desired_flow_table *lflow_table,
struct shash *pending_ct_zones,
struct hmap *pending_lb_tuples,
struct ovsdb_idl_index *sbrec_meter_by_name,
+ const struct sbrec_ecmp_nexthop_table *enh_table,
uint64_t req_cfg,
bool lflows_changed,
bool pflows_changed)
@@ -2703,6 +2755,8 @@ ofctrl_put(struct ovn_desired_flow_table *lflow_table,
/* OpenFlow messages to send to the switch to bring it up-to-date. */
struct ovs_list msgs = OVS_LIST_INITIALIZER(&msgs);
+ecmp_nexthop_monitor_run(enh_table, &msgs);
+
/* Iterate through ct zones that need to be flushed. */
struct shash_node *iter;
SHASH_FOR_EACH(iter, pending_ct_zones) {
diff --git a/controller/ofctrl.h b/controller/ofctrl.h
index 129e3b6ad..33953a8a4 100644
--- a/controller/ofctrl.h
+++ b/controller/ofctrl.h
@@ -31,6 +31,7 @@ struct ofpbuf;
struct ovsrec_bridge;
struct ovsrec_open_vswitch_table;
struct sbrec_meter_table;
+struct sbrec_ecmp_nexthop_table;
struct shash;
struct ovn_desired_flow_table {
@@ -59,6 +60,7 @@ void ofctrl_put(struct ovn_desired_flow_table *lflow_table,
struct shash *pending_ct_zones,
struct hmap *pending_lb_tuples,
struct ovsdb_idl_index *sbrec_meter_by_name,
+const struct sbrec_ecmp_nexthop_table *enh_table,
uint64_t nb_cfg,
bool lflow_changed,
bool pflow_changed);
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index ed95818a0..5c74f8bbf 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -6077,6 +6077,8 @@ main(int argc, char *argv[])
&ct_zones_data->pending,
&lb_data->removed_tu
[ovs-dev] [PATCH v4 ovn 0/3] Introduce ECMP_nexthop monitor in ovn-controller
Reported-at: https://issues.redhat.com/browse/FDP-56 Changes since v3: - rebase on top of https://patchwork.ozlabs.org/project/ovn/list/?series=409660 - use bitmap for id tracking in ofctrl.c - use simap for id tracking in northd.c - cosmetics Changes since v2: - rebase on top of https://patchwork.ozlabs.org/project/ovn/list/?series=409660 in order to use new bfd and static_routes maps - add IP northd node for ecmp_nexthop processing Changes since v1: - add ID column in ECMP_Nexthop table in SB db - remove nexthop-id in logical_router_static_route option column Lorenzo Bianconi (3): northd: Introduce ECMP_Nexthop table in SB db. northd: Add nexhop id in ct_label.label. ofctrl: Introduce ecmp_nexthop_monitor. controller/ofctrl.c | 54 controller/ofctrl.h | 2 + controller/ovn-controller.c | 2 + northd/en-lflow.c | 3 + northd/en-northd.c | 33 ++ northd/en-northd.h | 4 ++ northd/inc-proc-northd.c| 8 ++- northd/northd.c | 123 +--- northd/northd.h | 11 ovn-sb.ovsschema| 18 +- ovn-sb.xml | 31 + tests/ovn-northd.at | 4 ++ tests/ovn.at| 4 +- tests/system-ovn.at | 66 --- 14 files changed, 324 insertions(+), 39 deletions(-) -- 2.45.2 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next v5 08/10] selftests: openvswitch: add userspace parsing
Adrian Moreno writes: > 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. > > 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 v5 07/10] selftests: openvswitch: add emit_sample action
Adrian Moreno writes: > Add sample and emit_sample action support to ovs-dpctl.py. > > Refactor common attribute parsing logic into an external function. > > 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 v5 06/10] net: openvswitch: store sampling probability in cb.
On 25 Jun 2024, at 22:51, Adrian Moreno wrote: > 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 emit_sample to pass it down to psample. > > Signed-off-by: Adrian Moreno Acked-by: Eelco Chaudron ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next v5 05/10] net: openvswitch: add emit_sample action
On 25 Jun 2024, at 22:51, Adrian Moreno wrote:
> Add support for a new action: emit_sample.
>
> 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.
I’ll add the same comment as I had in the user space part, and that
is that I feel from an OVS perspective this action should be called
emit_local() instead of emit_sample() to make it Datapath independent.
Or quoting the earlier comment:
“I’ll start the discussion again on the naming. The name "emit_sample()"
does not seem appropriate. This function's primary role is to copy the
packet and send it to a local collector, which varies depending on the
datapath. For the kernel datapath, this collector is psample, while for
userspace, it will likely be some kind of probe. This action is distinct
from the sample() action by design; it is a standalone action that can
be combined with others.
Furthermore, the action itself does not involve taking a sample; it
consistently pushes the packet to the local collector. Therefore, I
suggest renaming "emit_sample()" to "emit_local()". This same goes for
all the derivative ATTR naming.”
> 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 | 45 +++
> net/openvswitch/flow_netlink.c| 33 -
> 5 files changed, 123 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/netlink/specs/ovs_flow.yaml
> b/Documentation/netlink/specs/ovs_flow.yaml
> index 4fdfc6b5cae9..a7ab5593a24f 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: emit-sample
> +type: nest
> +nested-attributes: emit-sample-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: emit-sample-attrs
> +enum-name: ovs-emit-sample-attr
> +name-prefix: ovs-emit-sample-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..8cfa1b3f6b06 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_EMIT_SAMPLE_COOKIE_MAX_SIZE 16
> +/**
> + * enum ovs_emit_sample_attr - Attributes for %OVS_ACTION_ATTR_EMIT_SAMPLE
> + * action.
> + *
> + * @OVS_EMIT_SAMPLE_ATTR_GROUP: 32-bit number to identify the source of the
> + * sample.
> + * @OVS_EMIT_SAMPLE_ATTR_COOKIE: A variable-length binary cookie that
> contains
> + * user-defined metadata. The maximum length is
> OVS_EMIT_SAMPLE_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 packet being
> emitted.
Although this include file is kernel-related, it will probably be re-used for
other datapaths, so should we be more general here?
> + */
> +enum ovs_emit_sample_attr {
> + OVS_EMIT_SAMPLE_ATTR_GROUP = 1, /* u32 number. */
> + OVS_EMIT_SAMPLE_ATTR_COOKIE,/* Optional, user specified cookie. */
As we start a new set of attributes maybe it would be good starting it off in
alphabetical order?
> +
> + /* private: */
> + __OVS_EMIT_SAMPLE_ATTR_MAX
> +};
> +
> +#define OVS_EMIT_SAMPLE_ATTR_MAX (__OVS_EMIT_SAMPLE_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_EMIT_SAMPLE: 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_*.
Re: [ovs-dev] [PATCH net-next v5 04/10] net: psample: allow using rate as probability
On 25 Jun 2024, at 22:51, Adrian Moreno wrote: > 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. > > Signed-off-by: Adrian Moreno Acked-by: Eelco Chaudron ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next v5 03/10] net: psample: skip packet copy if no listeners
On 25 Jun 2024, at 22:51, Adrian Moreno wrote: > 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: Simon Horman > Signed-off-by: Adrian Moreno Acked-by: Eelco Chaudron ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next v5 02/10] net: sched: act_sample: add action cookie to sample
On 25 Jun 2024, at 22:51, Adrian Moreno wrote:
> If the action has a user_cookie, pass it along to the sample so it can
> be easily identified.
>
> 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);
Maybe I’m over paranoid, but can we assume user_cookie->len, will not be larger
than TC_COOKIE_MAX_SIZE?
Or should we do something like min(user_cookie->len, sizeof(cookie_data))
> + 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.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next v5 00/10] net: openvswitch: Add sample multicasting.
On 25 Jun 2024, at 22:51, Adrian Moreno wrote: > ** 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_EMIT_SAMPLE) is created. > It accepts a group and an optional cookie and uses psample to > multicast the packet and the metadata. Hi Adrian, I reviewed the first part of this series to ensure it aligns with the userspace integration. I skipped the selftest patches, assuming Aaron is handling/reviewing those. Thanks, Eelco ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next v5 01/10] net: psample: add user cookie
On 25 Jun 2024, at 22:51, Adrian Moreno wrote: > 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: Simon Horman > Signed-off-by: Adrian Moreno Acked-by: Eelco Chaudron ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next v5 09/10] selftests: openvswitch: parse trunc action
Adrian Moreno writes: > The trunc action was supported decode-able but not parse-able. Add > support for parsing the action string. > > 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 ovn 2/4] northd: Fix potential overflow
Bleep bloop. Greetings Ales Musil, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: The subject summary should end with a dot. Subject: northd: Fix potential overflow Lines checked: 32, Warnings: 1, Errors: 0 Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn 3/4] ovn-nbctl: Avoid uninitialized value for mirror index.
The local mirror doesn't have an index, avoid storing
uninitialized value in that case. Also prevent from index
printing for local mirrors.
Signed-off-by: Ales Musil
---
tests/ovn-nbctl.at| 9 +
utilities/ovn-nbctl.c | 8 +---
2 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
index 19c83a4a5..31de30921 100644
--- a/tests/ovn-nbctl.at
+++ b/tests/ovn-nbctl.at
@@ -452,6 +452,7 @@ OVN_NBCTL_TEST([ovn_nbctl_mirrors], [mirrors], [
check ovn-nbctl mirror-add mirror1 gre 0 from-lport 10.10.10.1
check ovn-nbctl mirror-add mirror2 erspan 1 to-lport 10.10.10.2
check ovn-nbctl mirror-add mirror3 gre 2 to-lport 10.10.10.3
+check ovn-nbctl mirror-add mirror-local local both 10.10.10.3
check ovn-nbctl ls-add sw0
check ovn-nbctl lsp-add sw0 sw0-port1
check ovn-nbctl lsp-add sw0 sw0-port2
@@ -480,6 +481,11 @@ check_column "$mirror3uuid" nb:Logical_Switch_Port
mirror_rules name=sw0-port3
dnl Verify if multiple ports are attached to the same mirror properly
AT_CHECK([ovn-nbctl mirror-list], [0], [dnl
+mirror-local:
+ Type : local
+ Sink : 10.10.10.3
+ Filter : both
+
mirror1:
Type : gre
Sink : 10.10.10.1
@@ -500,6 +506,9 @@ mirror3:
])
+dnl Remove the local mirror
+check ovn-nbctl mirror-del mirror-local
+
dnl Detach one source port from mirror
check ovn-nbctl lsp-detach-mirror sw0-port3 mirror3
diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
index 32ca4f750..c37cc010c 100644
--- a/utilities/ovn-nbctl.c
+++ b/utilities/ovn-nbctl.c
@@ -7691,7 +7691,7 @@ nbctl_mirror_add(struct ctl_context *ctx)
const char *sink = NULL;
const char *type = NULL;
const char *name = NULL;
-int64_t index;
+int64_t index = -1;
char *error = NULL;
const struct nbrec_mirror *mirror_check = NULL;
int pos = 1;
@@ -7824,8 +7824,10 @@ nbctl_mirror_list(struct ctl_context *ctx)
ds_put_format(&ctx->output, " Type : %s\n", mirror->type);
ds_put_format(&ctx->output, " Sink : %s\n", mirror->sink);
ds_put_format(&ctx->output, " Filter : %s\n", mirror->filter);
-ds_put_format(&ctx->output, " Index/Key: %ld\n",
- (long int) mirror->index);
+if (strcmp(mirror->type, "local")) {
+ds_put_format(&ctx->output, " Index/Key: %"PRId64"\n",
+ mirror->index);
+}
ds_put_cstr(&ctx->output, "\n");
}
--
2.45.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn 4/4] treewide: Prevent potential NULL ptr deref.
Prevent potential NULL ptr deref that might have happened in corner
case.
Signed-off-by: Ales Musil
---
northd/northd.c | 2 +-
utilities/ovn-ic-nbctl.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/northd/northd.c b/northd/northd.c
index 7e474a7b8..b4d8e069b 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -16180,8 +16180,8 @@ build_lflows_thread(void *arg)
&lsi->actions);
}
}
+lsi->thread_lflow_counter = thread_lflow_counter;
}
-lsi->thread_lflow_counter = thread_lflow_counter;
post_completed_work(control);
}
return NULL;
diff --git a/utilities/ovn-ic-nbctl.c b/utilities/ovn-ic-nbctl.c
index 4317c385a..77a69df0b 100644
--- a/utilities/ovn-ic-nbctl.c
+++ b/utilities/ovn-ic-nbctl.c
@@ -821,7 +821,7 @@ do_ic_nbctl(const char *args, struct ctl_command *commands,
size_t n_commands,
const struct icnbrec_ic_nb_global *inb = icnbrec_ic_nb_global_first(idl);
if (!inb) {
/* XXX add verification that table is empty */
-icnbrec_ic_nb_global_insert(txn);
+ inb = icnbrec_ic_nb_global_insert(txn);
}
if (wait_type != NBCTL_WAIT_NONE) {
--
2.45.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn 2/4] northd: Fix potential overflow
The threshold computation could overflow during the conversion to ms.
Make sure that the arithmetic is done on 64-bit value instead of
32-bit one.
Signed-off-by: Ales Musil
---
northd/aging.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/northd/aging.c b/northd/aging.c
index 9685044e7..e095f29b2 100644
--- a/northd/aging.c
+++ b/northd/aging.c
@@ -330,7 +330,8 @@ aging_context_handle_timestamp(struct aging_context *ctx,
int64_t timestamp,
}
ovs_assert(ctx->threshold);
-uint64_t threshold = 1000 * find_threshold_for_ip(ip, ctx->threshold);
+uint64_t threshold =
+1000 * (uint64_t) find_threshold_for_ip(ip, ctx->threshold);
if (!threshold) {
return false;
--
2.45.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn 1/4] treewide: Remove dead code.
The coverity static analysis identified a few places with
extra checks and dead code that cannot be logically reached.
Remove this dead code from the codebase.
Signed-off-by: Ales Musil
---
controller/binding.c| 2 +-
controller/lflow.c | 5 +
controller/lflow.h | 3 +--
controller/ovn-controller.c | 3 +--
controller/pinctrl.c| 8 +---
lib/inc-proc-eng.c | 8 ++--
lib/lex.c | 1 -
northd/test-ipam.c | 4
8 files changed, 7 insertions(+), 27 deletions(-)
diff --git a/controller/binding.c b/controller/binding.c
index f467caf07..b7e7f4874 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -401,7 +401,7 @@ configure_qos(const struct sbrec_port_binding *pb,
q->queue_id = queue_id;
}
free(q->network);
-q->network = network ? xstrdup(network) : NULL;
+q->network = xstrdup(network);
q->min_rate = min_rate;
q->max_rate = max_rate;
q->burst = burst;
diff --git a/controller/lflow.c b/controller/lflow.c
index 1e05665a1..b4c379044 100644
--- a/controller/lflow.c
+++ b/controller/lflow.c
@@ -2388,15 +2388,12 @@ lflow_handle_changed_lbs(struct lflow_ctx_in *l_ctx_in,
struct lflow_ctx_out *l_ctx_out,
const struct uuidset *deleted_lbs,
const struct uuidset *updated_lbs,
- const struct uuidset *new_lbs,
- const struct hmap *old_lbs)
+ const struct uuidset *new_lbs)
{
const struct ovn_controller_lb *lb;
struct uuidset_node *uuid_node;
UUIDSET_FOR_EACH (uuid_node, deleted_lbs) {
-lb = ovn_controller_lb_find(old_lbs, &uuid_node->uuid);
-
VLOG_DBG("Remove hairpin flows for deleted load balancer "UUID_FMT,
UUID_ARGS(&uuid_node->uuid));
ofctrl_remove_flows(l_ctx_out->flow_table, &uuid_node->uuid);
diff --git a/controller/lflow.h b/controller/lflow.h
index 1d20cae35..7c947a909 100644
--- a/controller/lflow.h
+++ b/controller/lflow.h
@@ -178,8 +178,7 @@ bool lflow_handle_changed_lbs(struct lflow_ctx_in *l_ctx_in,
struct lflow_ctx_out *l_ctx_out,
const struct uuidset *deleted_lbs,
const struct uuidset *updated_lbs,
- const struct uuidset *new_lbs,
- const struct hmap *old_lbs);
+ const struct uuidset *new_lbs);
bool lflow_handle_changed_fdbs(struct lflow_ctx_in *, struct lflow_ctx_out *);
void lflow_destroy(void);
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index ed95818a0..0cbf6df58 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -4589,8 +4589,7 @@ lflow_output_lb_data_handler(struct engine_node *node,
void *data)
bool handled = lflow_handle_changed_lbs(&l_ctx_in, &l_ctx_out,
&lb_data->deleted,
&lb_data->updated,
-&lb_data->new,
-&lb_data->old_lbs);
+&lb_data->new);
engine_set_node_state(node, EN_UPDATED);
return handled;
diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index 8adec6179..f1f4cec58 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -4507,17 +4507,11 @@ ipv6_ra_send(struct rconn *swconn, struct ipv6_ra_state
*ra)
uint64_t packet_stub[128 / 8];
struct dp_packet packet;
-uint16_t router_lt = IPV6_ND_RA_LIFETIME;
-
-if (!router_lt) {
-/* Reset PRF to MEDIUM if router lifetime is not set */
-ra->config->mo_flags &= ~IPV6_ND_RA_OPT_PRF_LOW;
-}
dp_packet_use_stub(&packet, packet_stub, sizeof packet_stub);
compose_nd_ra(&packet, ra->config->eth_src, ra->config->eth_dst,
&ra->config->ipv6_src, &ra->config->ipv6_dst,
-255, ra->config->mo_flags, htons(router_lt), 0, 0,
+255, ra->config->mo_flags, htons(IPV6_ND_RA_LIFETIME), 0, 0,
ra->config->mtu);
for (int i = 0; i < ra->config->prefixes.n_ipv6_addrs; i++) {
diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c
index 4b379229e..a01440bb4 100644
--- a/lib/inc-proc-eng.c
+++ b/lib/inc-proc-eng.c
@@ -131,7 +131,6 @@ engine_dump_stats(struct unixctl_conn *conn, int argc,
const char *dump_eng_node_name = (argc > 1 ? argv[1] : NULL);
const char *dump_stat_type = (argc > 2 ? argv[2] : NULL);
-bool success = true;
for (size_t i = 0; i < engine_n_nodes; i++) {
struct engine_node *node = engine_nodes[i];
@@ -163,11 +162,8 @@ engine_dump_stats(struct unixctl_conn *conn, int argc,
break;
}
[ovs-dev] [PATCH ovn 0/4] Fix issues reported by coverity
Coverity discovered a couple of isse that and dead code. The series aims to fix some of them. Ales Musil (4): treewide: Remove dead code. northd: Fix potential overflow ovn-nbctl: Avoid uninitialized value for mirror index. treewide: Prevent potential NULL ptr deref. controller/binding.c| 2 +- controller/lflow.c | 5 + controller/lflow.h | 3 +-- controller/ovn-controller.c | 3 +-- controller/pinctrl.c| 8 +--- lib/inc-proc-eng.c | 8 ++-- lib/lex.c | 1 - northd/aging.c | 3 ++- northd/northd.c | 2 +- northd/test-ipam.c | 4 tests/ovn-nbctl.at | 9 + utilities/ovn-ic-nbctl.c| 2 +- utilities/ovn-nbctl.c | 8 +--- 13 files changed, 25 insertions(+), 33 deletions(-) -- 2.45.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC v2 9/9] tests: Test local sampling.
On 5 Jun 2024, at 22:23, Adrian Moreno wrote: > Test simultaneous IPFIX and local sampling including slow-path. I guess Ilya's comments on this patch summarize most of my comments. I had one small additional question. See below. //Eelco > Signed-off-by: Adrian Moreno > --- > tests/system-common-macros.at | 4 ++ > tests/system-traffic.at | 105 ++ > 2 files changed, 109 insertions(+) > > diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at > index 2a68cd664..22b8885e4 100644 > --- a/tests/system-common-macros.at > +++ b/tests/system-common-macros.at > @@ -378,3 +378,7 @@ m4_define([OVS_CHECK_GITHUB_ACTION], > # OVS_CHECK_DROP_ACTION() > m4_define([OVS_CHECK_DROP_ACTION], > [AT_SKIP_IF([! grep -q "Datapath supports explicit drop action" > ovs-vswitchd.log])]) > + > +# OVS_CHECK_EMIT_SAMPLE() > +m4_define([OVS_CHECK_EMIT_SAMPLE], > +[AT_SKIP_IF([! grep -q "Datapath supports emit_sample" > ovs-vswitchd.log])]) > diff --git a/tests/system-traffic.at b/tests/system-traffic.at > index bd7647cbe..babc56b56 100644 > --- a/tests/system-traffic.at > +++ b/tests/system-traffic.at > @@ -8977,3 +8977,108 @@ OVS_WAIT_UNTIL([cat p2.pcap | grep -E "0x0050: * > * *5002 *2000 *b85e *00 > > OVS_TRAFFIC_VSWITCHD_STOP > AT_CLEANUP > + > +AT_SETUP([emit_sample]) > +OVS_TRAFFIC_VSWITCHD_START() > +OVS_CHECK_EMIT_SAMPLE() > + > +ADD_NAMESPACES(at_ns0, at_ns1) > +NS_CHECK_EXEC([at_ns0], [sysctl -w net.ipv6.conf.all.disable_ipv6=1], [0], > [ignore]) > +NS_CHECK_EXEC([at_ns1], [sysctl -w net.ipv6.conf.all.disable_ipv6=1], [0], > [ignore]) > + > +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24", "e4:11:22:33:44:55") > +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24", "e4:11:22:33:44:66") > + > +NS_CHECK_EXEC([at_ns0], [arp -s 10.1.1.2 e4:11:22:33:44:66]) > +NS_CHECK_EXEC([at_ns1], [arp -s 10.1.1.1 e4:11:22:33:44:55]) > + > + > +AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg > ofproto_dpif_upcall:dbg]) > + > +AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \ > +-- --id=@ipfix create IPFIX targets=\"127.0.0.1:4739\" \ > +-- create Flow_Sample_Collector_Set id=1 bridge=@br0 > ipfix=@ipfix, local_sample_group=10 \ > +-- create Flow_Sample_Collector_Set id=2 bridge=@br0 > ipfix=@ipfix, local_sample_group=12], > + [0], [ignore]) > + > +AT_DATA([flows.txt], [dnl > +in_port=ovs-p0,ip > actions=sample(probability=65535,collector_set_id=1,obs_domain_id=1431655765,obs_point_id=1717986918),output(port=ovs-p1,max_len=100) > +in_port=ovs-p1,ip > actions=sample(probability=65535,collector_set_id=2,obs_domain_id=2290649224,obs_point_id=2576980377),output(port=ovs-p0,max_len=100) > +]) > + > +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) > + > +OVS_DAEMONIZE([ovstest test-psample > psample.out], [psample1.pid]) > + > +NS_CHECK_EXEC([at_ns0], [ping -q -c 1 10.1.1.2 | FORMAT_PING], [0], [dnl > +1 packets transmitted, 1 received, 0% packet loss, time 0ms > +]) > + > +m4_define([SAMPLE1], [group_id=0xa > obs_domain=0x,obs_point=0x > .*icmp.*nw_src=10.1.1.1,nw_dst=10.1.1.2]) > +m4_define([SAMPLE2], [group_id=0xc > obs_domain=0x,obs_point=0x > .*icmp.*nw_src=10.1.1.2,nw_dst=10.1.1.1]) > +AT_CHECK([grep -E 'SAMPLE1' psample.out >/dev/null]) > +AT_CHECK([grep -E 'SAMPLE2' psample.out >/dev/null]) > + > +AT_CHECK([ovs-ofctl dump-ipfix-flow br0 | sed 's/tx pkts=[[0-9]]*/tx > pkts=24/' | sed 's/tx errs=[[0-9]]*/tx errs=0/'], [0], [dnl Why are we making tx pkts always 24, and are we waiving any potential tx errors? Is this something you have seen not being consistent, if so any idea why? > +NXST_IPFIX_FLOW reply (xid=0x2): 2 ids > + id 1: flows=1, current flows=0, sampled pkts=1, ipv4 ok=1, ipv6 ok=0, tx > pkts=24 > + pkts errs=0, ipv4 errs=0, ipv6 errs=0, tx errs=0 > + id 2: flows=1, current flows=0, sampled pkts=1, ipv4 ok=1, ipv6 ok=0, tx > pkts=24 > + pkts errs=0, ipv4 errs=0, ipv6 errs=0, tx errs=0 > +]) > + > +AT_CHECK([ovs-appctl lsample/show br0], [0], [dnl > +local sample statistics for bridge "br0": > +- Collector Set ID: 1 > + Local Sample Group: 10 > + Total number of bytes: 98 > + Total number of packets: 1 > + > +- Collector Set ID: 2 > + Local Sample Group: 12 > + Total number of bytes: 98 > + Total number of packets: 1 > +]) > + > +# Disable trunc feature to force traffic to go through slow path. > +AT_CHECK([ovs-appctl dpif/set-dp-features br0 trunc false]) > + > +AT_CHECK([ovs-appctl ofproto/trace br0 > 'in_port=ovs-p0,dl_src=e4:11:22:33:44:55,dl_dst=e4:11:22:33:44:66,dl_type=0x0800,nw_src=10.1.1.1,nw_dst=10.1.1.12'], > [0], [stdout]) > +AT_CHECK([tail -3 stdout], [0], [dnl > +Datapath actions: > emit_sample(group=10,cookie=),userspace(pid=4294967295,flow_sample(probability=65535,collector_set_id=1,obs_domain_id=1431655765,obs_point_id=1717986918,output_port=4294967295)),trunc(100),3 > +This flow is h
Re: [ovs-dev] [RFC v2 8/9] tests: Add test-psample testing utility.
On 5 Jun 2024, at 22:23, Adrian Moreno wrote:
> This simple program reads from psample and prints the packets to stdout.
Some comments below.
//Eelco
> Signed-off-by: Adrian Moreno
> ---
> include/linux/automake.mk | 1 +
> include/linux/psample.h | 68 +
> tests/automake.mk | 3 +-
> tests/test-psample.c | 282 ++
> 4 files changed, 353 insertions(+), 1 deletion(-)
> create mode 100644 include/linux/psample.h
> create mode 100644 tests/test-psample.c
>
> diff --git a/include/linux/automake.mk b/include/linux/automake.mk
> index cdae5eedc..ac306b53c 100644
> --- a/include/linux/automake.mk
> +++ b/include/linux/automake.mk
> @@ -3,6 +3,7 @@ noinst_HEADERS += \
> include/linux/netfilter/nf_conntrack_sctp.h \
> include/linux/openvswitch.h \
> include/linux/pkt_cls.h \
> + include/linux/psample.h \
> include/linux/gen_stats.h \
> include/linux/tc_act/tc_mpls.h \
> include/linux/tc_act/tc_pedit.h \
> diff --git a/include/linux/psample.h b/include/linux/psample.h
> new file mode 100644
> index 0..d5761b730
> --- /dev/null
> +++ b/include/linux/psample.h
> @@ -0,0 +1,68 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +#ifndef __LINUX_PSAMPLE_H
> +#define __LINUX_PSAMPLE_H
> +
> +enum {
> + PSAMPLE_ATTR_IIFINDEX,
> + PSAMPLE_ATTR_OIFINDEX,
> + PSAMPLE_ATTR_ORIGSIZE,
> + PSAMPLE_ATTR_SAMPLE_GROUP,
> + PSAMPLE_ATTR_GROUP_SEQ,
> + PSAMPLE_ATTR_SAMPLE_RATE,
> + PSAMPLE_ATTR_DATA,
> + PSAMPLE_ATTR_GROUP_REFCOUNT,
> + PSAMPLE_ATTR_TUNNEL,
> +
> + PSAMPLE_ATTR_PAD,
> + PSAMPLE_ATTR_OUT_TC,/* u16 */
> + PSAMPLE_ATTR_OUT_TC_OCC,/* u64, bytes */
> + PSAMPLE_ATTR_LATENCY, /* u64, nanoseconds */
> + 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
> +};
> +
> +enum psample_command {
> + PSAMPLE_CMD_SAMPLE,
> + PSAMPLE_CMD_GET_GROUP,
> + PSAMPLE_CMD_NEW_GROUP,
> + PSAMPLE_CMD_DEL_GROUP,
> + PSAMPLE_CMD_SAMPLE_FILTER_SET,
> +};
> +
> +enum psample_tunnel_key_attr {
> + PSAMPLE_TUNNEL_KEY_ATTR_ID, /* be64 Tunnel ID */
> + PSAMPLE_TUNNEL_KEY_ATTR_IPV4_SRC, /* be32 src IP address. */
> + PSAMPLE_TUNNEL_KEY_ATTR_IPV4_DST, /* be32 dst IP address. */
> + PSAMPLE_TUNNEL_KEY_ATTR_TOS,/* u8 Tunnel IP ToS. */
> + PSAMPLE_TUNNEL_KEY_ATTR_TTL,/* u8 Tunnel IP TTL. */
> + PSAMPLE_TUNNEL_KEY_ATTR_DONT_FRAGMENT, /* No argument, set DF. */
> + PSAMPLE_TUNNEL_KEY_ATTR_CSUM, /* No argument. CSUM
> packet. */
> + PSAMPLE_TUNNEL_KEY_ATTR_OAM,/* No argument. OAM frame.
> */
> + PSAMPLE_TUNNEL_KEY_ATTR_GENEVE_OPTS,/* Array of Geneve options.
> */
> + PSAMPLE_TUNNEL_KEY_ATTR_TP_SRC, /* be16 src Transport Port.
> */
> + PSAMPLE_TUNNEL_KEY_ATTR_TP_DST, /* be16 dst Transport Port.
> */
> + PSAMPLE_TUNNEL_KEY_ATTR_VXLAN_OPTS, /* Nested VXLAN opts* */
> + PSAMPLE_TUNNEL_KEY_ATTR_IPV6_SRC, /* struct in6_addr src IPv6
> address. */
> + PSAMPLE_TUNNEL_KEY_ATTR_IPV6_DST, /* struct in6_addr dst IPv6
> address. */
> + PSAMPLE_TUNNEL_KEY_ATTR_PAD,
> + PSAMPLE_TUNNEL_KEY_ATTR_ERSPAN_OPTS,/* struct erspan_metadata */
> + PSAMPLE_TUNNEL_KEY_ATTR_IPV4_INFO_BRIDGE, /* No argument.
> IPV4_INFO_BRIDGE mode.*/
> + __PSAMPLE_TUNNEL_KEY_ATTR_MAX
> +};
> +
> +/* Can be overridden at runtime by module option */
> +#define PSAMPLE_ATTR_MAX (__PSAMPLE_ATTR_MAX - 1)
> +
> +#define PSAMPLE_NL_MCGRP_CONFIG_NAME "config"
> +#define PSAMPLE_NL_MCGRP_SAMPLE_NAME "packets"
> +#define PSAMPLE_GENL_NAME "psample"
> +#define PSAMPLE_GENL_VERSION 1
> +
> +#endif
> diff --git a/tests/automake.mk b/tests/automake.mk
> index 04f48f2d8..edfc2cb33 100644
> --- a/tests/automake.mk
> +++ b/tests/automake.mk
> @@ -499,7 +499,8 @@ endif
> if LINUX
> tests_ovstest_SOURCES += \
> tests/test-netlink-conntrack.c \
> - tests/test-netlink-policy.c
> + tests/test-netlink-policy.c \
> + tests/test-psample.c
> endif
>
> tests_ovstest_LDADD = lib/libopenvswitch.la
> diff --git a/tests/test-psample.c b/tests/test-psample.c
> new file mode 100644
> index 0..f04d903b1
> --- /dev/null
> +++ b/tests/test-psample.c
> @@ -0,0 +1,282 @@
> +/*
> + * Copyright (c) 2024 Red Hat, Inc.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not
Re: [ovs-dev] [RFC v2 7/9] ofproto-dpif-lsample: Show stats via unixctl.
On 5 Jun 2024, at 22:23, Adrian Moreno wrote:
> Add a command to dump statistics per exporter.
Some comments below.
//Eelco
> Signed-off-by: Adrian Moreno
> ---
> NEWS | 2 +
> ofproto/ofproto-dpif-lsample.c | 113 +
> ofproto/ofproto-dpif-lsample.h | 1 +
> ofproto/ofproto-dpif.c | 1 +
> 4 files changed, 117 insertions(+)
>
> diff --git a/NEWS b/NEWS
> index 1c05a7120..a443f9fe1 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -11,6 +11,8 @@ Post-v3.3.0
> allows samples to be emitted locally (instead of via IPFIX) in a
> datapath-specific manner via the new datapath action called
> "emit_sample".
> Linux kernel datapath is the first to support this feature.
> + A new unixctl command 'lsample/show' shows packet and bytes statistics
> + per local sample exporter.
>
>
> v3.3.0 - 16 Feb 2024
> diff --git a/ofproto/ofproto-dpif-lsample.c b/ofproto/ofproto-dpif-lsample.c
> index 0c71e354d..e31dabac4 100644
> --- a/ofproto/ofproto-dpif-lsample.c
> +++ b/ofproto/ofproto-dpif-lsample.c
> @@ -21,7 +21,10 @@
> #include "dpif.h"
> #include "hash.h"
> #include "ofproto.h"
> +#include "ofproto-dpif.h"
> +#include "openvswitch/dynamic-string.h"
> #include "openvswitch/thread.h"
> +#include "unixctl.h"
>
> /* Dpif local sampling.
> *
> @@ -218,3 +221,113 @@ dpif_lsample_unref(struct dpif_lsample *lsample)
> dpif_lsample_destroy(lsample);
> }
> }
> +
> +static int
> +compare_exporter_list(const void *a_, const void *b_)
We are not comparing a list here, so we should choose a more appropriate name,
maybe something like; compare_exporter_collector_set_id(), or if you want it
shorter, maybe comp_exporter_collector_id(), use you fantasy (but not to
much ;).
> +{
> +const struct lsample_exporter_node *a, *b;
> +
> +a = *(struct lsample_exporter_node **)a_;
> +b = *(struct lsample_exporter_node **)b_;
Fix space around casting.
> +
> +if (a->exporter.options.collector_set_id >
> +b->exporter.options.collector_set_id) {
> +return 1;
> +}
> +if (a->exporter.options.collector_set_id <
> +b->exporter.options.collector_set_id) {
> +return -1;
> +}
> +return 0;
> +}
> +
> +static void
> +lsample_exporter_list(struct dpif_lsample *lsample,
> + struct lsample_exporter_node ***list,
> + size_t *num_exporters)
> +{
> +struct lsample_exporter_node *node;
> +struct lsample_exporter_node **exporter_list;
Reverse Christmas tree order.
> +size_t k = 0, n;
> +
> +n = cmap_count(&lsample->exporters);
> +
> +exporter_list = xcalloc(n, sizeof *exporter_list);
> +
> +CMAP_FOR_EACH (node, node, &lsample->exporters) {
> +if (k >= n) {
> +break;
> +}
> +exporter_list[k++] = node;
> +}
> +
> +qsort(exporter_list, k, sizeof *exporter_list, compare_exporter_list);
> +
> +*list = exporter_list;
> +*num_exporters = k;
> +}
> +
> +static void
> +lsample_unixctl_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
> + const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
> +{
> +struct lsample_exporter_node **node_list = NULL;
> +struct lsample_exporter_node *node;
> +struct ds ds = DS_EMPTY_INITIALIZER;
Reverse Christmas tree? Or even better move the *node definition inside the
for loop below.
> +const struct ofproto_dpif *ofproto;
> +size_t i, num;
> +
> +ofproto = ofproto_dpif_lookup_by_name(argv[1]);
> +if (!ofproto) {
> +unixctl_command_reply_error(conn, "no such bridge");
> +return;
> +}
> +
> +if (!ofproto->lsample) {
> +unixctl_command_reply_error(conn, "no local sampling exporters "
> +"configured");
unixctl_command_reply_error(conn,
"no local sampling exporters configured");
> +return;
> +}
> +
> +ds_put_format(&ds, "local sample statistics for bridge \"%s\":\n",
Maybe capital L for Local?
> + argv[1]);
> +
> +lsample_exporter_list(ofproto->lsample, &node_list, &num);
> +
> +for (i = 0; i < num; i++) {
> +uint64_t n_bytes;
> +uint64_t n_packets;
Reverse Christmas tree
> +
> +node = node_list[i];
> +
> +atomic_read_relaxed(&node->exporter.n_packets, &n_packets);
> +atomic_read_relaxed(&node->exporter.n_bytes, &n_bytes);
> +
> +if (i) {
> +ds_put_cstr(&ds, "\n");
> +}
> +
> +ds_put_format(&ds, "- Collector Set ID: %"PRIu32"\n",
> +node->exporter.options.collector_set_id);
> +ds_put_format(&ds, " Local Sample Group: %"PRIu32"\n",
> +node->exporter.options.group_id);
Indentation is off for both format calls above.
> +ds_put_format(&ds, " Total number of bytes: %"PRIu64"\n", n_bytes);
> +ds_put_format(&ds,
Re: [ovs-dev] [RFC v2 6/9] ofproto-dpif-xlate-cache: Add lsample to xcache.
On 5 Jun 2024, at 22:23, Adrian Moreno wrote:
> Add a cache entry type for local sample objects.
> Store both the dpif_lsample reference and the collector_set_id so we can
> quickly find the particular exporter.
>
> Using this mechanism, account for packet and byte statistics.
Hi Adrian,
Some small nits below.
Cheers,
Eelco
> Signed-off-by: Adrian Moreno
> ---
> ofproto/ofproto-dpif-lsample.c | 18 ++
> ofproto/ofproto-dpif-lsample.h | 4
> ofproto/ofproto-dpif-xlate-cache.c | 11 ++-
> ofproto/ofproto-dpif-xlate-cache.h | 6 ++
> ofproto/ofproto-dpif-xlate.c | 15 +++
> ofproto/ofproto-dpif.c | 1 +
> 6 files changed, 54 insertions(+), 1 deletion(-)
>
> diff --git a/ofproto/ofproto-dpif-lsample.c b/ofproto/ofproto-dpif-lsample.c
> index 2c0b5da89..0c71e354d 100644
> --- a/ofproto/ofproto-dpif-lsample.c
> +++ b/ofproto/ofproto-dpif-lsample.c
> @@ -18,6 +18,7 @@
> #include "ofproto-dpif-lsample.h"
>
> #include "cmap.h"
> +#include "dpif.h"
> #include "hash.h"
> #include "ofproto.h"
> #include "openvswitch/thread.h"
> @@ -44,6 +45,8 @@ struct dpif_lsample {
>
> struct lsample_exporter {
> struct ofproto_lsample_options options;
> +atomic_uint64_t n_packets;
> +atomic_uint64_t n_bytes;
> };
>
> struct lsample_exporter_node {
> @@ -156,6 +159,21 @@ dpif_lsample_get_group_id(struct dpif_lsample *ps,
> uint32_t collector_set_id,
> return found;
> }
>
> +void
> +dpif_lsample_credit_stats(struct dpif_lsample *lsample,
> + uint32_t collector_set_id,
> + const struct dpif_flow_stats *stats)
> +{
> +struct lsample_exporter_node *node;
> +uint64_t orig;
> +
> +node = dpif_lsample_find_exporter_node(lsample, collector_set_id);
> +if (node) {
> +atomic_add_relaxed(&node->exporter.n_packets, stats->n_packets,
> &orig);
> +atomic_add_relaxed(&node->exporter.n_bytes, stats->n_bytes, &orig);
> +}
> +}
> +
> struct dpif_lsample *
> dpif_lsample_create(void)
> {
> diff --git a/ofproto/ofproto-dpif-lsample.h b/ofproto/ofproto-dpif-lsample.h
> index f13425575..2ce096161 100644
> --- a/ofproto/ofproto-dpif-lsample.h
> +++ b/ofproto/ofproto-dpif-lsample.h
> @@ -23,6 +23,7 @@
>
> struct dpif_lsample;
> struct ofproto_lsample_options;
> +struct dpif_flow_stats;
>
> struct dpif_lsample *dpif_lsample_create(void);
> void dpif_lsample_unref(struct dpif_lsample *);
> @@ -36,5 +37,8 @@ bool dpif_lsample_get_group_id(struct dpif_lsample *,
> uint32_t collector_set_id,
> uint32_t *group_id);
>
> +void dpif_lsample_credit_stats(struct dpif_lsample *,
> + uint32_t collector_set_id,
> + const struct dpif_flow_stats *);
> #endif /* OFPROTO_DPIF_LSAMPLE_H */
>
> diff --git a/ofproto/ofproto-dpif-xlate-cache.c
> b/ofproto/ofproto-dpif-xlate-cache.c
> index 2e1fcb3a6..508e5fcb2 100644
> --- a/ofproto/ofproto-dpif-xlate-cache.c
> +++ b/ofproto/ofproto-dpif-xlate-cache.c
> @@ -35,9 +35,10 @@
> #include "learn.h"
> #include "mac-learning.h"
> #include "netdev-vport.h"
> +#include "ofproto/ofproto-dpif.h"
> #include "ofproto/ofproto-dpif-mirror.h"
> +#include "ofproto/ofproto-dpif-lsample.h"
If you re-order you might as well put lsample before mirror ;)
> #include "ofproto/ofproto-dpif-xlate.h"
> -#include "ofproto/ofproto-dpif.h"
> #include "ofproto/ofproto-provider.h"
> #include "openvswitch/dynamic-string.h"
> #include "openvswitch/vlog.h"
> @@ -162,6 +163,11 @@ xlate_push_stats_entry(struct xc_entry *entry,
> }
>
> break;
> +case XC_LSAMPLE:
> +dpif_lsample_credit_stats(entry->lsample.lsample,
> + entry->lsample.collector_set_id,
> + stats);
> +break;
> default:
> OVS_NOT_REACHED();
> }
> @@ -245,6 +251,9 @@ xlate_cache_clear_entry(struct xc_entry *entry)
> break;
> case XC_TUNNEL_HEADER:
> break;
> +case XC_LSAMPLE:
> +dpif_lsample_unref(entry->lsample.lsample);
> +break;
> default:
> OVS_NOT_REACHED();
> }
> diff --git a/ofproto/ofproto-dpif-xlate-cache.h
> b/ofproto/ofproto-dpif-xlate-cache.h
> index 0fc6d2ea6..df8115419 100644
> --- a/ofproto/ofproto-dpif-xlate-cache.h
> +++ b/ofproto/ofproto-dpif-xlate-cache.h
> @@ -29,6 +29,7 @@
> struct bfd;
> struct bond;
> struct dpif_flow_stats;
> +struct dpif_lsample;
> struct flow;
> struct group_dpif;
> struct mbridge;
> @@ -53,6 +54,7 @@ enum xc_type {
> XC_GROUP,
> XC_TNL_NEIGH,
> XC_TUNNEL_HEADER,
> +XC_LSAMPLE,
> };
>
> /* xlate_cache entries hold enough information to perform the side effects of
> @@ -126,6 +128,10 @@ struct xc_entry {
> } operation;
> uint16_t hdr_size;
> } tunnel_hdr;
> +struct {
> +
Re: [ovs-dev] [RFC v2 5/9] ofproto-dpif-xlate: Use emit_sample for local sample.
On 5 Jun 2024, at 22:23, Adrian Moreno wrote:
> Use the newly added emit_sample to implement OpenFlow sample() actions
> with local sampling configuration.
See some comments below.
Cheers,
Eelco
> Signed-off-by: Adrian Moreno
> ---
> ofproto/ofproto-dpif-lsample.c | 17
> ofproto/ofproto-dpif-lsample.h | 6 ++
> ofproto/ofproto-dpif-xlate.c | 163 -
> ofproto/ofproto-dpif-xlate.h | 3 +-
> ofproto/ofproto-dpif.c | 2 +-
> 5 files changed, 144 insertions(+), 47 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-lsample.c b/ofproto/ofproto-dpif-lsample.c
> index 7bdafac19..2c0b5da89 100644
> --- a/ofproto/ofproto-dpif-lsample.c
> +++ b/ofproto/ofproto-dpif-lsample.c
> @@ -139,6 +139,23 @@ dpif_lsample_set_options(struct dpif_lsample *lsample,
> return changed;
> }
>
> +/* Returns the group_id of the exporter with the given collector_set_id, if
> it
> + * exists. */
nit: The below will fit on one line
/* Returns the exporter group_id for a given collector_set_id, if it exists. */
> +bool
> +dpif_lsample_get_group_id(struct dpif_lsample *ps, uint32_t collector_set_id,
> + uint32_t *group_id)
> +{
> +struct lsample_exporter_node *node;
> +bool found = false;
> +
> +node = dpif_lsample_find_exporter_node(ps, collector_set_id);
> +if (node) {
> +found = true;
> +*group_id = node->exporter.options.group_id;
> +}
> +return found;
> +}
> +
> struct dpif_lsample *
> dpif_lsample_create(void)
> {
> diff --git a/ofproto/ofproto-dpif-lsample.h b/ofproto/ofproto-dpif-lsample.h
> index c23ea8372..f13425575 100644
> --- a/ofproto/ofproto-dpif-lsample.h
> +++ b/ofproto/ofproto-dpif-lsample.h
> @@ -19,6 +19,7 @@
>
> #include
> #include
> +#include
Maybe add in alphabetical order.
> struct dpif_lsample;
> struct ofproto_lsample_options;
> @@ -31,4 +32,9 @@ bool dpif_lsample_set_options(struct dpif_lsample *,
>const struct ofproto_lsample_options *,
>size_t n_opts);
>
> +bool dpif_lsample_get_group_id(struct dpif_lsample *,
> + uint32_t collector_set_id,
> + uint32_t *group_id);
> +
> #endif /* OFPROTO_DPIF_LSAMPLE_H */
> +
Accedantely added a newline?
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 7c4950895..5bd215d31 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -47,6 +47,7 @@
> #include "ofproto/ofproto-dpif-ipfix.h"
> #include "ofproto/ofproto-dpif-mirror.h"
> #include "ofproto/ofproto-dpif-monitor.h"
> +#include "ofproto/ofproto-dpif-lsample.h"
> #include "ofproto/ofproto-dpif-sflow.h"
> #include "ofproto/ofproto-dpif-trace.h"
> #include "ofproto/ofproto-dpif-xlate-cache.h"
> @@ -117,6 +118,7 @@ struct xbridge {
> struct dpif_sflow *sflow; /* SFlow handle, or null. */
> struct dpif_ipfix *ipfix; /* Ipfix handle, or null. */
> struct netflow *netflow; /* Netflow handle, or null. */
> +struct dpif_lsample *lsample; /* Local sample handle, or null. */
> struct stp *stp; /* STP or null if disabled. */
> struct rstp *rstp;/* RSTP or null if disabled. */
>
> @@ -687,6 +689,7 @@ static void xlate_xbridge_set(struct xbridge *, struct
> dpif *,
>const struct dpif_sflow *,
>const struct dpif_ipfix *,
>const struct netflow *,
> + const struct dpif_lsample *,
>bool forward_bpdu, bool has_in_band,
>const struct dpif_backer_support *,
>const struct xbridge_addr *);
> @@ -1070,6 +1073,7 @@ xlate_xbridge_set(struct xbridge *xbridge,
>const struct dpif_sflow *sflow,
>const struct dpif_ipfix *ipfix,
>const struct netflow *netflow,
> + const struct dpif_lsample *lsample,
nit: I would move above netflow, as you also do the actual init/unref before
netflow.
>bool forward_bpdu, bool has_in_band,
>const struct dpif_backer_support *support,
>const struct xbridge_addr *addr)
> @@ -1099,6 +1103,11 @@ xlate_xbridge_set(struct xbridge *xbridge,
> xbridge->ipfix = dpif_ipfix_ref(ipfix);
> }
>
> +if (xbridge->lsample != lsample) {
> +dpif_lsample_unref(xbridge->lsample);
> +xbridge->lsample = dpif_lsample_ref(lsample);
> +}
> +
> if (xbridge->stp != stp) {
> stp_unref(xbridge->stp);
> xbridge->stp = stp_ref(stp);
> @@ -1214,8 +1223,9 @@ xlate_xbridge_copy(struct xbridge *xbridge)
>xbridge->dpif, xbridge->ml, xbridge->stp,
>xbridge->rstp, xbridge->ms, xbridge->mbridge,
Re: [ovs-dev] [RFC v2 4/9] vswitchd: Add local sampling to vswitchd schema.
On 5 Jun 2024, at 22:23, Adrian Moreno wrote:
> Add as new column in the Flow_Sample_Collector_Set table named
> "local_sample_group" which enables this feature.
>
> Signed-off-by: Adrian Moreno
> ---
> NEWS | 4 ++
> vswitchd/bridge.c | 78 +++---
> vswitchd/vswitch.ovsschema | 9 -
> vswitchd/vswitch.xml | 39 +--
> 4 files changed, 119 insertions(+), 11 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index b92cec532..1c05a7120 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -7,6 +7,10 @@ Post-v3.3.0
> - The primary development branch has been renamed from 'master' to 'main'.
> The OVS tree remains hosted on GitHub.
> https://github.com/openvswitch/ovs.git
> + - Local sampling is introduced. It reuses the OpenFlow sample action and
> + allows samples to be emitted locally (instead of via IPFIX) in a
> + datapath-specific manner via the new datapath action called
> "emit_sample".
> + Linux kernel datapath is the first to support this feature.
>
>
> v3.3.0 - 16 Feb 2024
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index 95a65fcdc..cd7dc6646 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -288,6 +288,7 @@ static void bridge_configure_mac_table(struct bridge *);
> static void bridge_configure_mcast_snooping(struct bridge *);
> static void bridge_configure_sflow(struct bridge *, int
> *sflow_bridge_number);
> static void bridge_configure_ipfix(struct bridge *);
> +static void bridge_configure_lsample(struct bridge *);
> static void bridge_configure_spanning_tree(struct bridge *);
> static void bridge_configure_tables(struct bridge *);
> static void bridge_configure_dp_desc(struct bridge *);
> @@ -989,6 +990,7 @@ bridge_reconfigure(const struct ovsrec_open_vswitch
> *ovs_cfg)
> bridge_configure_netflow(br);
> bridge_configure_sflow(br, &sflow_bridge_number);
> bridge_configure_ipfix(br);
> +bridge_configure_lsample(br);
> bridge_configure_spanning_tree(br);
> bridge_configure_tables(br);
> bridge_configure_dp_desc(br);
> @@ -1537,10 +1539,11 @@ ovsrec_ipfix_is_valid(const struct ovsrec_ipfix
> *ipfix)
> return ipfix && ipfix->n_targets > 0;
> }
>
> -/* Returns whether a Flow_Sample_Collector_Set row is valid. */
> +/* Returns whether a Flow_Sample_Collector_Set row constains valid IPFIX
constains -> 'contains a'
> + * configuration. */
> static bool
> -ovsrec_fscs_is_valid(const struct ovsrec_flow_sample_collector_set *fscs,
> - const struct bridge *br)
> +ovsrec_fscs_is_valid_ipfix(const struct ovsrec_flow_sample_collector_set
> *fscs,
> + const struct bridge *br)
> {
> return ovsrec_ipfix_is_valid(fscs->ipfix) && fscs->bridge == br->cfg;
> }
> @@ -1558,7 +1561,7 @@ bridge_configure_ipfix(struct bridge *br)
> const char *virtual_obs_id;
>
> OVSREC_FLOW_SAMPLE_COLLECTOR_SET_FOR_EACH(fe_cfg, idl) {
> -if (ovsrec_fscs_is_valid(fe_cfg, br)) {
> +if (ovsrec_fscs_is_valid_ipfix(fe_cfg, br)) {
> n_fe_opts++;
> }
> }
> @@ -1621,7 +1624,7 @@ bridge_configure_ipfix(struct bridge *br)
> fe_opts = xcalloc(n_fe_opts, sizeof *fe_opts);
> opts = fe_opts;
> OVSREC_FLOW_SAMPLE_COLLECTOR_SET_FOR_EACH(fe_cfg, idl) {
> -if (ovsrec_fscs_is_valid(fe_cfg, br)) {
> +if (ovsrec_fscs_is_valid_ipfix(fe_cfg, br)) {
> opts->collector_set_id = fe_cfg->id;
> sset_init(&opts->targets);
> sset_add_array(&opts->targets, fe_cfg->ipfix->targets,
> @@ -1667,6 +1670,71 @@ bridge_configure_ipfix(struct bridge *br)
> }
> }
>
> +/* Returns whether a Flow_Sample_Collector_Set row contains valid local
> + * sampling configuraiton. */
...row contains a valid local...
++
configuraiton -> configuration
> +static bool
> +ovsrec_fscs_is_valid_local(const struct ovsrec_flow_sample_collector_set
> *fscs,
> + const struct bridge *br)
> +{
> +return fscs->local_sample_group && fscs->n_local_sample_group == 1 &&
> + fscs->bridge == br->cfg;
> +}
> +
> +/* Set local sample configuration on 'br'. */
> +static void
> +bridge_configure_lsample(struct bridge *br)
> +{
> +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> +const struct ovsrec_flow_sample_collector_set *fscs;
> +struct ofproto_lsample_options *opts_array, *opts;
> +size_t n_opts = 0;
> +int ret;
> +
> +/* Iterate the Flow_Sample_Collector_Set table twice.
> + * First to get the number of valid configuration entries, then to
> process
> + * each of them and build an array of options. */
> +OVSREC_FLOW_SAMPLE_COLLECTOR_SET_FOR_EACH (fscs, idl) {
> +if (ovsrec_fscs_is_valid_local(fscs, br)) {
> +n_opts ++;
> +}
> +}
Did you consider just all
Re: [ovs-dev] [RFC v2 3/9] ofproto: Add ofproto-dpif-lsample.
On 5 Jun 2024, at 22:23, Adrian Moreno wrote:
> Add a new resource in ofproto-dpif and the corresponding API in
> ofproto_provider.h to represent and local sampling configuration.
>
> Signed-off-by: Adrian Moreno
See some comments below.
Cheers,
Eelco
> ---
> ofproto/automake.mk| 2 +
> ofproto/ofproto-dpif-lsample.c | 185 +
> ofproto/ofproto-dpif-lsample.h | 34 ++
> ofproto/ofproto-dpif.c | 37 +++
> ofproto/ofproto-dpif.h | 1 +
> ofproto/ofproto-provider.h | 9 ++
> ofproto/ofproto.c | 12 +++
> ofproto/ofproto.h | 8 ++
> 8 files changed, 288 insertions(+)
> create mode 100644 ofproto/ofproto-dpif-lsample.c
> create mode 100644 ofproto/ofproto-dpif-lsample.h
>
> diff --git a/ofproto/automake.mk b/ofproto/automake.mk
> index 7c08b563b..fd39bf561 100644
> --- a/ofproto/automake.mk
> +++ b/ofproto/automake.mk
> @@ -34,6 +34,8 @@ ofproto_libofproto_la_SOURCES = \
> ofproto/ofproto-dpif-mirror.h \
> ofproto/ofproto-dpif-monitor.c \
> ofproto/ofproto-dpif-monitor.h \
> + ofproto/ofproto-dpif-lsample.c \
> + ofproto/ofproto-dpif-lsample.h \
Guess these need to move before the dpif-m* files.
> ofproto/ofproto-dpif-rid.c \
> ofproto/ofproto-dpif-rid.h \
> ofproto/ofproto-dpif-sflow.c \
> diff --git a/ofproto/ofproto-dpif-lsample.c b/ofproto/ofproto-dpif-lsample.c
> new file mode 100644
> index 0..7bdafac19
> --- /dev/null
> +++ b/ofproto/ofproto-dpif-lsample.c
> @@ -0,0 +1,185 @@
> +/*
> + * Copyright (c) 2024 Red Hat, Inc.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include
> +#include "ofproto-dpif-lsample.h"
> +
> +#include "cmap.h"
> +#include "hash.h"
> +#include "ofproto.h"
> +#include "openvswitch/thread.h"
> +
> +/* Dpif local sampling.
> + *
> + * Thread safety: dpif_lsample allows lockless concurrent reads of local
> + * sampling exporters as long as the following restrictions are met:
> + * 1) While the last reference is being dropped, i.e: a thread is calling
> + * "dpif_lsample_unref" on the last reference, other threads cannot call
> + * "dpif_lsample_ref".
> + * 2) Threads do not quiese while holding references to internal
> + * lsample_exporter objects.
> + */
> +
> +struct dpif_lsample {
> +struct cmap exporters; /* Contains lsample_exporter_node
> instances
> + indexed by collector_set_id. */
> +struct ovs_mutex mutex; /* Protects concurrent insertion/deletion
> + * of exporters. */
> +
> +struct ovs_refcount ref_cnt;/* Controls references to this instance.
> */
> +};
> +
> +struct lsample_exporter {
> +struct ofproto_lsample_options options;
> +};
> +
> +struct lsample_exporter_node {
> +struct cmap_node node; /* In dpif_lsample->exporters. */
> +struct lsample_exporter exporter;
> +};
> +
> +static void
> +dpif_lsample_delete_exporter(struct dpif_lsample *lsample,
> + struct lsample_exporter_node *node)
> +{
> +ovs_mutex_lock(&lsample->mutex);
> +cmap_remove(&lsample->exporters, &node->node,
> +hash_int(node->exporter.options.collector_set_id, 0));
> +ovs_mutex_unlock(&lsample->mutex);
> +
> +ovsrcu_postpone(free, node);
> +}
> +
> +/* Adds an exporter with the provided options which are copied. */
> +static struct lsample_exporter_node *
> +dpif_lsample_add_exporter(struct dpif_lsample *lsample,
> + const struct ofproto_lsample_options *options)
> +{
> +struct lsample_exporter_node *node;
New line between definitions and code.
> +node = xzalloc(sizeof *node);
> +node->exporter.options = *options;
> +
> +ovs_mutex_lock(&lsample->mutex);
> +cmap_insert(&lsample->exporters, &node->node,
> +hash_int(options->collector_set_id, 0));
> +ovs_mutex_unlock(&lsample->mutex);
> +
> +return node;
> +}
> +
> +static struct lsample_exporter_node *
> +dpif_lsample_find_exporter_node(const struct dpif_lsample *lsample,
> +const uint32_t collector_set_id)
> +{
> +struct lsample_exporter_node *node;
> +
> +CMAP_FOR_EACH_WITH_HASH (node, node,
> +hash_int(collector_set_id, 0),
> +&lsample->exporters) {
> +
Re: [ovs-dev] [RFC v2 2/9] ofproto_dpif: Check for emit_sample support.
On 5 Jun 2024, at 22:23, Adrian Moreno wrote:
> Only kernel datapath supports this action so add a function in dpif.c
> that checks for that.
>
> Signed-off-by: Adrian Moreno
Small nits below, other than that the patches looks good to me.
//Eelco
Acked-by: Eelco Chaudron
> ---
> lib/dpif.c | 7 +++
> lib/dpif.h | 1 +
> ofproto/ofproto-dpif.c | 45 ++
> ofproto/ofproto-dpif.h | 6 +-
> 4 files changed, 58 insertions(+), 1 deletion(-)
>
> diff --git a/lib/dpif.c b/lib/dpif.c
> index 489d6a095..ecba967c3 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -1953,6 +1953,13 @@ dpif_supports_lb_output_action(const struct dpif *dpif)
> return dpif_is_netdev(dpif);
> }
>
> +bool
> +dpif_may_support_emit_sample(const struct dpif *dpif)
> +{
> +/* Userspace datapath does not support this action. */
> +return !dpif_is_netdev(dpif);
> +}
> +
> /* Meters */
> void
> dpif_meter_get_features(const struct dpif *dpif,
> diff --git a/lib/dpif.h b/lib/dpif.h
> index a764e8a59..08473ea6f 100644
> --- a/lib/dpif.h
> +++ b/lib/dpif.h
> @@ -942,6 +942,7 @@ char *dpif_get_dp_version(const struct dpif *);
> bool dpif_supports_tnl_push_pop(const struct dpif *);
> bool dpif_may_support_explicit_drop_action(const struct dpif *);
> bool dpif_synced_dp_layers(struct dpif *);
> +bool dpif_may_support_emit_sample(const struct dpif *);
Maybe add this above dpif_may_support_explicit_drop_action()?
> /* Log functions. */
> struct vlog_module;
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 32d037be6..035479285 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -873,6 +873,12 @@ ovs_lb_output_action_supported(struct ofproto_dpif
> *ofproto)
> return ofproto->backer->rt_support.lb_output_action;
> }
>
> +bool
> +ovs_emit_sample_supported(struct ofproto_dpif *ofproto)
> +{
> +return ofproto->backer->rt_support.emit_sample;
> +}
> +
> /* Tests whether 'backer''s datapath supports recirculation. Only newer
> * datapaths support OVS_KEY_ATTR_RECIRC_ID in keys. We need to disable some
> * features on older datapaths that don't support this feature.
> @@ -1609,6 +1615,44 @@ check_add_mpls(struct dpif_backer *backer)
> return supported;
> }
>
> +/* Tests whether 'backer''s datapath supports the OVS_ACTION_ATTR_EMIT_SAMPLE
> + * action. */
> +static bool
> +check_emit_sample(struct dpif_backer *backer)
> +{
> +uint8_t cookie[OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE];
> +struct odputil_keybuf keybuf;
> +struct ofpbuf actions;
> +struct ofpbuf key;
> +struct flow flow;
> +bool supported;
> +
> +struct odp_flow_key_parms odp_parms = {
> +.flow = &flow,
> +.probe = true,
> +};
> +
> +memset(&flow, 0, sizeof flow);
> +ofpbuf_use_stack(&key, &keybuf, sizeof keybuf);
> +odp_flow_key_from_flow(&odp_parms, &key);
> +ofpbuf_init(&actions, 64);
> +
> +/* Generate a random max-size cookie. */
> +random_bytes(cookie, sizeof(cookie));
> +
> +odp_put_emit_sample_action(&actions, 10, cookie, sizeof cookie);
> +
> +supported = dpif_may_support_emit_sample(backer->dpif) &&
> +dpif_probe_feature(backer->dpif, "emit_sample", &key, &actions,
> NULL);
> +
> +ofpbuf_uninit(&actions);
> +VLOG_INFO("%s: Datapath %s emit_sample",
> + dpif_name(backer->dpif),
> + supported ? "supports" : "does not support");
> +return supported;
> +}
> +
> +
Guess a single new line would be enough.
> #define CHECK_FEATURE__(NAME, SUPPORT, FIELD, VALUE, ETHTYPE) \
> static bool \
> check_##NAME(struct dpif_backer *backer)\
> @@ -1698,6 +1742,7 @@ check_support(struct dpif_backer *backer)
> dpif_supports_lb_output_action(backer->dpif);
> backer->rt_support.ct_zero_snat = dpif_supports_ct_zero_snat(backer);
> backer->rt_support.add_mpls = check_add_mpls(backer);
> +backer->rt_support.emit_sample = check_emit_sample(backer);
>
> /* Flow fields. */
> backer->rt_support.odp.ct_state = check_ct_state(backer);
> diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
> index d33f73df8..ae6568463 100644
> --- a/ofproto/ofproto-dpif.h
> +++ b/ofproto/ofproto-dpif.h
> @@ -213,7 +213,10 @@ struct group_dpif *group_dpif_lookup(struct ofproto_dpif
> *,
> DPIF_SUPPORT_FIELD(bool, ct_zero_snat, "Conntrack all-zero IP SNAT")\
> \
> /* True if the datapath supports add_mpls action. */\
> -DPIF_SUPPORT_FIELD(bool, add_mpls, "MPLS Label add")
> +DPIF_SUPPORT_FIELD(bool, add_mpls, "MPLS Label add")\
> +\
> +/* True if the datapath supports emit_sample acti
Re: [ovs-dev] [RFC v2 1/9] odp-util: Add support OVS_ACTION_ATTR_EMIT_SAMPLE.
On 5 Jun 2024, at 22:23, Adrian Moreno wrote:
> Add support for parsing and formatting the new action.
>
> Also, flag OVS_ACTION_ATTR_SAMPLE as requiring datapath assistance if it
> contains a nested OVS_ACTION_ATTR_EMIT_SAMPLE. The reason is that the
> sampling rate form the parent "sample" is made available to the nested
> "emit_sample" by the kernel.
Hi Adrian,
Thanks for this series! This email kicks off my review of the series,
see comments below and in the other patches.
Cheers,
Eelco
> Signed-off-by: Adrian Moreno
> ---
> include/linux/openvswitch.h | 25 +
> lib/dpif-netdev.c| 1 +
> lib/dpif.c | 1 +
> lib/odp-execute.c| 25 -
> lib/odp-util.c | 103 +++
> lib/odp-util.h | 3 +
> ofproto/ofproto-dpif-ipfix.c | 1 +
> ofproto/ofproto-dpif-sflow.c | 1 +
> python/ovs/flow/odp.py | 8 +++
> tests/odp.at | 16 ++
> 10 files changed, 183 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
> index d9fb991ef..b4e0647bd 100644
> --- a/include/linux/openvswitch.h
> +++ b/include/linux/openvswitch.h
> @@ -992,6 +992,30 @@ struct check_pkt_len_arg {
> };
> #endif
>
> +#define OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE 16
> +/**
> + * enum ovs_emit_sample_attr - Attributes for %OVS_ACTION_ATTR_EMIT_SAMPLE
> + * action.
> + *
> + * @OVS_EMIT_SAMPLE_ATTR_GROUP: 32-bit number to identify the source of the
> + * sample.
> + * @OVS_EMIT_SAMPLE_ATTR_COOKIE: A variable-length binary cookie that
> contains
> + * user-defined metadata. The maximum length is 16 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 to limit the size of the packet being emitted.
I'll start the discussion again on the naming. The name "emit_sample()"
does not seem appropriate. This function's primary role is to copy the
packet and send it to a local collector, which varies depending on the
datapath. For the kernel datapath, this collector is psample, while for
userspace, it will likely be some kind of probe. This action is distinct
from the sample() action by design; it is a standalone action that can
be combined with others.
Furthermore, the action itself does not involve taking a sample; it
consistently pushes the packet to the local collector. Therefore, I
suggest renaming "emit_sample()" to "emit_local()". This same goes for
all the derivative ATTR naming.
> + */
> +enum ovs_emit_sample_attr {
> + OVS_EMIT_SAMPLE_ATTR_UNPSEC,
> + OVS_EMIT_SAMPLE_ATTR_GROUP, /* u32 number. */
> + OVS_EMIT_SAMPLE_ATTR_COOKIE,/* Optional, user specified cookie. */
Fix comment alignment? Maybe also change the order to be alphabetical?
> + __OVS_EMIT_SAMPLE_ATTR_MAX
> +};
> +
> +#define OVS_EMIT_SAMPLE_ATTR_MAX (__OVS_EMIT_SAMPLE_ATTR_MAX - 1)
> +
> +
> /**
> * enum ovs_action_attr - Action types.
> *
> @@ -1087,6 +,7 @@ enum ovs_action_attr {
> OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */
> OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */
> OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */
> + OVS_ACTION_ATTR_EMIT_SAMPLE, /* Nested OVS_EMIT_SAMPLE_ATTR_*. */
>
> #ifndef __KERNEL__
> OVS_ACTION_ATTR_TUNNEL_PUSH, /* struct ovs_action_push_tnl*/
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index c7f9e1490..c1171890c 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -9519,6 +9519,7 @@ dp_execute_cb(void *aux_, struct dp_packet_batch
> *packets_,
> case OVS_ACTION_ATTR_DROP:
> case OVS_ACTION_ATTR_ADD_MPLS:
> case OVS_ACTION_ATTR_DEC_TTL:
> +case OVS_ACTION_ATTR_EMIT_SAMPLE:
> case __OVS_ACTION_ATTR_MAX:
> OVS_NOT_REACHED();
> }
> diff --git a/lib/dpif.c b/lib/dpif.c
> index 23eb18495..489d6a095 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -1192,6 +1192,7 @@ dpif_execute_helper_cb(void *aux_, struct
> dp_packet_batch *packets_,
> case OVS_ACTION_ATTR_TUNNEL_PUSH:
> case OVS_ACTION_ATTR_TUNNEL_POP:
> case OVS_ACTION_ATTR_USERSPACE:
> +case OVS_ACTION_ATTR_EMIT_SAMPLE:
> case OVS_ACTION_ATTR_RECIRC: {
> struct dpif_execute execute;
> struct ofpbuf execute_actions;
> diff --git a/lib/odp-execute.c b/lib/odp-execute.c
> index 081e4d432..967abfd0a 100644
> --- a/lib/odp-execute.c
> +++ b/lib/odp-execute.c
> @@ -818,13 +818,13 @@ requires_datapath_assistance(const struct nlattr *a)
> case OVS_ACTION_ATTR_RECIRC:
> case OVS_ACTION_ATTR_CT:
> case OVS_ACTION_ATTR_METER:
> +case OVS_ACTION_ATTR_EMIT_SAMPLE:
> return true;
>
> case OVS_ACTION_ATTR_SET:
> case OVS_ACTION_ATTR_SET_MASKED:
> case OVS_ACTION_ATTR_PUSH_VLAN:
> case OVS_ACTION_ATTR_POP_VLAN:
> -case OVS_A
Re: [ovs-dev] [PATCH net-next v5 10/10] selftests: openvswitch: add emit_sample test
On 6/25/24 22:51, Adrian Moreno wrote: > 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 | 114 +- > .../selftests/net/openvswitch/ovs-dpctl.py| 73 ++- > 2 files changed, 181 insertions(+), 6 deletions(-) > > diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh > b/tools/testing/selftests/net/openvswitch/openvswitch.sh > index 15bca0708717..aeb9bee772be 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 > + emit_sample emit_sample: Sampling packets > with psample" There is an extra space character right after emit_sample word. This makes './openvswitch.sh emit_sample' to not run the test, because 'emit_sample' != 'emit_sample '. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next v5 06/10] net: openvswitch: store sampling probability in cb.
On 6/25/24 22:51, Adrian Moreno wrote: > 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 emit_sample to pass it down to psample. > > 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(-) Reviewed-by: Ilya Maximets ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next v5 05/10] net: openvswitch: add emit_sample action
On 6/25/24 22:51, Adrian Moreno wrote:
> Add support for a new action: emit_sample.
>
> 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.
>
> 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 | 45 +++
> net/openvswitch/flow_netlink.c| 33 -
> 5 files changed, 123 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/netlink/specs/ovs_flow.yaml
> b/Documentation/netlink/specs/ovs_flow.yaml
> index 4fdfc6b5cae9..a7ab5593a24f 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: emit-sample
> +type: nest
> +nested-attributes: emit-sample-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: emit-sample-attrs
> +enum-name: ovs-emit-sample-attr
> +name-prefix: ovs-emit-sample-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..8cfa1b3f6b06 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_EMIT_SAMPLE_COOKIE_MAX_SIZE 16
> +/**
> + * enum ovs_emit_sample_attr - Attributes for %OVS_ACTION_ATTR_EMIT_SAMPLE
> + * action.
> + *
> + * @OVS_EMIT_SAMPLE_ATTR_GROUP: 32-bit number to identify the source of the
> + * sample.
> + * @OVS_EMIT_SAMPLE_ATTR_COOKIE: A variable-length binary cookie that
> contains
> + * user-defined metadata. The maximum length is
> OVS_EMIT_SAMPLE_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 packet being
> emitted.
> + */
> +enum ovs_emit_sample_attr {
> + OVS_EMIT_SAMPLE_ATTR_GROUP = 1, /* u32 number. */
> + OVS_EMIT_SAMPLE_ATTR_COOKIE,/* Optional, user specified cookie. */
> +
> + /* private: */
> + __OVS_EMIT_SAMPLE_ATTR_MAX
> +};
> +
> +#define OVS_EMIT_SAMPLE_ATTR_MAX (__OVS_EMIT_SAMPLE_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_EMIT_SAMPLE: 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_EMIT_SAMPLE, /* Nested OVS_EMIT_SAMPLE_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..1f555cbba312 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
Re: [ovs-dev] [RFC v2 5/9] ofproto-dpif-xlate: Use emit_sample for local sample.
On Wed, Jun 26, 2024 at 11:28:57AM GMT, Ilya Maximets wrote: > On 6/26/24 07:14, Adrián Moreno wrote: > > On Tue, Jun 25, 2024 at 11:04:42PM GMT, Ilya Maximets wrote: > >> On 6/25/24 22:53, Adrián Moreno wrote: > >>> On Mon, Jun 24, 2024 at 07:43:32PM GMT, Adrián Moreno wrote: > On Mon, Jun 24, 2024 at 04:05:36PM GMT, Ilya Maximets wrote: > > On 6/24/24 15:19, Adrián Moreno wrote: > >> On Mon, Jun 24, 2024 at 01:37:44PM GMT, Ilya Maximets wrote: > >>> On 6/5/24 22:23, Adrian Moreno wrote: > Use the newly added emit_sample to implement OpenFlow sample() > actions > with local sampling configuration. > > Signed-off-by: Adrian Moreno > --- > ofproto/ofproto-dpif-lsample.c | 17 > ofproto/ofproto-dpif-lsample.h | 6 ++ > ofproto/ofproto-dpif-xlate.c | 163 > - > ofproto/ofproto-dpif-xlate.h | 3 +- > ofproto/ofproto-dpif.c | 2 +- > 5 files changed, 144 insertions(+), 47 deletions(-) > >>> > >>> Not a full review, just a note that this patch needs tests in > >>> ofproto-dpif.at > >>> that would check that we generate correct datapath flows. > >>> > >> > >> I thought about that, but ofproto-dpif.at is based on dummy datapat > >> (userspace) which does not support this action. The configuration will > >> be ignored and the datapath actions will not be generated. That's why I > >> relied on system-traffic.at tests. Do you see any way around this? > > > > We don't need actual datapath support if we're not sending any actual > > trafic. You should be able to just turn on the capability and check > > that ofproto/trace generates correct actions. > > > > We test kernel tunnels this way, for example. See the macro > > OVS_VSWITCHD_DISABLE_TUNNEL_PUSH_POP. And some other similar checks. > > > > >>> > >>> Tried it but it doesn't seem to work. I now remember I hit this issue > >>> originally :-). > >>> We cannot enable a capability beyond the datapath's "boot time" > >>> capabilities. > >>> > >>> If you try to run > >>> "ovs-appctl dpif/set-dp-features br0 emit_sample true", the command > >>> fails with: > >>> "Can not enable features not supported by the datapth" > >>> > >>> The safeguard does make sense in general (prevents users from > >>> knee-capping themselves) but for testing maybe we want to force it? > >>> > >>> OTOH, marking this feature as supported in the userspace datapath would > >>> not be catastrophic, we could just warn the action is not supported > >>> in odp_execute(), or consider VLOG_DBG_RL as current implementation > >>> until we come up with a good one. However, I cannot of a good way of > >>> of properly reporting this to the user beforehand so I think it could > >>> still cause a lot of confusion. > >>> > >>> Thoughts? > >> > >> Hmm. This command is generally for testing only or experiments. > >> It's not documented and hence we do not support changing datapath > >> features in runtime. > > > > Oh! It's true that it's docummented, although it does appear in > > "list-commands". I thought we supported disabling features, I guess we > > might on a case-by-case basis and through OVSDB. > > > >> > >> It should be fine to add another command like force-dp-features to > >> overcome the restriction. Just make sure that it is not documented > >> and doesn't appear in the list-commands output. > >> > > > > If "set-dp-features" is already an exceptional path, can't we just add a > > "--force" option to it? > > I guess, it's fine to add a flag. Just don't document it. But, please, > add a scary warning in this case saying that unsupported feature is enabled > and the behavior is unpredictable from this point on. Tests can ignore the > warning. > I will not documment it but I don't think we should remove from "list-commands" either. And sure, I'll add a scary warning. Thanks. Adrián ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC v2 5/9] ofproto-dpif-xlate: Use emit_sample for local sample.
On 6/26/24 07:14, Adrián Moreno wrote: > On Tue, Jun 25, 2024 at 11:04:42PM GMT, Ilya Maximets wrote: >> On 6/25/24 22:53, Adrián Moreno wrote: >>> On Mon, Jun 24, 2024 at 07:43:32PM GMT, Adrián Moreno wrote: On Mon, Jun 24, 2024 at 04:05:36PM GMT, Ilya Maximets wrote: > On 6/24/24 15:19, Adrián Moreno wrote: >> On Mon, Jun 24, 2024 at 01:37:44PM GMT, Ilya Maximets wrote: >>> On 6/5/24 22:23, Adrian Moreno wrote: Use the newly added emit_sample to implement OpenFlow sample() actions with local sampling configuration. Signed-off-by: Adrian Moreno --- ofproto/ofproto-dpif-lsample.c | 17 ofproto/ofproto-dpif-lsample.h | 6 ++ ofproto/ofproto-dpif-xlate.c | 163 - ofproto/ofproto-dpif-xlate.h | 3 +- ofproto/ofproto-dpif.c | 2 +- 5 files changed, 144 insertions(+), 47 deletions(-) >>> >>> Not a full review, just a note that this patch needs tests in >>> ofproto-dpif.at >>> that would check that we generate correct datapath flows. >>> >> >> I thought about that, but ofproto-dpif.at is based on dummy datapat >> (userspace) which does not support this action. The configuration will >> be ignored and the datapath actions will not be generated. That's why I >> relied on system-traffic.at tests. Do you see any way around this? > > We don't need actual datapath support if we're not sending any actual > trafic. You should be able to just turn on the capability and check > that ofproto/trace generates correct actions. > > We test kernel tunnels this way, for example. See the macro > OVS_VSWITCHD_DISABLE_TUNNEL_PUSH_POP. And some other similar checks. > >>> >>> Tried it but it doesn't seem to work. I now remember I hit this issue >>> originally :-). >>> We cannot enable a capability beyond the datapath's "boot time" >>> capabilities. >>> >>> If you try to run >>> "ovs-appctl dpif/set-dp-features br0 emit_sample true", the command >>> fails with: >>> "Can not enable features not supported by the datapth" >>> >>> The safeguard does make sense in general (prevents users from >>> knee-capping themselves) but for testing maybe we want to force it? >>> >>> OTOH, marking this feature as supported in the userspace datapath would >>> not be catastrophic, we could just warn the action is not supported >>> in odp_execute(), or consider VLOG_DBG_RL as current implementation >>> until we come up with a good one. However, I cannot of a good way of >>> of properly reporting this to the user beforehand so I think it could >>> still cause a lot of confusion. >>> >>> Thoughts? >> >> Hmm. This command is generally for testing only or experiments. >> It's not documented and hence we do not support changing datapath >> features in runtime. > > Oh! It's true that it's docummented, although it does appear in > "list-commands". I thought we supported disabling features, I guess we > might on a case-by-case basis and through OVSDB. > >> >> It should be fine to add another command like force-dp-features to >> overcome the restriction. Just make sure that it is not documented >> and doesn't appear in the list-commands output. >> > > If "set-dp-features" is already an exceptional path, can't we just add a > "--force" option to it? I guess, it's fine to add a flag. Just don't document it. But, please, add a scary warning in this case saying that unsupported feature is enabled and the behavior is unpredictable from this point on. Tests can ignore the warning. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH]ipf: Fix ovs ipf crash.
hi, llya and Michael
1. The stack trace can be found in attachment,the position of the crash is not
fixed,is random.
2. For packets that have been successfully reassembled and have not timed
out,they will exist
in the frag_complete_list and can be added to the batch;For packets that have
not yet been
successfully reassembled, as long as the timeout does not exceed
IPF-FRAG-LIST-TIMEOUT+
IPF-FRAG-LIST-PURGE-TIME-ADJ,all fragments of the packet can still be added to
the batch
once the reassembly is successful.
As follow is the crash reproduction method,It is occasional, but the
probability is relatively high.
netperf performance test command:
client:
#!/bin/bash
server_ip=192.168.1.3
for j in `seq 64`; do
port=$[16000+j]
netperf -H ${server_ip} -l ${1:-300} -t UDP_STREAM -p $port > /dev/null
2>&1 &
done
server:
#!/bin/bash
for j in `seq 64`; do
netserver -p $[16000+j] > server_$[16000+j].netperf > /dev/null 2>&1 &
done
At 2024-05-28 23:16:46, "Ilya Maximets" wrote:
>On 5/23/24 09:40, laixiangwu wrote:
>> Description:
>>
>> when 1) The fragment timeout is between 15 seconds and 25 seconds; 2)
>> The ipf_list currently has received more than 32 fragments, and there
>> are other fragments of same big packet that have not been received.
>>
>> When the above two scenario conditions are met, due to exceeding the
>> capacity of the packet batch(here is 32), ipf_dp_packet_batch_add
>> returns false, and ipf_list will not be cleared. However, the 32
>> fragments packets added to the packet batch will be processed normally.
>> When receiving the subsequent fragments of the ipf_list, because the
>> first 32 fragments have been processed, when processing subsequent
>> fragment packets, relevant information about the processed fragment
>> packets will be read，therefore will occur carsh.
>> One solution is do not forward timeout fragment packets from the above
>> scenarios, that is, do not add them to the packet batch, and handle
>> other scenarios according to the original logic.
>> Signed-off-by: laixiangwu <15310488...@163.com>
>> ---
>> lib/ipf.c | 10 --
>> 1 file changed, 4 insertions(+), 6 deletions(-)
>
>Hi, laixiangwu. This version of the patch looks the same as the
>previous one here:
>
> https://patchwork.ozlabs.org/project/openvswitch/patch/20240522021957.2292-1-15310488...@163.com/
>
>And I see Mike asked a few questions for the approach there.
>Could you, please, answer those?
>
>For now, I'll mark this patch with 'Changes Requested'.
>
>If you plan to send a new version based on Mike's comments, please, add
>'v6' to the subject prefix, i.e. [PATCH v6], since it's technically a
>6th version of it.
>
>Best regards, Ilya Maximets.
Thread 22 (Thread 0x7f03f504b000 (LWP 36639)):
#0 0x7f03e36ec20d in fts_read () from /lib64/libc.so.6
No symbol table info available.
#1 0x0262cd40 in ?? ()
No symbol table info available.
#2 0x02444940 in ?? ()
No symbol table info available.
#3 0x7ffcf35bacd8 in ?? ()
No symbol table info available.
#4 0x in ?? ()
No symbol table info available.
Thread 21 (Thread 0x7f03cf7c0700 (LWP 36661)):
#0 0x7f03e36ec20d in fts_read () from /lib64/libc.so.6
No symbol table info available.
#1 0x7f03b00240d0 in ?? ()
No symbol table info available.
#2 0x7f03ba60 in ?? ()
No symbol table info available.
#3 0x02582e10 in ?? ()
No symbol table info available.
#4 0x00801001 in ?? ()
No symbol table info available.
#5 0x000a in ?? ()
No symbol table info available.
#6 0x00023aeace9d in ?? ()
No symbol table info available.
#7 0x7f03e510333c in poll_block () at lib/poll-loop.c:364
loop = 0x20
node = 0x0
pollfds = 0x7f03b020
elapsed = 0
retval =
i =
#8 0x7f03e56bc3c7 in udpif_revalidator (arg=0x2582e10) at
ofproto/ofproto-dpif-upcall.c:960
flow_limit =
duration =
revalidator = 0x2582e10
udpif = 0x2487430
leader = true
start_time = 9578401437
---Type to continue, or q to quit---
last_reval_seq =
n_flows = 10
#9 0x7f03e50ed5cf in ovsthread_wrapper (aux_=) at
lib/ovs-thread.c:354
auxp =
aux = {start = 0x7f03e56bc200 , arg = 0x2582e10,
name = "revalidator\000\000\000\000"}
id = 7
subprogram_name = 0x7f03b8c0 ""
#10 0x7f03e4419dd5 in start_thread () from /lib64/libpthread.so.0
No symbol table info available.
#11 0x7f03e36f6ead in __libc_ifunc_impl_list () from /lib64/libc.so.6
No symbol table info available.
#12 0x in ?? ()
No symbol table info available.
Thread 20 (Thread 0x7f03de898700 (LWP 36657)):
#0 0x7f03e36ec20d in fts_read () from /lib64/libc.so.6
No symbol table info available.
#1 0x7f03c40008c0 in ?? ()
No symbol table info available.
#2 0x7f03c4000970 in ?? ()
No symbol table info available.
#3 0x7f03d7fc2da0 in ?? ()
No symbol tab
Re: [ovs-dev] [PATCH]ipf: Fix ovs ipf crash.
hi, llya and Michael
1. The stack trace can be found in attachment, the position of the crash is not
fixed, is random.
2. For packets that have been successfully reassembled and have not timed out,
they will exist in the frag_complete_list and can be added to the batch；For
packets that have not yet been successfully reassembled, as long as the timeout
does not exceed IPF-FRAG-LIST-TIMEOUT+IPF-FRAG-LIST-PURGE-TIME-ADJ, all
fragments of the packet can still be added to the batch once the reassembly is
successful.
As follow is the crash reproduction method，It is occasional, but the
probability is relatively high.
netperf performance test command：
client：
#!/bin/bash
server_ip=192.168.1.3
for j in `seq 64`; do
port=$[16000+j]
netperf -H ${server_ip} -l ${1:-300} -t UDP_STREAM -p $port > /dev/null
2>&1 &
done
server：
#!/bin/bash
for j in `seq 64`; do
netserver -p $[16000+j] > server_$[16000+j].netperf > /dev/null 2>&1 &
done
--
它山之石，可以攻玉。
At 2024-05-28 23:16:46, "Ilya Maximets" wrote:
>On 5/23/24 09:40, laixiangwu wrote:
>> Description:
>>
>> when 1) The fragment timeout is between 15 seconds and 25 seconds; 2)
>> The ipf_list currently has received more than 32 fragments, and there
>> are other fragments of same big packet that have not been received.
>>
>> When the above two scenario conditions are met, due to exceeding the
>> capacity of the packet batch(here is 32), ipf_dp_packet_batch_add
>> returns false, and ipf_list will not be cleared. However, the 32
>> fragments packets added to the packet batch will be processed normally.
>> When receiving the subsequent fragments of the ipf_list, because the
>> first 32 fragments have been processed, when processing subsequent
>> fragment packets, relevant information about the processed fragment
>> packets will be read，therefore will occur carsh.
>> One solution is do not forward timeout fragment packets from the above
>> scenarios, that is, do not add them to the packet batch, and handle
>> other scenarios according to the original logic.
>> Signed-off-by: laixiangwu <15310488...@163.com>
>> ---
>> lib/ipf.c | 10 --
>> 1 file changed, 4 insertions(+), 6 deletions(-)
>
>Hi, laixiangwu. This version of the patch looks the same as the
>previous one here:
>
> https://patchwork.ozlabs.org/project/openvswitch/patch/20240522021957.2292-1-15310488...@163.com/
>
>And I see Mike asked a few questions for the approach there.
>Could you, please, answer those?
>
>For now, I'll mark this patch with 'Changes Requested'.
>
>If you plan to send a new version based on Mike's comments, please, add
>'v6' to the subject prefix, i.e. [PATCH v6], since it's technically a
>6th version of it.
>
>Best regards, Ilya Maximets.
Thread 22 (Thread 0x7f03f504b000 (LWP 36639)):
#0 0x7f03e36ec20d in fts_read () from /lib64/libc.so.6
No symbol table info available.
#1 0x0262cd40 in ?? ()
No symbol table info available.
#2 0x02444940 in ?? ()
No symbol table info available.
#3 0x7ffcf35bacd8 in ?? ()
No symbol table info available.
#4 0x in ?? ()
No symbol table info available.
Thread 21 (Thread 0x7f03cf7c0700 (LWP 36661)):
#0 0x7f03e36ec20d in fts_read () from /lib64/libc.so.6
No symbol table info available.
#1 0x7f03b00240d0 in ?? ()
No symbol table info available.
#2 0x7f03ba60 in ?? ()
No symbol table info available.
#3 0x02582e10 in ?? ()
No symbol table info available.
#4 0x00801001 in ?? ()
No symbol table info available.
#5 0x000a in ?? ()
No symbol table info available.
#6 0x00023aeace9d in ?? ()
No symbol table info available.
#7 0x7f03e510333c in poll_block () at lib/poll-loop.c:364
loop = 0x20
node = 0x0
pollfds = 0x7f03b020
elapsed = 0
retval =
i =
#8 0x7f03e56bc3c7 in udpif_revalidator (arg=0x2582e10) at
ofproto/ofproto-dpif-upcall.c:960
flow_limit =
duration =
revalidator = 0x2582e10
udpif = 0x2487430
leader = true
start_time = 9578401437
---Type to continue, or q to quit---
last_reval_seq =
n_flows = 10
#9 0x7f03e50ed5cf in ovsthread_wrapper (aux_=) at
lib/ovs-thread.c:354
auxp =
aux = {start = 0x7f03e56bc200 , arg = 0x2582e10,
name = "revalidator\000\000\000\000"}
id = 7
subprogram_name = 0x7f03b8c0 ""
#10 0x7f03e4419dd5 in start_thread () from /lib64/libpthread.so.0
No symbol table info available.
#11 0x7f03e36f6ead in __libc_ifunc_impl_list () from /lib64/libc.so.6
No symbol table info available.
#12 0x in ?? ()
No symbol table info available.
Thread 20 (Thread 0x7f03de898700 (LWP 36657)):
#0 0x7f03e36ec20d in fts_read () from /lib64/libc.so.6
No symbol table info available.
#1 0x7f03c40008c0 in ?? ()
No symbol table info available.
#2 0x7f03c4000970 in ?? ()
No symbol table info available.
#3 0x7f03d7fc
Re: [ovs-dev] [PATCH net-next v5 04/10] net: psample: allow using rate as probability
On Tue, Jun 25, 2024 at 10:51:47PM +0200, Adrian Moreno wrote: > 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. > > Signed-off-by: Adrian Moreno Reviewed-by: Ido Schimmel ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next v5 02/10] net: sched: act_sample: add action cookie to sample
On Tue, Jun 25, 2024 at 10:51:45PM +0200, Adrian Moreno wrote: > If the action has a user_cookie, pass it along to the sample so it can > be easily identified. > > Signed-off-by: Adrian Moreno Reviewed-by: Ido Schimmel ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next v5 03/10] net: psample: skip packet copy if no listeners
On Tue, Jun 25, 2024 at 10:51:46PM +0200, Adrian Moreno wrote: > 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: Simon Horman > Signed-off-by: Adrian Moreno Reviewed-by: Ido Schimmel ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next v5 01/10] net: psample: add user cookie
On Tue, Jun 25, 2024 at 10:51:44PM +0200, Adrian Moreno wrote: > 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: Simon Horman > Signed-off-by: Adrian Moreno Reviewed-by: Ido Schimmel ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
