date:20240514

On Tue, May 14, 2024 at 3:19 PM Ilya Maximets  wrote:

> On 5/14/24 10:38, Ales Musil wrote:
> > There were two things missing for the Fedora builds, 32-bit
> > version of glibc to allows the -m32 compilation on Fedora
> > and numactl-devel package.
> >
> > Signed-off-by: Ales Musil 
> > ---
> >  .ci/linux-build.sh | 3 +++
> >  utilities/containers/fedora/Dockerfile | 1 +
> >  2 files changed, 4 insertions(+)
> >
> > diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
> > index 78f17f8bd..12966f532 100755
> > --- a/.ci/linux-build.sh
> > +++ b/.ci/linux-build.sh
> > @@ -83,6 +83,9 @@ function configure_gcc()
> >  # do it directly because gcc-multilib is not available
> >  # for arm64
> >  sudo apt update && sudo apt install -y gcc-multilib
> > +elif which dnf; then
> > +# Install equivalent of gcc-multilib for Fedora.
> > +sudo dnf -y update && sudo dnf -y install glibc-devel.i686
>
> dnf always refreshes package cache.  'dnf update' will actually update
> all the packages to the latest versions.  I'm not sure it is an intended
> behavior here.
>

Yeah good point, we shouldn't update the packages just install the missing
one. So only the install part is needed, I'll wait for other reviews before
posting v2.


>
> >  fi
> >  fi
> >  }
> > diff --git a/utilities/containers/fedora/Dockerfile
> b/utilities/containers/fedora/Dockerfile
> > index 9b8386aae..d40a7b31f 100755
> > --- a/utilities/containers/fedora/Dockerfile
> > +++ b/utilities/containers/fedora/Dockerfile
> > @@ -28,6 +28,7 @@ RUN dnf -y update \
> >  libtool \
> >  net-tools \
> >  nmap-ncat \
> > +numactl-devel \
> >  openssl \
> >  openssl-devel \
> >  procps-ng \
>
>
Thanks,
Ales
-- 

Ales Musil

Senior Software Engineer - OVN Core

Red Hat EMEA 

amu...@redhat.com

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

Re: [ovs-dev] [PATCH ovn 5/7] ci: Add missing packages to run Fedora image in GH CI.

On 5/14/24 10:38, Ales Musil wrote:
> There were two things missing for the Fedora builds, 32-bit
> version of glibc to allows the -m32 compilation on Fedora
> and numactl-devel package.
> 
> Signed-off-by: Ales Musil 
> ---
>  .ci/linux-build.sh | 3 +++
>  utilities/containers/fedora/Dockerfile | 1 +
>  2 files changed, 4 insertions(+)
> 
> diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
> index 78f17f8bd..12966f532 100755
> --- a/.ci/linux-build.sh
> +++ b/.ci/linux-build.sh
> @@ -83,6 +83,9 @@ function configure_gcc()
>  # do it directly because gcc-multilib is not available
>  # for arm64
>  sudo apt update && sudo apt install -y gcc-multilib
> +elif which dnf; then
> +# Install equivalent of gcc-multilib for Fedora.
> +sudo dnf -y update && sudo dnf -y install glibc-devel.i686

dnf always refreshes package cache.  'dnf update' will actually update
all the packages to the latest versions.  I'm not sure it is an intended
behavior here.

>  fi
>  fi
>  }
> diff --git a/utilities/containers/fedora/Dockerfile 
> b/utilities/containers/fedora/Dockerfile
> index 9b8386aae..d40a7b31f 100755
> --- a/utilities/containers/fedora/Dockerfile
> +++ b/utilities/containers/fedora/Dockerfile
> @@ -28,6 +28,7 @@ RUN dnf -y update \
>  libtool \
>  net-tools \
>  nmap-ncat \
> +numactl-devel \
>  openssl \
>  openssl-devel \
>  procps-ng \

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

Re: [ovs-dev] [PATCH v2] utilities: Correct deletion reason in flow_reval_monitor.py.




On 13 May 2024, at 17:02, Ilya Maximets wrote:

> On 5/8/24 11:19, Eelco Chaudron wrote:
>> The flow_reval_monitor.py script incorrectly reported the reasons for
>> FDR_PURGE and FDR_TOO_EXPENSIVE, as their descriptions were swapped.
>> This patch rectifies the order using a dictionary to avoid similar
>> problems in the future.
>>
>> In addition this patch also syncs the delete reason output of the
>> script, with the comments in the code.
>>
>> Fixes: 86b9e653ef22 ("revalidator: Add a USDT probe during flow deletion 
>> with purge reason.")
>> Signed-off-by: Eelco Chaudron 
>>
>> ---
>> v2: - Converted the list of strings to dictionary.
>> - Added comment to code to keep code and script in sync.
>> - Unified flow_delete reason comments and script output.
>> ---
>>  ofproto/ofproto-dpif-upcall.c| 25 ---
>>  utilities/usdt-scripts/flow_reval_monitor.py | 32 ++--
>>  2 files changed, 30 insertions(+), 27 deletions(-)
>>
>> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
>> index 73901b651..e4d348985 100644
>> --- a/ofproto/ofproto-dpif-upcall.c
>> +++ b/ofproto/ofproto-dpif-upcall.c
>> @@ -1,3 +1,4 @@
>> +
>>  /* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016 Nicira, Inc.
>>   *
>>   * Licensed under the Apache License, Version 2.0 (the "License");
>> @@ -270,18 +271,20 @@ enum ukey_state {
>>  };
>>  #define N_UKEY_STATES (UKEY_DELETED + 1)
>>
>> +/* Ukey delete reasons used by USDT probes.  Please keep in sync with the
>> + * definition in utilities/usdt-scripts/flow_reval_monitor.py.  */
>>  enum flow_del_reason {
>> -FDR_NONE = 0,   /* No deletion reason for the flow. */
>> -FDR_AVOID_CACHING,  /* Flow deleted to avoid caching. */
>> -FDR_BAD_ODP_FIT,/* The flow had a bad ODP flow fit. */
>> -FDR_FLOW_IDLE,  /* The flow went unused and was deleted. */
>> -FDR_FLOW_LIMIT, /* All flows being killed. */
>> -FDR_FLOW_WILDCARDED,/* The flow needed a narrower wildcard mask. */
>> -FDR_NO_OFPROTO, /* The flow didn't have an associated ofproto. 
>> */
>> -FDR_PURGE,  /* User action caused flows to be killed. */
>> -FDR_TOO_EXPENSIVE,  /* The flow was too expensive to revalidate. */
>> -FDR_UPDATE_FAIL,/* Flow state transition was unexpected. */
>> -FDR_XLATION_ERROR,  /* There was an error translating the flow. */
>> +FDR_NONE = 0,   /* No delete reason specified. */
>> +FDR_AVOID_CACHING,  /* Cache avoidance flag set. */
>> +FDR_BAD_ODP_FIT,/* Bad ODP flow fit. */
>> +FDR_FLOW_IDLE,  /* Flow idle timeout. */
>> +FDR_FLOW_LIMIT, /* Kill all flows condition reached. */
>> +FDR_FLOW_WILDCARDED,/* Flow needs a narrower wildcard mask. */
>> +FDR_NO_OFPROTO, /* Flow lacks ofproto dpif association. */
>> +FDR_PURGE,  /* User requested flow deletion. */
>> +FDR_TOO_EXPENSIVE,  /* Too expensive to revalidate. */
>> +FDR_UPDATE_FAIL,/* Datapath update failed. */
>> +FDR_XLATION_ERROR,  /* Flow translation error. */
>>  };
>>
>>  /* 'udpif_key's are responsible for tracking the little bit of state udpif
>> diff --git a/utilities/usdt-scripts/flow_reval_monitor.py 
>> b/utilities/usdt-scripts/flow_reval_monitor.py
>> index 534ba8fa2..bc3a28443 100755
>> --- a/utilities/usdt-scripts/flow_reval_monitor.py
>> +++ b/utilities/usdt-scripts/flow_reval_monitor.py
>> @@ -254,19 +254,19 @@ FdrReasons = IntEnum(
>>  start=0,
>>  )
>>
>> -FdrReasonStrings = [
>> -"No deletion reason",
>> -"Cache avoidance flag set",
>> -"Bad ODP flow fit",
>> -"Idle flow timed out",
>> -"Kill all flows condition detected",
>> -"Mask too wide - need narrower match",
>> -"No matching ofproto rules",
>> -"Too expensive to revalidate",
>> -"Purged with user action",
>> -"Flow state inconsistent after updates",
>> -"Flow translation error",
>> -]
>
> Should we also have a comment here describing from where these are
> coming from?
>
>> +FdrReasonStrings = {
>> +FdrReasons.FDR_NONE: "No delete reason specified",
>> +FdrReasons.FDR_AVOID_CACHING: "Cache avoidance flag set",
>> +FdrReasons.FDR_BAD_ODP_FIT: "Bad ODP flow fit",
>> +FdrReasons.FDR_FLOW_IDLE: "Flow idle timeout",
>> +FdrReasons.FDR_FLOW_LIMIT: "Kill all flows condition reached",
>> +FdrReasons.FDR_FLOW_WILDCARDED: "Flow needs a narrower wildcard mask",
>> +FdrReasons.FDR_NO_OFPROTO: "Flow lacks ofproto dpif association",
>
> Can we rename this one into "Bridge not found" maybe?
>
> 'ofproto' and 'dpif' are not user-friendly words.  'ofproto' is an entirely
> internal concept.

ACK on both. Just sent out a v3.

//Eelco

>> +FdrReasons.FDR_PURGE: "User requested flow deletion",
>> +FdrReasons.FDR_TOO_EXPENSIVE: "Too expensive to revalidate",
>> +FdrReasons.FDR_UPDATE_FAIL: "Datapa

[ovs-dev] [PATCH v3] utilities: Correct deletion reason in flow_reval_monitor.py.

The flow_reval_monitor.py script incorrectly reported the reasons for
FDR_PURGE and FDR_TOO_EXPENSIVE, as their descriptions were swapped.
This patch rectifies the order using a dictionary to avoid similar
problems in the future.

In addition this patch also syncs the delete reason output of the
script, with the comments in the code.

Fixes: 86b9e653ef22 ("revalidator: Add a USDT probe during flow deletion with 
purge reason.")
Signed-off-by: Eelco Chaudron 

---
v3: - Renamed ofproto dpif to bridge in delete reasons.
- Added comment pointing back to delete reasons in .c.
v2: - Converted the list of strings to dictionary.
- Added comment to code to keep code and script in sync.
- Unified flow_delete reason comments and script output.
---
 ofproto/ofproto-dpif-upcall.c| 24 +++--
 utilities/usdt-scripts/flow_reval_monitor.py | 37 +++-
 2 files changed, 34 insertions(+), 27 deletions(-)

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 73901b651..83609ec62 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -270,18 +270,20 @@ enum ukey_state {
 };
 #define N_UKEY_STATES (UKEY_DELETED + 1)
 
+/* Ukey delete reasons used by USDT probes.  Please keep in sync with the
+ * definition in utilities/usdt-scripts/flow_reval_monitor.py.  */
 enum flow_del_reason {
-FDR_NONE = 0,   /* No deletion reason for the flow. */
-FDR_AVOID_CACHING,  /* Flow deleted to avoid caching. */
-FDR_BAD_ODP_FIT,/* The flow had a bad ODP flow fit. */
-FDR_FLOW_IDLE,  /* The flow went unused and was deleted. */
-FDR_FLOW_LIMIT, /* All flows being killed. */
-FDR_FLOW_WILDCARDED,/* The flow needed a narrower wildcard mask. */
-FDR_NO_OFPROTO, /* The flow didn't have an associated ofproto. */
-FDR_PURGE,  /* User action caused flows to be killed. */
-FDR_TOO_EXPENSIVE,  /* The flow was too expensive to revalidate. */
-FDR_UPDATE_FAIL,/* Flow state transition was unexpected. */
-FDR_XLATION_ERROR,  /* There was an error translating the flow. */
+FDR_NONE = 0,   /* No delete reason specified. */
+FDR_AVOID_CACHING,  /* Cache avoidance flag set. */
+FDR_BAD_ODP_FIT,/* Bad ODP flow fit. */
+FDR_FLOW_IDLE,  /* Flow idle timeout. */
+FDR_FLOW_LIMIT, /* Kill all flows condition reached. */
+FDR_FLOW_WILDCARDED,/* Flow needs a narrower wildcard mask. */
+FDR_NO_OFPROTO, /* Bridge not found. */
+FDR_PURGE,  /* User requested flow deletion. */
+FDR_TOO_EXPENSIVE,  /* Too expensive to revalidate. */
+FDR_UPDATE_FAIL,/* Datapath update failed. */
+FDR_XLATION_ERROR,  /* Flow translation error. */
 };
 
 /* 'udpif_key's are responsible for tracking the little bit of state udpif
diff --git a/utilities/usdt-scripts/flow_reval_monitor.py 
b/utilities/usdt-scripts/flow_reval_monitor.py
index 534ba8fa2..28479a565 100755
--- a/utilities/usdt-scripts/flow_reval_monitor.py
+++ b/utilities/usdt-scripts/flow_reval_monitor.py
@@ -236,6 +236,11 @@ RevalResult = IntEnum(
 ],
 start=0,
 )
+
+#
+# The below FdrReasons and FdrReasonStrings definitions can be found in the
+# ofproto/ofproto-dpif-upcall.c file.  Please keep them in sync.
+#
 FdrReasons = IntEnum(
 "flow_del_reason",
 [
@@ -254,19 +259,19 @@ FdrReasons = IntEnum(
 start=0,
 )
 
-FdrReasonStrings = [
-"No deletion reason",
-"Cache avoidance flag set",
-"Bad ODP flow fit",
-"Idle flow timed out",
-"Kill all flows condition detected",
-"Mask too wide - need narrower match",
-"No matching ofproto rules",
-"Too expensive to revalidate",
-"Purged with user action",
-"Flow state inconsistent after updates",
-"Flow translation error",
-]
+FdrReasonStrings = {
+FdrReasons.FDR_NONE: "No delete reason specified",
+FdrReasons.FDR_AVOID_CACHING: "Cache avoidance flag set",
+FdrReasons.FDR_BAD_ODP_FIT: "Bad ODP flow fit",
+FdrReasons.FDR_FLOW_IDLE: "Flow idle timeout",
+FdrReasons.FDR_FLOW_LIMIT: "Kill all flows condition reached",
+FdrReasons.FDR_FLOW_WILDCARDED: "Flow needs a narrower wildcard mask",
+FdrReasons.FDR_NO_OFPROTO: "Bridge not found",
+FdrReasons.FDR_PURGE: "User requested flow deletion",
+FdrReasons.FDR_TOO_EXPENSIVE: "Too expensive to revalidate",
+FdrReasons.FDR_UPDATE_FAIL: "Datapath update failed",
+FdrReasons.FDR_XLATION_ERROR: "Flow translation error"
+}
 
 
 def err(msg, code=-1):
@@ -572,10 +577,10 @@ def print_expiration(event):
 """Prints a UFID eviction with a reason."""
 ufid_str = format_ufid(event.ufid)
 
-if event.reason > len(FdrReasons):
-reason = f"Unknown reason '{event.reason}'"
-else:
+try:
 reason = FdrReasonStrings[event.reason]
+except KeyError:
+reason = f"Unknown reason '{event.

Re: [ovs-dev] [RFC 01/11] odp-util: Add support to new psample uAPI.



On 14 May 2024, at 14:50, Ilya Maximets wrote:

> On 5/14/24 14:43, Eelco Chaudron wrote:
>>
>>
>> On 14 May 2024, at 14:28, Ilya Maximets wrote:
>>
>>> On 5/14/24 13:27, Eelco Chaudron wrote:


 On 14 May 2024, at 13:05, Ilya Maximets wrote:

> On 5/14/24 12:14, Adrian Moreno wrote:
>>
>>
>> On 5/14/24 11:09 AM, Ilya Maximets wrote:
>>> On 5/14/24 09:39, Adrian Moreno wrote:


 On 5/10/24 12:45 PM, Adrian Moreno wrote:
>
>
> On 5/10/24 12:06 PM, Eelco Chaudron wrote:
>> On 24 Apr 2024, at 21:53, Adrian Moreno wrote:
>>
>>> The new odp sample attributes allow userspace to specify a group_id 
>>> and
>>> user-defined cookie to be passed down to psample.
>>>
>>> Add support for parsing and formatting such action.
>>>
>>> Signed-off-by: Adrian Moreno 
>>
>> Hi Adrian,
>>
>> See my comments below inline.
>>
>> //Eelco
>>
>>> ---
>>>    include/linux/openvswitch.h  |  49 +---
>>>    lib/odp-execute.c    |   3 +
>>>    lib/odp-util.c   | 150 
>>> ++-
>>>    ofproto/ofproto-dpif-ipfix.c |   2 +
>>>    python/ovs/flow/odp.py   |   2 +
>>>    tests/odp.at |   5 ++
>>>    6 files changed, 163 insertions(+), 48 deletions(-)
>>>
>>> diff --git a/include/linux/openvswitch.h 
>>> b/include/linux/openvswitch.h
>>> index d9fb991ef..3e748405c 100644
>>> --- a/include/linux/openvswitch.h
>>> +++ b/include/linux/openvswitch.h
>>> @@ -696,6 +696,7 @@ enum ovs_flow_attr {
>>>    #define OVS_UFID_F_OMIT_MASK (1 << 1)
>>>    #define OVS_UFID_F_OMIT_ACTIONS  (1 << 2)
>>>
>>> +#define OVS_PSAMPLE_COOKIE_MAX_SIZE 16
>>>    /**
>>>     * enum ovs_sample_attr - Attributes for %OVS_ACTION_ATTR_SAMPLE 
>>> action.
>>>     * @OVS_SAMPLE_ATTR_PROBABILITY: 32-bit fraction of packets to 
>>> sample with
>>> @@ -703,15 +704,27 @@ enum ovs_flow_attr {
>>>     * %UINT32_MAX samples all packets and intermediate values 
>>> sample intermediate
>>>     * fractions of packets.
>>>     * @OVS_SAMPLE_ATTR_ACTIONS: Set of actions to execute in 
>>> sampling event.
>>> - * Actions are passed as nested attributes.
>>> + * Actions are passed as nested attributes. Optional if
>>> + * OVS_SAMPLE_ATTR_PSAMPLE_GROUP is set.
>>> + * @OVS_SAMPLE_ATTR_PSAMPLE_GROUP: A 32-bit number to be used as 
>>> psample group.
>>> + * Optional if OVS_SAMPLE_ATTR_ACTIONS is set.
>>> + * @OVS_SAMPLE_ATTR_PSAMPLE_COOKIE: A variable-length binary 
>>> cookie that, if
>>> + * provided, will be copied to the psample cookie.
>>
>> Should we mention any limit on the actual length of the cookie?
>>
>> Any reason we explicitly call this psample? From an OVS perspective, 
>> this
>> could just be
>> SAMPLE_ATTR_FORWARD/OFFLOAD/DESTINATION/ENDPOINT/COLLECTOR/TARGET_COOKIE
>>  and
>> _GROUP. Other datapaths, do not have PSAMPLE.
>>
>
> See my response to your comment on the cover-letter for possible name 
> alternatives.
>
>
>> Thinking about it more, from an OVS perspective we should probably 
>> not even
>> send down a COOKIE, but we should send down an obs_domain_id and 
>> obs_point_id,
>> and then the kernel can move this into a cookie.
>>
>
> I did consider this. However, the opaque cookie fits well with the tc 
> API which
> does not have two 32-bit values but an opaque 128-bit cookie. So if 
> the action
> is offloaded OVS needs to "encode" the two 32-bit values into the 
> cookie anyways.
>
>> The collector itself could be called system/local collector, or 
>> something like
>> that. This way it can use for example psample for kernel and UDST 
>> probes for
>> usespace. Both can pass the group and cookie (obs_domain_id and 
>> obs_point_id).
>>
>> Also, the presence of any of this should not dictate the psample 
>> action, we
>> probably need a specific OVS_SAMPLE_ATTR_SYSTEM_TARGET type nl flag.
>>
>
> It clearer and it might simplify option verification a bit, but isn't 
> a bit
> redundant? There is no flag for action execution for instance, the 
> presence of
> the attribute is enough.
>
> An alternative would be to have nested attributes, e.g:
>
> enum ovs_sample_attr {
>

Re: [ovs-dev] [RFC 01/11] odp-util: Add support to new psample uAPI.




On 5/14/24 1:27 PM, Eelco Chaudron wrote:



On 14 May 2024, at 13:05, Ilya Maximets wrote:


On 5/14/24 12:14, Adrian Moreno wrote:



On 5/14/24 11:09 AM, Ilya Maximets wrote:

On 5/14/24 09:39, Adrian Moreno wrote:



On 5/10/24 12:45 PM, Adrian Moreno wrote:



On 5/10/24 12:06 PM, Eelco Chaudron wrote:

On 24 Apr 2024, at 21:53, Adrian Moreno wrote:


The new odp sample attributes allow userspace to specify a group_id and
user-defined cookie to be passed down to psample.

Add support for parsing and formatting such action.

Signed-off-by: Adrian Moreno 


Hi Adrian,

See my comments below inline.

//Eelco


---
    include/linux/openvswitch.h  |  49 +---
    lib/odp-execute.c    |   3 +
    lib/odp-util.c   | 150 ++-
    ofproto/ofproto-dpif-ipfix.c |   2 +
    python/ovs/flow/odp.py   |   2 +
    tests/odp.at |   5 ++
    6 files changed, 163 insertions(+), 48 deletions(-)

diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
index d9fb991ef..3e748405c 100644
--- a/include/linux/openvswitch.h
+++ b/include/linux/openvswitch.h
@@ -696,6 +696,7 @@ enum ovs_flow_attr {
    #define OVS_UFID_F_OMIT_MASK (1 << 1)
    #define OVS_UFID_F_OMIT_ACTIONS  (1 << 2)

+#define OVS_PSAMPLE_COOKIE_MAX_SIZE 16
    /**
     * enum ovs_sample_attr - Attributes for %OVS_ACTION_ATTR_SAMPLE action.
     * @OVS_SAMPLE_ATTR_PROBABILITY: 32-bit fraction of packets to sample with
@@ -703,15 +704,27 @@ enum ovs_flow_attr {
     * %UINT32_MAX samples all packets and intermediate values sample 
intermediate
     * fractions of packets.
     * @OVS_SAMPLE_ATTR_ACTIONS: Set of actions to execute in sampling event.
- * Actions are passed as nested attributes.
+ * Actions are passed as nested attributes. Optional if
+ * OVS_SAMPLE_ATTR_PSAMPLE_GROUP is set.
+ * @OVS_SAMPLE_ATTR_PSAMPLE_GROUP: A 32-bit number to be used as psample group.
+ * Optional if OVS_SAMPLE_ATTR_ACTIONS is set.
+ * @OVS_SAMPLE_ATTR_PSAMPLE_COOKIE: A variable-length binary cookie that, if
+ * provided, will be copied to the psample cookie.


Should we mention any limit on the actual length of the cookie?

Any reason we explicitly call this psample? From an OVS perspective, this
could just be
SAMPLE_ATTR_FORWARD/OFFLOAD/DESTINATION/ENDPOINT/COLLECTOR/TARGET_COOKIE and
_GROUP. Other datapaths, do not have PSAMPLE.



See my response to your comment on the cover-letter for possible name 
alternatives.



Thinking about it more, from an OVS perspective we should probably not even
send down a COOKIE, but we should send down an obs_domain_id and obs_point_id,
and then the kernel can move this into a cookie.



I did consider this. However, the opaque cookie fits well with the tc API which
does not have two 32-bit values but an opaque 128-bit cookie. So if the action
is offloaded OVS needs to "encode" the two 32-bit values into the cookie 
anyways.


The collector itself could be called system/local collector, or something like
that. This way it can use for example psample for kernel and UDST probes for
usespace. Both can pass the group and cookie (obs_domain_id and obs_point_id).

Also, the presence of any of this should not dictate the psample action, we
probably need a specific OVS_SAMPLE_ATTR_SYSTEM_TARGET type nl flag.



It clearer and it might simplify option verification a bit, but isn't a bit
redundant? There is no flag for action execution for instance, the presence of
the attribute is enough.

An alternative would be to have nested attributes, e.g:

enum ovs_sample_attr {
   OVS_SAMPLE_ATTR_UNSPEC,
   OVS_SAMPLE_ATTR_PROBABILITY, /* u32 number */
   OVS_SAMPLE_ATTR_ACTIONS, /* Nested OVS_ACTION_ATTR_* attributes. */
   OVS_SAMPLE_ATTR_SYSTEM, /* Nested OVS_SAMPLE_SYSTEM_ATTR_* 
attributes. */

   __OVS_SAMPLE_ATTR_MAX,
};

enum ovs_sample_system_attr {
   OVS_SAMPLE_SYSTEM_ATTR_OBS_DOMAIN,  /* u32 number */
   OVS_SAMPLE_SYSTEM_ATTR_OBS_POINT, /* u32 number */
   OVS_SAMPLE_SYSTEM_ATTR_COOKIE, /* Nested OVS_ACTION_ATTR_*

   __OVS_SSAMPLE_SYSTEM_ATTR_MAX,
};

WDYT?



Another benefit of nested attributes is the easier addition of other
sample-related attributes, I already have a candidate: OVS_SAMPLE_SYSTEM_TRUNC

This will be passed to psample (md->trunc_size) who will truncate the packet
size to the specified value. This can be useful for large packets for which we
are only interested in the headers.

WDYT?

[snip]



Hrm.  This makes me think these should not be attributes of a sample action at 
all,
but rather we should have a separate EMIT_SAMPLE action that just calls 
psample, so
we can combine it with other actions:

sample(..., actions(userspace(...),trunc(100),emit_sample(cookie=...)))

or even:

sample(..., 
actions(clone(trunc(100),emit_sample(cookie=...)),userspace(...)))

This will also simplify the requires_datapath_assistance() check as we can just
check the action and not iterate ove

Re: [ovs-dev] [RFC 01/11] odp-util: Add support to new psample uAPI.

On 5/14/24 14:43, Eelco Chaudron wrote:
> 
> 
> On 14 May 2024, at 14:28, Ilya Maximets wrote:
> 
>> On 5/14/24 13:27, Eelco Chaudron wrote:
>>>
>>>
>>> On 14 May 2024, at 13:05, Ilya Maximets wrote:
>>>
 On 5/14/24 12:14, Adrian Moreno wrote:
>
>
> On 5/14/24 11:09 AM, Ilya Maximets wrote:
>> On 5/14/24 09:39, Adrian Moreno wrote:
>>>
>>>
>>> On 5/10/24 12:45 PM, Adrian Moreno wrote:


 On 5/10/24 12:06 PM, Eelco Chaudron wrote:
> On 24 Apr 2024, at 21:53, Adrian Moreno wrote:
>
>> The new odp sample attributes allow userspace to specify a group_id 
>> and
>> user-defined cookie to be passed down to psample.
>>
>> Add support for parsing and formatting such action.
>>
>> Signed-off-by: Adrian Moreno 
>
> Hi Adrian,
>
> See my comments below inline.
>
> //Eelco
>
>> ---
>>    include/linux/openvswitch.h  |  49 +---
>>    lib/odp-execute.c    |   3 +
>>    lib/odp-util.c   | 150 
>> ++-
>>    ofproto/ofproto-dpif-ipfix.c |   2 +
>>    python/ovs/flow/odp.py   |   2 +
>>    tests/odp.at |   5 ++
>>    6 files changed, 163 insertions(+), 48 deletions(-)
>>
>> diff --git a/include/linux/openvswitch.h 
>> b/include/linux/openvswitch.h
>> index d9fb991ef..3e748405c 100644
>> --- a/include/linux/openvswitch.h
>> +++ b/include/linux/openvswitch.h
>> @@ -696,6 +696,7 @@ enum ovs_flow_attr {
>>    #define OVS_UFID_F_OMIT_MASK (1 << 1)
>>    #define OVS_UFID_F_OMIT_ACTIONS  (1 << 2)
>>
>> +#define OVS_PSAMPLE_COOKIE_MAX_SIZE 16
>>    /**
>>     * enum ovs_sample_attr - Attributes for %OVS_ACTION_ATTR_SAMPLE 
>> action.
>>     * @OVS_SAMPLE_ATTR_PROBABILITY: 32-bit fraction of packets to 
>> sample with
>> @@ -703,15 +704,27 @@ enum ovs_flow_attr {
>>     * %UINT32_MAX samples all packets and intermediate values sample 
>> intermediate
>>     * fractions of packets.
>>     * @OVS_SAMPLE_ATTR_ACTIONS: Set of actions to execute in 
>> sampling event.
>> - * Actions are passed as nested attributes.
>> + * Actions are passed as nested attributes. Optional if
>> + * OVS_SAMPLE_ATTR_PSAMPLE_GROUP is set.
>> + * @OVS_SAMPLE_ATTR_PSAMPLE_GROUP: A 32-bit number to be used as 
>> psample group.
>> + * Optional if OVS_SAMPLE_ATTR_ACTIONS is set.
>> + * @OVS_SAMPLE_ATTR_PSAMPLE_COOKIE: A variable-length binary cookie 
>> that, if
>> + * provided, will be copied to the psample cookie.
>
> Should we mention any limit on the actual length of the cookie?
>
> Any reason we explicitly call this psample? From an OVS perspective, 
> this
> could just be
> SAMPLE_ATTR_FORWARD/OFFLOAD/DESTINATION/ENDPOINT/COLLECTOR/TARGET_COOKIE
>  and
> _GROUP. Other datapaths, do not have PSAMPLE.
>

 See my response to your comment on the cover-letter for possible name 
 alternatives.


> Thinking about it more, from an OVS perspective we should probably 
> not even
> send down a COOKIE, but we should send down an obs_domain_id and 
> obs_point_id,
> and then the kernel can move this into a cookie.
>

 I did consider this. However, the opaque cookie fits well with the tc 
 API which
 does not have two 32-bit values but an opaque 128-bit cookie. So if 
 the action
 is offloaded OVS needs to "encode" the two 32-bit values into the 
 cookie anyways.

> The collector itself could be called system/local collector, or 
> something like
> that. This way it can use for example psample for kernel and UDST 
> probes for
> usespace. Both can pass the group and cookie (obs_domain_id and 
> obs_point_id).
>
> Also, the presence of any of this should not dictate the psample 
> action, we
> probably need a specific OVS_SAMPLE_ATTR_SYSTEM_TARGET type nl flag.
>

 It clearer and it might simplify option verification a bit, but isn't 
 a bit
 redundant? There is no flag for action execution for instance, the 
 presence of
 the attribute is enough.

 An alternative would be to have nested attributes, e.g:

 enum ovs_sample_attr {
   OVS_SAMPLE_ATTR_UNSPEC,
   OVS_SAMPLE_ATTR_PROBABILITY, /* u32 number */
   OVS_SAMPLE_ATTR_ACTIONS, /* Nested OVS_ACTION_ATTR_* 
>>>

Re: [ovs-dev] [RFC 01/11] odp-util: Add support to new psample uAPI.



On 14 May 2024, at 14:28, Ilya Maximets wrote:

> On 5/14/24 13:27, Eelco Chaudron wrote:
>>
>>
>> On 14 May 2024, at 13:05, Ilya Maximets wrote:
>>
>>> On 5/14/24 12:14, Adrian Moreno wrote:


 On 5/14/24 11:09 AM, Ilya Maximets wrote:
> On 5/14/24 09:39, Adrian Moreno wrote:
>>
>>
>> On 5/10/24 12:45 PM, Adrian Moreno wrote:
>>>
>>>
>>> On 5/10/24 12:06 PM, Eelco Chaudron wrote:
 On 24 Apr 2024, at 21:53, Adrian Moreno wrote:

> The new odp sample attributes allow userspace to specify a group_id 
> and
> user-defined cookie to be passed down to psample.
>
> Add support for parsing and formatting such action.
>
> Signed-off-by: Adrian Moreno 

 Hi Adrian,

 See my comments below inline.

 //Eelco

> ---
>    include/linux/openvswitch.h  |  49 +---
>    lib/odp-execute.c    |   3 +
>    lib/odp-util.c   | 150 
> ++-
>    ofproto/ofproto-dpif-ipfix.c |   2 +
>    python/ovs/flow/odp.py   |   2 +
>    tests/odp.at |   5 ++
>    6 files changed, 163 insertions(+), 48 deletions(-)
>
> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
> index d9fb991ef..3e748405c 100644
> --- a/include/linux/openvswitch.h
> +++ b/include/linux/openvswitch.h
> @@ -696,6 +696,7 @@ enum ovs_flow_attr {
>    #define OVS_UFID_F_OMIT_MASK (1 << 1)
>    #define OVS_UFID_F_OMIT_ACTIONS  (1 << 2)
>
> +#define OVS_PSAMPLE_COOKIE_MAX_SIZE 16
>    /**
>     * enum ovs_sample_attr - Attributes for %OVS_ACTION_ATTR_SAMPLE 
> action.
>     * @OVS_SAMPLE_ATTR_PROBABILITY: 32-bit fraction of packets to 
> sample with
> @@ -703,15 +704,27 @@ enum ovs_flow_attr {
>     * %UINT32_MAX samples all packets and intermediate values sample 
> intermediate
>     * fractions of packets.
>     * @OVS_SAMPLE_ATTR_ACTIONS: Set of actions to execute in sampling 
> event.
> - * Actions are passed as nested attributes.
> + * Actions are passed as nested attributes. Optional if
> + * OVS_SAMPLE_ATTR_PSAMPLE_GROUP is set.
> + * @OVS_SAMPLE_ATTR_PSAMPLE_GROUP: A 32-bit number to be used as 
> psample group.
> + * Optional if OVS_SAMPLE_ATTR_ACTIONS is set.
> + * @OVS_SAMPLE_ATTR_PSAMPLE_COOKIE: A variable-length binary cookie 
> that, if
> + * provided, will be copied to the psample cookie.

 Should we mention any limit on the actual length of the cookie?

 Any reason we explicitly call this psample? From an OVS perspective, 
 this
 could just be
 SAMPLE_ATTR_FORWARD/OFFLOAD/DESTINATION/ENDPOINT/COLLECTOR/TARGET_COOKIE
  and
 _GROUP. Other datapaths, do not have PSAMPLE.

>>>
>>> See my response to your comment on the cover-letter for possible name 
>>> alternatives.
>>>
>>>
 Thinking about it more, from an OVS perspective we should probably not 
 even
 send down a COOKIE, but we should send down an obs_domain_id and 
 obs_point_id,
 and then the kernel can move this into a cookie.

>>>
>>> I did consider this. However, the opaque cookie fits well with the tc 
>>> API which
>>> does not have two 32-bit values but an opaque 128-bit cookie. So if the 
>>> action
>>> is offloaded OVS needs to "encode" the two 32-bit values into the 
>>> cookie anyways.
>>>
 The collector itself could be called system/local collector, or 
 something like
 that. This way it can use for example psample for kernel and UDST 
 probes for
 usespace. Both can pass the group and cookie (obs_domain_id and 
 obs_point_id).

 Also, the presence of any of this should not dictate the psample 
 action, we
 probably need a specific OVS_SAMPLE_ATTR_SYSTEM_TARGET type nl flag.

>>>
>>> It clearer and it might simplify option verification a bit, but isn't a 
>>> bit
>>> redundant? There is no flag for action execution for instance, the 
>>> presence of
>>> the attribute is enough.
>>>
>>> An alternative would be to have nested attributes, e.g:
>>>
>>> enum ovs_sample_attr {
>>>   OVS_SAMPLE_ATTR_UNSPEC,
>>>   OVS_SAMPLE_ATTR_PROBABILITY, /* u32 number */
>>>   OVS_SAMPLE_ATTR_ACTIONS, /* Nested OVS_ACTION_ATTR_* 
>>> attributes. */
>>>   OVS_SAMPLE_ATTR_SYSTEM, /* Nested OVS_SAMPLE_SYSTEM_ATTR_* 
>>> attributes. */
>>>
>>>   __OVS_SAMPLE_ATTR_MAX,
>>> };
>>>

Re: [ovs-dev] [RFC 01/11] odp-util: Add support to new psample uAPI.

On 5/14/24 13:27, Eelco Chaudron wrote:
> 
> 
> On 14 May 2024, at 13:05, Ilya Maximets wrote:
> 
>> On 5/14/24 12:14, Adrian Moreno wrote:
>>>
>>>
>>> On 5/14/24 11:09 AM, Ilya Maximets wrote:
 On 5/14/24 09:39, Adrian Moreno wrote:
>
>
> On 5/10/24 12:45 PM, Adrian Moreno wrote:
>>
>>
>> On 5/10/24 12:06 PM, Eelco Chaudron wrote:
>>> On 24 Apr 2024, at 21:53, Adrian Moreno wrote:
>>>
 The new odp sample attributes allow userspace to specify a group_id and
 user-defined cookie to be passed down to psample.

 Add support for parsing and formatting such action.

 Signed-off-by: Adrian Moreno 
>>>
>>> Hi Adrian,
>>>
>>> See my comments below inline.
>>>
>>> //Eelco
>>>
 ---
    include/linux/openvswitch.h  |  49 +---
    lib/odp-execute.c    |   3 +
    lib/odp-util.c   | 150 
 ++-
    ofproto/ofproto-dpif-ipfix.c |   2 +
    python/ovs/flow/odp.py   |   2 +
    tests/odp.at |   5 ++
    6 files changed, 163 insertions(+), 48 deletions(-)

 diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
 index d9fb991ef..3e748405c 100644
 --- a/include/linux/openvswitch.h
 +++ b/include/linux/openvswitch.h
 @@ -696,6 +696,7 @@ enum ovs_flow_attr {
    #define OVS_UFID_F_OMIT_MASK (1 << 1)
    #define OVS_UFID_F_OMIT_ACTIONS  (1 << 2)

 +#define OVS_PSAMPLE_COOKIE_MAX_SIZE 16
    /**
     * enum ovs_sample_attr - Attributes for %OVS_ACTION_ATTR_SAMPLE 
 action.
     * @OVS_SAMPLE_ATTR_PROBABILITY: 32-bit fraction of packets to 
 sample with
 @@ -703,15 +704,27 @@ enum ovs_flow_attr {
     * %UINT32_MAX samples all packets and intermediate values sample 
 intermediate
     * fractions of packets.
     * @OVS_SAMPLE_ATTR_ACTIONS: Set of actions to execute in sampling 
 event.
 - * Actions are passed as nested attributes.
 + * Actions are passed as nested attributes. Optional if
 + * OVS_SAMPLE_ATTR_PSAMPLE_GROUP is set.
 + * @OVS_SAMPLE_ATTR_PSAMPLE_GROUP: A 32-bit number to be used as 
 psample group.
 + * Optional if OVS_SAMPLE_ATTR_ACTIONS is set.
 + * @OVS_SAMPLE_ATTR_PSAMPLE_COOKIE: A variable-length binary cookie 
 that, if
 + * provided, will be copied to the psample cookie.
>>>
>>> Should we mention any limit on the actual length of the cookie?
>>>
>>> Any reason we explicitly call this psample? From an OVS perspective, 
>>> this
>>> could just be
>>> SAMPLE_ATTR_FORWARD/OFFLOAD/DESTINATION/ENDPOINT/COLLECTOR/TARGET_COOKIE
>>>  and
>>> _GROUP. Other datapaths, do not have PSAMPLE.
>>>
>>
>> See my response to your comment on the cover-letter for possible name 
>> alternatives.
>>
>>
>>> Thinking about it more, from an OVS perspective we should probably not 
>>> even
>>> send down a COOKIE, but we should send down an obs_domain_id and 
>>> obs_point_id,
>>> and then the kernel can move this into a cookie.
>>>
>>
>> I did consider this. However, the opaque cookie fits well with the tc 
>> API which
>> does not have two 32-bit values but an opaque 128-bit cookie. So if the 
>> action
>> is offloaded OVS needs to "encode" the two 32-bit values into the cookie 
>> anyways.
>>
>>> The collector itself could be called system/local collector, or 
>>> something like
>>> that. This way it can use for example psample for kernel and UDST 
>>> probes for
>>> usespace. Both can pass the group and cookie (obs_domain_id and 
>>> obs_point_id).
>>>
>>> Also, the presence of any of this should not dictate the psample 
>>> action, we
>>> probably need a specific OVS_SAMPLE_ATTR_SYSTEM_TARGET type nl flag.
>>>
>>
>> It clearer and it might simplify option verification a bit, but isn't a 
>> bit
>> redundant? There is no flag for action execution for instance, the 
>> presence of
>> the attribute is enough.
>>
>> An alternative would be to have nested attributes, e.g:
>>
>> enum ovs_sample_attr {
>>   OVS_SAMPLE_ATTR_UNSPEC,
>>   OVS_SAMPLE_ATTR_PROBABILITY, /* u32 number */
>>   OVS_SAMPLE_ATTR_ACTIONS, /* Nested OVS_ACTION_ATTR_* 
>> attributes. */
>>   OVS_SAMPLE_ATTR_SYSTEM, /* Nested OVS_SAMPLE_SYSTEM_ATTR_* 
>> attributes. */
>>
>>   __OVS_SAMPLE_ATTR_MAX,
>> };
>>
>> enum ovs_sample_system_attr {
>>   OVS_SAMPLE_SYSTEM_ATTR_OBS_DOMAIN,  /* u32 number */
>>   OVS_SAMPLE_SYSTEM_ATTR_OBS_POINT, /* u32 number */
>>   OVS_SAMP

Re: [ovs-dev] [RFC 01/11] odp-util: Add support to new psample uAPI.



On 14 May 2024, at 13:05, Ilya Maximets wrote:

> On 5/14/24 12:14, Adrian Moreno wrote:
>>
>>
>> On 5/14/24 11:09 AM, Ilya Maximets wrote:
>>> On 5/14/24 09:39, Adrian Moreno wrote:


 On 5/10/24 12:45 PM, Adrian Moreno wrote:
>
>
> On 5/10/24 12:06 PM, Eelco Chaudron wrote:
>> On 24 Apr 2024, at 21:53, Adrian Moreno wrote:
>>
>>> The new odp sample attributes allow userspace to specify a group_id and
>>> user-defined cookie to be passed down to psample.
>>>
>>> Add support for parsing and formatting such action.
>>>
>>> Signed-off-by: Adrian Moreno 
>>
>> Hi Adrian,
>>
>> See my comments below inline.
>>
>> //Eelco
>>
>>> ---
>>>    include/linux/openvswitch.h  |  49 +---
>>>    lib/odp-execute.c    |   3 +
>>>    lib/odp-util.c   | 150 
>>> ++-
>>>    ofproto/ofproto-dpif-ipfix.c |   2 +
>>>    python/ovs/flow/odp.py   |   2 +
>>>    tests/odp.at |   5 ++
>>>    6 files changed, 163 insertions(+), 48 deletions(-)
>>>
>>> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
>>> index d9fb991ef..3e748405c 100644
>>> --- a/include/linux/openvswitch.h
>>> +++ b/include/linux/openvswitch.h
>>> @@ -696,6 +696,7 @@ enum ovs_flow_attr {
>>>    #define OVS_UFID_F_OMIT_MASK (1 << 1)
>>>    #define OVS_UFID_F_OMIT_ACTIONS  (1 << 2)
>>>
>>> +#define OVS_PSAMPLE_COOKIE_MAX_SIZE 16
>>>    /**
>>>     * enum ovs_sample_attr - Attributes for %OVS_ACTION_ATTR_SAMPLE 
>>> action.
>>>     * @OVS_SAMPLE_ATTR_PROBABILITY: 32-bit fraction of packets to 
>>> sample with
>>> @@ -703,15 +704,27 @@ enum ovs_flow_attr {
>>>     * %UINT32_MAX samples all packets and intermediate values sample 
>>> intermediate
>>>     * fractions of packets.
>>>     * @OVS_SAMPLE_ATTR_ACTIONS: Set of actions to execute in sampling 
>>> event.
>>> - * Actions are passed as nested attributes.
>>> + * Actions are passed as nested attributes. Optional if
>>> + * OVS_SAMPLE_ATTR_PSAMPLE_GROUP is set.
>>> + * @OVS_SAMPLE_ATTR_PSAMPLE_GROUP: A 32-bit number to be used as 
>>> psample group.
>>> + * Optional if OVS_SAMPLE_ATTR_ACTIONS is set.
>>> + * @OVS_SAMPLE_ATTR_PSAMPLE_COOKIE: A variable-length binary cookie 
>>> that, if
>>> + * provided, will be copied to the psample cookie.
>>
>> Should we mention any limit on the actual length of the cookie?
>>
>> Any reason we explicitly call this psample? From an OVS perspective, this
>> could just be
>> SAMPLE_ATTR_FORWARD/OFFLOAD/DESTINATION/ENDPOINT/COLLECTOR/TARGET_COOKIE 
>> and
>> _GROUP. Other datapaths, do not have PSAMPLE.
>>
>
> See my response to your comment on the cover-letter for possible name 
> alternatives.
>
>
>> Thinking about it more, from an OVS perspective we should probably not 
>> even
>> send down a COOKIE, but we should send down an obs_domain_id and 
>> obs_point_id,
>> and then the kernel can move this into a cookie.
>>
>
> I did consider this. However, the opaque cookie fits well with the tc API 
> which
> does not have two 32-bit values but an opaque 128-bit cookie. So if the 
> action
> is offloaded OVS needs to "encode" the two 32-bit values into the cookie 
> anyways.
>
>> The collector itself could be called system/local collector, or 
>> something like
>> that. This way it can use for example psample for kernel and UDST probes 
>> for
>> usespace. Both can pass the group and cookie (obs_domain_id and 
>> obs_point_id).
>>
>> Also, the presence of any of this should not dictate the psample action, 
>> we
>> probably need a specific OVS_SAMPLE_ATTR_SYSTEM_TARGET type nl flag.
>>
>
> It clearer and it might simplify option verification a bit, but isn't a 
> bit
> redundant? There is no flag for action execution for instance, the 
> presence of
> the attribute is enough.
>
> An alternative would be to have nested attributes, e.g:
>
> enum ovs_sample_attr {
>   OVS_SAMPLE_ATTR_UNSPEC,
>   OVS_SAMPLE_ATTR_PROBABILITY, /* u32 number */
>   OVS_SAMPLE_ATTR_ACTIONS, /* Nested OVS_ACTION_ATTR_* 
> attributes. */
>   OVS_SAMPLE_ATTR_SYSTEM, /* Nested OVS_SAMPLE_SYSTEM_ATTR_* 
> attributes. */
>
>   __OVS_SAMPLE_ATTR_MAX,
> };
>
> enum ovs_sample_system_attr {
>   OVS_SAMPLE_SYSTEM_ATTR_OBS_DOMAIN,  /* u32 number */
>   OVS_SAMPLE_SYSTEM_ATTR_OBS_POINT, /* u32 number */
>   OVS_SAMPLE_SYSTEM_ATTR_COOKIE, /* Nested OVS_ACTION_ATTR_*
>
>   __OVS_SSAMPLE_SYSTEM_ATTR_MAX,
> };
>
> WDYT?
>

 Another benefit of nested attributes i

Re: [ovs-dev] [RFC 01/11] odp-util: Add support to new psample uAPI.

On 5/14/24 12:14, Adrian Moreno wrote:
> 
> 
> On 5/14/24 11:09 AM, Ilya Maximets wrote:
>> On 5/14/24 09:39, Adrian Moreno wrote:
>>>
>>>
>>> On 5/10/24 12:45 PM, Adrian Moreno wrote:


 On 5/10/24 12:06 PM, Eelco Chaudron wrote:
> On 24 Apr 2024, at 21:53, Adrian Moreno wrote:
>
>> The new odp sample attributes allow userspace to specify a group_id and
>> user-defined cookie to be passed down to psample.
>>
>> Add support for parsing and formatting such action.
>>
>> Signed-off-by: Adrian Moreno 
>
> Hi Adrian,
>
> See my comments below inline.
>
> //Eelco
>
>> ---
>>    include/linux/openvswitch.h  |  49 +---
>>    lib/odp-execute.c    |   3 +
>>    lib/odp-util.c   | 150 ++-
>>    ofproto/ofproto-dpif-ipfix.c |   2 +
>>    python/ovs/flow/odp.py   |   2 +
>>    tests/odp.at |   5 ++
>>    6 files changed, 163 insertions(+), 48 deletions(-)
>>
>> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
>> index d9fb991ef..3e748405c 100644
>> --- a/include/linux/openvswitch.h
>> +++ b/include/linux/openvswitch.h
>> @@ -696,6 +696,7 @@ enum ovs_flow_attr {
>>    #define OVS_UFID_F_OMIT_MASK (1 << 1)
>>    #define OVS_UFID_F_OMIT_ACTIONS  (1 << 2)
>>
>> +#define OVS_PSAMPLE_COOKIE_MAX_SIZE 16
>>    /**
>>     * enum ovs_sample_attr - Attributes for %OVS_ACTION_ATTR_SAMPLE 
>> action.
>>     * @OVS_SAMPLE_ATTR_PROBABILITY: 32-bit fraction of packets to sample 
>> with
>> @@ -703,15 +704,27 @@ enum ovs_flow_attr {
>>     * %UINT32_MAX samples all packets and intermediate values sample 
>> intermediate
>>     * fractions of packets.
>>     * @OVS_SAMPLE_ATTR_ACTIONS: Set of actions to execute in sampling 
>> event.
>> - * Actions are passed as nested attributes.
>> + * Actions are passed as nested attributes. Optional if
>> + * OVS_SAMPLE_ATTR_PSAMPLE_GROUP is set.
>> + * @OVS_SAMPLE_ATTR_PSAMPLE_GROUP: A 32-bit number to be used as 
>> psample group.
>> + * Optional if OVS_SAMPLE_ATTR_ACTIONS is set.
>> + * @OVS_SAMPLE_ATTR_PSAMPLE_COOKIE: A variable-length binary cookie 
>> that, if
>> + * provided, will be copied to the psample cookie.
>
> Should we mention any limit on the actual length of the cookie?
>
> Any reason we explicitly call this psample? From an OVS perspective, this
> could just be
> SAMPLE_ATTR_FORWARD/OFFLOAD/DESTINATION/ENDPOINT/COLLECTOR/TARGET_COOKIE 
> and
> _GROUP. Other datapaths, do not have PSAMPLE.
>

 See my response to your comment on the cover-letter for possible name 
 alternatives.


> Thinking about it more, from an OVS perspective we should probably not 
> even
> send down a COOKIE, but we should send down an obs_domain_id and 
> obs_point_id,
> and then the kernel can move this into a cookie.
>

 I did consider this. However, the opaque cookie fits well with the tc API 
 which
 does not have two 32-bit values but an opaque 128-bit cookie. So if the 
 action
 is offloaded OVS needs to "encode" the two 32-bit values into the cookie 
 anyways.

> The collector itself could be called system/local collector, or something 
> like
> that. This way it can use for example psample for kernel and UDST probes 
> for
> usespace. Both can pass the group and cookie (obs_domain_id and 
> obs_point_id).
>
> Also, the presence of any of this should not dictate the psample action, 
> we
> probably need a specific OVS_SAMPLE_ATTR_SYSTEM_TARGET type nl flag.
>

 It clearer and it might simplify option verification a bit, but isn't a bit
 redundant? There is no flag for action execution for instance, the 
 presence of
 the attribute is enough.

 An alternative would be to have nested attributes, e.g:

 enum ovs_sample_attr {
   OVS_SAMPLE_ATTR_UNSPEC,
   OVS_SAMPLE_ATTR_PROBABILITY, /* u32 number */
   OVS_SAMPLE_ATTR_ACTIONS, /* Nested OVS_ACTION_ATTR_* attributes. 
 */
   OVS_SAMPLE_ATTR_SYSTEM, /* Nested OVS_SAMPLE_SYSTEM_ATTR_* 
 attributes. */

   __OVS_SAMPLE_ATTR_MAX,
 };

 enum ovs_sample_system_attr {
   OVS_SAMPLE_SYSTEM_ATTR_OBS_DOMAIN,  /* u32 number */
   OVS_SAMPLE_SYSTEM_ATTR_OBS_POINT, /* u32 number */
   OVS_SAMPLE_SYSTEM_ATTR_COOKIE, /* Nested OVS_ACTION_ATTR_*

   __OVS_SSAMPLE_SYSTEM_ATTR_MAX,
 };

 WDYT?

>>>
>>> Another benefit of nested attributes is the easier addition of other
>>> sample-related attributes, I already have a candidate: 
>>> OVS_SAMPLE_SYSTEM_TRUNC
>>>
>>> This will be passed to psample (md->trunc_size) who will truncate the pa

[ovs-dev] OVN technical community meeting - May 13th

2024-05-14 Thread Dumitru Ceara



> I went ahead and scheduled a new instance:
> Date/Time: Monday, May 13th, 3PM UTC
> Link: https://meet.google.com/zns-gqsd-jdn?authuser=0&hs=122
> Agenda:
> https://docs.google.com/document/d/1dG4GwcYOSs4uArPGtOoaP5tH4KCto-GH_C3tIXSnZZ8/edit?usp=meetingnotes
> 

Thanks, everyone, for attending the OVN community meeting yesterday!  It
was a very interesting discussion about how to better integrate OVN and BGP!

Here's the link to the recording:
https://youtu.be/k448ada9aFQ

And the transcript of the chat:
https://drive.google.com/file/d/1VuF5l9wSR9z4rIH_LVF3PJBScj4Mb8JT

I'd like to schedule a new session in 5 weeks as it seems there's still
quite some details worth while discussing about how to actually
implement the BGP+OVN integration.

I was thinking of Monday, June 17th, 3PM UTC.  I'll wait for a few days
for people to check and potentially suggest alternatives before sending
out an invite.

Best regards,
Dumitru

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

Re: [ovs-dev] [RFC 01/11] odp-util: Add support to new psample uAPI.




On 5/14/24 11:09 AM, Ilya Maximets wrote:

On 5/14/24 09:39, Adrian Moreno wrote:



On 5/10/24 12:45 PM, Adrian Moreno wrote:



On 5/10/24 12:06 PM, Eelco Chaudron wrote:

On 24 Apr 2024, at 21:53, Adrian Moreno wrote:


The new odp sample attributes allow userspace to specify a group_id and
user-defined cookie to be passed down to psample.

Add support for parsing and formatting such action.

Signed-off-by: Adrian Moreno 


Hi Adrian,

See my comments below inline.

//Eelco


---
   include/linux/openvswitch.h  |  49 +---
   lib/odp-execute.c    |   3 +
   lib/odp-util.c   | 150 ++-
   ofproto/ofproto-dpif-ipfix.c |   2 +
   python/ovs/flow/odp.py   |   2 +
   tests/odp.at |   5 ++
   6 files changed, 163 insertions(+), 48 deletions(-)

diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
index d9fb991ef..3e748405c 100644
--- a/include/linux/openvswitch.h
+++ b/include/linux/openvswitch.h
@@ -696,6 +696,7 @@ enum ovs_flow_attr {
   #define OVS_UFID_F_OMIT_MASK (1 << 1)
   #define OVS_UFID_F_OMIT_ACTIONS  (1 << 2)

+#define OVS_PSAMPLE_COOKIE_MAX_SIZE 16
   /**
    * enum ovs_sample_attr - Attributes for %OVS_ACTION_ATTR_SAMPLE action.
    * @OVS_SAMPLE_ATTR_PROBABILITY: 32-bit fraction of packets to sample with
@@ -703,15 +704,27 @@ enum ovs_flow_attr {
    * %UINT32_MAX samples all packets and intermediate values sample 
intermediate
    * fractions of packets.
    * @OVS_SAMPLE_ATTR_ACTIONS: Set of actions to execute in sampling event.
- * Actions are passed as nested attributes.
+ * Actions are passed as nested attributes. Optional if
+ * OVS_SAMPLE_ATTR_PSAMPLE_GROUP is set.
+ * @OVS_SAMPLE_ATTR_PSAMPLE_GROUP: A 32-bit number to be used as psample group.
+ * Optional if OVS_SAMPLE_ATTR_ACTIONS is set.
+ * @OVS_SAMPLE_ATTR_PSAMPLE_COOKIE: A variable-length binary cookie that, if
+ * provided, will be copied to the psample cookie.


Should we mention any limit on the actual length of the cookie?

Any reason we explicitly call this psample? From an OVS perspective, this
could just be
SAMPLE_ATTR_FORWARD/OFFLOAD/DESTINATION/ENDPOINT/COLLECTOR/TARGET_COOKIE and
_GROUP. Other datapaths, do not have PSAMPLE.



See my response to your comment on the cover-letter for possible name 
alternatives.



Thinking about it more, from an OVS perspective we should probably not even
send down a COOKIE, but we should send down an obs_domain_id and obs_point_id,
and then the kernel can move this into a cookie.



I did consider this. However, the opaque cookie fits well with the tc API which
does not have two 32-bit values but an opaque 128-bit cookie. So if the action
is offloaded OVS needs to "encode" the two 32-bit values into the cookie 
anyways.


The collector itself could be called system/local collector, or something like
that. This way it can use for example psample for kernel and UDST probes for
usespace. Both can pass the group and cookie (obs_domain_id and obs_point_id).

Also, the presence of any of this should not dictate the psample action, we
probably need a specific OVS_SAMPLE_ATTR_SYSTEM_TARGET type nl flag.



It clearer and it might simplify option verification a bit, but isn't a bit
redundant? There is no flag for action execution for instance, the presence of
the attribute is enough.

An alternative would be to have nested attributes, e.g:

enum ovs_sample_attr {
  OVS_SAMPLE_ATTR_UNSPEC,
  OVS_SAMPLE_ATTR_PROBABILITY, /* u32 number */
  OVS_SAMPLE_ATTR_ACTIONS, /* Nested OVS_ACTION_ATTR_* attributes. */
  OVS_SAMPLE_ATTR_SYSTEM, /* Nested OVS_SAMPLE_SYSTEM_ATTR_* 
attributes. */

  __OVS_SAMPLE_ATTR_MAX,
};

enum ovs_sample_system_attr {
  OVS_SAMPLE_SYSTEM_ATTR_OBS_DOMAIN,  /* u32 number */
  OVS_SAMPLE_SYSTEM_ATTR_OBS_POINT, /* u32 number */
  OVS_SAMPLE_SYSTEM_ATTR_COOKIE, /* Nested OVS_ACTION_ATTR_*

  __OVS_SSAMPLE_SYSTEM_ATTR_MAX,
};

WDYT?



Another benefit of nested attributes is the easier addition of other
sample-related attributes, I already have a candidate: OVS_SAMPLE_SYSTEM_TRUNC

This will be passed to psample (md->trunc_size) who will truncate the packet
size to the specified value. This can be useful for large packets for which we
are only interested in the headers.

WDYT?

[snip]



Hrm.  This makes me think these should not be attributes of a sample action at 
all,
but rather we should have a separate EMIT_SAMPLE action that just calls 
psample, so
we can combine it with other actions:

   sample(..., actions(userspace(...),trunc(100),emit_sample(cookie=...)))

or even:

   sample(..., 
actions(clone(trunc(100),emit_sample(cookie=...)),userspace(...)))

This will also simplify the requires_datapath_assistance() check as we can just
check the action and not iterate over the list of attributes.  And it will be
capable to only send this one action to the datapath and not the whole thing.



My suggestion of adding a OVS_SAMPLE_{}

Re: [ovs-dev] [RFC 00/11] Add psample support to NXAST_SAMPLE action.

On 5/14/24 09:42, Adrian Moreno wrote:
> 
> 
> On 5/10/24 12:14 PM, Eelco Chaudron wrote:
>>
>>
>> On 10 May 2024, at 10:23, Adrian Moreno wrote:
>>
>>> On 5/10/24 9:14 AM, Eelco Chaudron wrote:


 On 24 Apr 2024, at 21:53, Adrian Moreno wrote:

> This is the userspace counterpart of the work being done in the kernel
> [1]. Sending it as RFC to get some early feedback on the overall
> solution.
>
> ** Problem description **
> Currently, OVS supports several observability features, such as
> per-bridge IPFIX, per-flow IPFIX and sFlow. However, given the
> complexity of OVN-generated pipelines, a single sample is typically not
> enough to troubleshoot what is OVN/OVS is doing with the packet. We need
> highler level metadata alongside the packet sample.
>
> This can be achieved by the means of per-flow IPFIX sampling and
> NXAST_SAMPLE action, through which OVN can add two arbitrary 32-bit
> values (observation_domain_id and observation_point_id) that are sent
> alongside the packet information in the IPFIX frames.
>
> However, there is a fundamental limitation of the existing sampling
> infrastructure, specially in the kernel datapath: samples are handled by
> ovs-vswitchd, forcing the need for an upcall (userspace action). The
> fact that samples share the same netlink sockets and handler thread cpu
> time with actual packet misse, can easily cause packet drops making this
> solution unfit for use in highly loaded production systems.
>
> Users are left with little option than guessing what sampling rate will
> be OK for their traffic pattern and dealing with the lost accuracy.
>
> ** Feature Description **
> In order to solve this situation and enable this feature to be safely
> turned on in production, this RFC uses the psample support proposed in
> [1] to send NXAST_SAMPLE samples to psample adding the observability
> domain and point information as metadata.
>
> ~~ API ~~
> The API is simply a new field called "psample_group" in the
> Flow_Sample_Collector_Set (FSCS) table. Configuring this value is
> orthogonal to also associating the FSCS entry to an entry in the IPFIX
> table.
>
> Apart from that configuration, the controller needs to add NXAST_SAMPLE
> actions that refer the entry's id.
>
> ~~ HW Offload ~~
> psample is already supported by tc using the act_sample action. The
> metadata is taken from the actions' cookie.
> This series also adds support for offloading the odp sample action,
> only when it includes psample information but not nested actions.
>
> A bit of special care has to be taken in the tc layer to not store the
> ufid in the sample's cookie as it's used to carry action-specific data.
>
> ~~ Datapath support ~~
> This new behavior of the datapth sample action is only supported on the
> kernel datapath. A more detailed analysis is needed (and planned) to
> find a way to also optimize the userspace datapath. However, although
> potentially inefficient, there is not that much risk of dropping packets
> with the existing IPFIX infrastructure.
>
> ~~ Testing ~~
> The series includes an utility program called "ovs-psample" that listens
> to packets multicasted by the psample module and prints them (also
> printing the obs_domain and obs_point ids). In addition the kernel
> series includes a tracepoint for easy testing and troubleshooting.
>
> [1] https://patchwork.kernel.org/project/netdevbpf/list/?series=847473

 Hi Andrew,

>>>
>>> Who is Andrew? :-D
>>
>> Sorry :( Handling too many things in parallel...
>>
 Thanks for sending out this RFC series. I did a code-only review (no 
 testing), and the main concern I have (besides some locking) is the direct 
 mapping to psample. If we decide that for userspace we are going to use 
 USDT probes, we need another target, and we will duplicate a lot of code. 
 My proposal would be to have a more general target name like system, or 
 local (or a better name ;). This will be a system-local dpif target with a 
 32-bit group id.

>>>
>>> Regarding the name, I agree adding "psample" to the API can be confusing 
>>> for userspace datapath. Besides, I think it should also be in sync with the 
>>> odp naming:
>>>
>>> "system": My initial thought was it could be confused with the "system@" 
>>> name of the kernel datapath, but that is not present in the OVSDB level so 
>>> it could work. SAMPLE_ATTR_{SYSTEM_GROUP,SYSTEM_USER_COOKIE}?
>>>
>>> "offload": I did consider this but I discarded it as it could be confused 
>>> with actual hw/tc offloading of the sample action.
>>>
>>> "local": Funny, I initially thought of the opposite, i.e: "external" as the 
>>> sample collection and potential encoding (which is currently "local" to 
>>> ovs-vswitchd

Re: [ovs-dev] [RFC 08/11] netdev-offload-tc: Add sample support.



On 13 May 2024, at 21:59, Adrian Moreno wrote:

> On 5/13/24 2:38 PM, Adrian Moreno wrote:
>>
>>
>> On 5/13/24 1:32 PM, Eelco Chaudron wrote:
>>>
>>>
>>> On 13 May 2024, at 10:44, Adrian Moreno wrote:
>>>
 On 5/10/24 12:06 PM, Eelco Chaudron wrote:
> On 24 Apr 2024, at 21:53, Adrian Moreno wrote:
>
>> Offload the sample action if it contains psample information by creating
>> a tc "sample" action with the user cookie inside the action's cookie.
>>
>> Avoid using the "sample" action's cookie to store the ufid.
>
> I have some concerns about the sample action-only solution. What happened 
> to the idea of adding an additional cookie to the TC match? Can we add a 
> test case for the explicit drop?
>


 I guess we can ask the kernel community. However, I'm not 100% convinced 
 it makes a lot of sense form a kernel perspective. There is already a 
 cookie in every action so there is plenty of space to store user data. 
 Kernel does not enforce any semantics on the cookies, it's up to userspace 
 to interpret them at will. Also, you cannot have a flow without an action 
 IIUC.
 So having a new cookie added just because it makes OVS code sligthly 
 cleaner doesn't seem to be a super strong argument.
>>>
>>> ACK, but I thought this was part of the earlier discussion, so it should be 
>>> explored. I guess we can make the opposite argument, why a per-action 
>>> cookie if we could have a per-flow one? Having a match cookie will free the 
>>> action cookie for action related metadata.
>>>
>
> I did explore, theoretically, the idea. Adding TCA_COOKIE seemed to me like a 
> significant effort. It would probably mean adding it as a top-level filter 
> attribute (common to all filters, not only flower), handling concurrent 
> access and modification, adding support to iproute tooling, making sure it's 
> carried over through hw-offloading, etc.
>
> My bulk estimation is that this, itself, is way bigger than the psample 
> feature I'm trying to add.
>
> So, with all, I just considered the benefits would not compensate the effort 
> of creating a new filter attribute. considering:
> 1) AFAIKS, we have a way to identify flows without the need for cookies at all
> 2) We could very well use some other action (e.g: what this patch does)
> 3) I felt chances of it even being accepted are not supper high

If this is the case, it makes no sense to get this in right now. Although it 
still would be nice to have :) Simon do you know if this was ever discussed 
before?

 Regarding tc <-> ufid correlation. I'm a bit confused by the fact that 
 flow replacement seems to work on a tcf_id basis, meaning that each tc 
 flow has it's own unique tcf_id (or we would replace some other flow). 
 Also, there is a hmap mapping tcf_id to ufids already. Am I missing 
 something?
>>>
>>> You are right, I guess this exists to support kernels where we did not have 
>>> an actions cookie, so we use find_ufid() in the flow dump. So in theory if 
>>> there is no action with a flow cookie it will all still work.
>>>
>>
>> So, could we just rely on this mechanism which is also backwards compatible 
>> and just use the action cookie for action information?

Guess this would do, we should have some adequate test cases to cover any 
corner cases we can think of.

> In general, I think we should apply the sflow patch before this series.
>

 Agree. Are we all OK with the compatibility issues that will introduce?

> //Eelco
>
>> Signed-off-by: Adrian Moreno 
>> ---
>>    include/linux/automake.mk    |  5 +-
>>    include/linux/pkt_cls.h  |  2 +
>>    include/linux/tc_act/tc_sample.h | 26 ++
>>    lib/netdev-offload-tc.c  | 67 +
>>    lib/tc.c | 84 ++--
>>    lib/tc.h |  7 +++
>>    utilities/ovs-psample.c  |  8 +--
>>    7 files changed, 190 insertions(+), 9 deletions(-)
>>    create mode 100644 include/linux/tc_act/tc_sample.h
>>
>> diff --git a/include/linux/automake.mk b/include/linux/automake.mk
>> index ac306b53c..c2a270152 100644
>> --- a/include/linux/automake.mk
>> +++ b/include/linux/automake.mk
>> @@ -5,9 +5,10 @@ noinst_HEADERS += \
>>    include/linux/pkt_cls.h \
>>    include/linux/psample.h \
>>    include/linux/gen_stats.h \
>> +    include/linux/tc_act/tc_ct.h \
>>    include/linux/tc_act/tc_mpls.h \
>>    include/linux/tc_act/tc_pedit.h \
>> +    include/linux/tc_act/tc_sample.h \
>>    include/linux/tc_act/tc_skbedit.h \
>>    include/linux/tc_act/tc_tunnel_key.h \
>> -    include/linux/tc_act/tc_vlan.h \
>> -    include/linux/tc_act/tc_ct.h
>> +    include/linux/tc_act/tc_vlan.h
>> diff --git a/include/linux/pkt_cls.h b/includ

Re: [ovs-dev] [RFC 01/11] odp-util: Add support to new psample uAPI.




On 13 May 2024, at 14:44, Adrian Moreno wrote:

> On 5/13/24 2:38 PM, Ilya Maximets wrote:
>> On 5/13/24 09:17, Eelco Chaudron wrote:
>>>
>>>
>>> On 10 May 2024, at 15:06, Ilya Maximets wrote:
>>>
 On 5/10/24 14:01, Eelco Chaudron wrote:
>
>
> On 10 May 2024, at 12:45, Adrian Moreno wrote:
>
>> On 5/10/24 12:06 PM, Eelco Chaudron wrote:
>>> On 24 Apr 2024, at 21:53, Adrian Moreno wrote:
>>>
 The new odp sample attributes allow userspace to specify a group_id and
 user-defined cookie to be passed down to psample.

 Add support for parsing and formatting such action.

 Signed-off-by: Adrian Moreno 
>>>
>>> Hi Adrian,
>>>
>>> See my comments below inline.
>>>
>>> //Eelco
>>>
 ---
include/linux/openvswitch.h  |  49 +---
lib/odp-execute.c|   3 +
lib/odp-util.c   | 150 
 ++-
ofproto/ofproto-dpif-ipfix.c |   2 +
python/ovs/flow/odp.py   |   2 +
tests/odp.at |   5 ++
6 files changed, 163 insertions(+), 48 deletions(-)

 diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
 index d9fb991ef..3e748405c 100644
 --- a/include/linux/openvswitch.h
 +++ b/include/linux/openvswitch.h
 @@ -696,6 +696,7 @@ enum ovs_flow_attr {
#define OVS_UFID_F_OMIT_MASK (1 << 1)
#define OVS_UFID_F_OMIT_ACTIONS  (1 << 2)

 +#define OVS_PSAMPLE_COOKIE_MAX_SIZE 16
/**
 * enum ovs_sample_attr - Attributes for %OVS_ACTION_ATTR_SAMPLE 
 action.
 * @OVS_SAMPLE_ATTR_PROBABILITY: 32-bit fraction of packets to 
 sample with
 @@ -703,15 +704,27 @@ enum ovs_flow_attr {
 * %UINT32_MAX samples all packets and intermediate values sample 
 intermediate
 * fractions of packets.
 * @OVS_SAMPLE_ATTR_ACTIONS: Set of actions to execute in sampling 
 event.
 - * Actions are passed as nested attributes.
 + * Actions are passed as nested attributes. Optional if
 + * OVS_SAMPLE_ATTR_PSAMPLE_GROUP is set.
 + * @OVS_SAMPLE_ATTR_PSAMPLE_GROUP: A 32-bit number to be used as 
 psample group.
 + * Optional if OVS_SAMPLE_ATTR_ACTIONS is set.
 + * @OVS_SAMPLE_ATTR_PSAMPLE_COOKIE: A variable-length binary cookie 
 that, if
 + * provided, will be copied to the psample cookie.
>>>
>>> Should we mention any limit on the actual length of the cookie?
>>>
>>> Any reason we explicitly call this psample? From an OVS perspective, 
>>> this could just be 
>>> SAMPLE_ATTR_FORWARD/OFFLOAD/DESTINATION/ENDPOINT/COLLECTOR/TARGET_COOKIE
>>>  and _GROUP. Other datapaths, do not have PSAMPLE.
>>>
>>
>> See my response to your comment on the cover-letter for possible name 
>> alternatives.
>
> ACK, we can continue the discussion there.
>
>>> Thinking about it more, from an OVS perspective we should probably not 
>>> even send down a COOKIE, but we should send down an obs_domain_id and 
>>> obs_point_id, and then the kernel can move this into a cookie.
>>>
>>
>> I did consider this. However, the opaque cookie fits well with the tc 
>> API which does not have two 32-bit values but an opaque 128-bit cookie. 
>> So if the action is offloaded OVS needs to "encode" the two 32-bit 
>> values into the cookie anyways.
>
> Which is fine to me, the OVS API should be clear that we have two u32 
> entries. The cookie is implementation-specific, and we use the netlink 
> format as our API for all DPs.


 I'm not sure about this.  It is a kernel interface and we can't deliver
 two separate values from the psample.  So, passing two separate values
 to the kernel doesn't make a lot of sense.  They are going to be mangled
 into a single cookie anyway and we'll have to document that somehow.
 We may as well always mangle the data from the beginning, document once
 at the top level and save ourselves from documenting a million differences
 between different implementations.  While USDT could report them 
 separately,
 I'm not sure there is a ton of value in that.
>>>
>>> Our OVS action API is not tight to psample at all, so passing in a psample 
>>> cookie
>>> does not make sense. We should not pass in any psample references to the 
>>> sample
>>> action API. If it looks like the below, the API is clean and we can mangle 
>>> it before
>>> passing it to the psample function. I see no need to document this in the 
>>> kernel, as
>>> all the kernel does is provide the TC action cookie (unrelated to the OVS 
>>> actions API).
>>
>> We're passing this data directly to psample, not TC.  An

Re: [ovs-dev] [RFC 01/11] odp-util: Add support to new psample uAPI.

On 5/13/24 20:59, Adrian Moreno wrote:
> 
> 
> On 5/10/24 3:06 PM, Ilya Maximets wrote:
>> On 5/10/24 14:01, Eelco Chaudron wrote:
>>>
>>>
>>> On 10 May 2024, at 12:45, Adrian Moreno wrote:
>>>
 On 5/10/24 12:06 PM, Eelco Chaudron wrote:
> On 24 Apr 2024, at 21:53, Adrian Moreno wrote:
>
>> The new odp sample attributes allow userspace to specify a group_id and
>> user-defined cookie to be passed down to psample.
>>
>> Add support for parsing and formatting such action.
>>
>> Signed-off-by: Adrian Moreno 
>
> Hi Adrian,
>
> See my comments below inline.
>
> //Eelco
>
>> ---
>>include/linux/openvswitch.h  |  49 +---
>>lib/odp-execute.c|   3 +
>>lib/odp-util.c   | 150 ++-
>>ofproto/ofproto-dpif-ipfix.c |   2 +
>>python/ovs/flow/odp.py   |   2 +
>>tests/odp.at |   5 ++
>>6 files changed, 163 insertions(+), 48 deletions(-)
>>
>> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
>> index d9fb991ef..3e748405c 100644
>> --- a/include/linux/openvswitch.h
>> +++ b/include/linux/openvswitch.h
>> @@ -696,6 +696,7 @@ enum ovs_flow_attr {
>>#define OVS_UFID_F_OMIT_MASK (1 << 1)
>>#define OVS_UFID_F_OMIT_ACTIONS  (1 << 2)
>>
>> +#define OVS_PSAMPLE_COOKIE_MAX_SIZE 16
>>/**
>> * enum ovs_sample_attr - Attributes for %OVS_ACTION_ATTR_SAMPLE 
>> action.
>> * @OVS_SAMPLE_ATTR_PROBABILITY: 32-bit fraction of packets to sample 
>> with
>> @@ -703,15 +704,27 @@ enum ovs_flow_attr {
>> * %UINT32_MAX samples all packets and intermediate values sample 
>> intermediate
>> * fractions of packets.
>> * @OVS_SAMPLE_ATTR_ACTIONS: Set of actions to execute in sampling 
>> event.
>> - * Actions are passed as nested attributes.
>> + * Actions are passed as nested attributes. Optional if
>> + * OVS_SAMPLE_ATTR_PSAMPLE_GROUP is set.
>> + * @OVS_SAMPLE_ATTR_PSAMPLE_GROUP: A 32-bit number to be used as 
>> psample group.
>> + * Optional if OVS_SAMPLE_ATTR_ACTIONS is set.
>> + * @OVS_SAMPLE_ATTR_PSAMPLE_COOKIE: A variable-length binary cookie 
>> that, if
>> + * provided, will be copied to the psample cookie.
>
> Should we mention any limit on the actual length of the cookie?
>
> Any reason we explicitly call this psample? From an OVS perspective, this 
> could just be 
> SAMPLE_ATTR_FORWARD/OFFLOAD/DESTINATION/ENDPOINT/COLLECTOR/TARGET_COOKIE 
> and _GROUP. Other datapaths, do not have PSAMPLE.
>

 See my response to your comment on the cover-letter for possible name 
 alternatives.
>>>
>>> ACK, we can continue the discussion there.
>>>
> Thinking about it more, from an OVS perspective we should probably not 
> even send down a COOKIE, but we should send down an obs_domain_id and 
> obs_point_id, and then the kernel can move this into a cookie.
>

 I did consider this. However, the opaque cookie fits well with the tc API 
 which does not have two 32-bit values but an opaque 128-bit cookie. So if 
 the action is offloaded OVS needs to "encode" the two 32-bit values into 
 the cookie anyways.
>>>
>>> Which is fine to me, the OVS API should be clear that we have two u32 
>>> entries. The cookie is implementation-specific, and we use the netlink 
>>> format as our API for all DPs.
>>
>>
>> I'm not sure about this.  It is a kernel interface and we can't deliver
>> two separate values from the psample.  So, passing two separate values
>> to the kernel doesn't make a lot of sense.  They are going to be mangled
>> into a single cookie anyway and we'll have to document that somehow.
>> We may as well always mangle the data from the beginning, document once
>> at the top level and save ourselves from documenting a million differences
>> between different implementations.  While USDT could report them separately,
>> I'm not sure there is a ton of value in that.
>>
>>>
> The collector itself could be called system/local collector, or something 
> like that. This way it can use for example psample for kernel and UDST 
> probes for usespace. Both can pass the group and cookie (obs_domain_id 
> and obs_point_id).
>
> Also, the presence of any of this should not dictate the psample action, 
> we probably need a specific OVS_SAMPLE_ATTR_SYSTEM_TARGET type nl flag.
>

 It clearer and it might simplify option verification a bit, but isn't a 
 bit redundant? There is no flag for action execution for instance, the 
 presence of the attribute is enough.

 An alternative would be to have nested attributes, e.g:

 enum ovs_sample_attr {
OVS_SAMPLE_ATTR_UNSPEC,
OVS_SAMPLE_ATTR_PROBABILITY, /* u32 number */
OVS_SAMPLE_ATTR_A

Re: [ovs-dev] [RFC 01/11] odp-util: Add support to new psample uAPI.

On 5/14/24 09:39, Adrian Moreno wrote:
> 
> 
> On 5/10/24 12:45 PM, Adrian Moreno wrote:
>>
>>
>> On 5/10/24 12:06 PM, Eelco Chaudron wrote:
>>> On 24 Apr 2024, at 21:53, Adrian Moreno wrote:
>>>
 The new odp sample attributes allow userspace to specify a group_id and
 user-defined cookie to be passed down to psample.

 Add support for parsing and formatting such action.

 Signed-off-by: Adrian Moreno 
>>>
>>> Hi Adrian,
>>>
>>> See my comments below inline.
>>>
>>> //Eelco
>>>
 ---
   include/linux/openvswitch.h  |  49 +---
   lib/odp-execute.c    |   3 +
   lib/odp-util.c   | 150 ++-
   ofproto/ofproto-dpif-ipfix.c |   2 +
   python/ovs/flow/odp.py   |   2 +
   tests/odp.at |   5 ++
   6 files changed, 163 insertions(+), 48 deletions(-)

 diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
 index d9fb991ef..3e748405c 100644
 --- a/include/linux/openvswitch.h
 +++ b/include/linux/openvswitch.h
 @@ -696,6 +696,7 @@ enum ovs_flow_attr {
   #define OVS_UFID_F_OMIT_MASK (1 << 1)
   #define OVS_UFID_F_OMIT_ACTIONS  (1 << 2)

 +#define OVS_PSAMPLE_COOKIE_MAX_SIZE 16
   /**
    * enum ovs_sample_attr - Attributes for %OVS_ACTION_ATTR_SAMPLE action.
    * @OVS_SAMPLE_ATTR_PROBABILITY: 32-bit fraction of packets to sample 
 with
 @@ -703,15 +704,27 @@ enum ovs_flow_attr {
    * %UINT32_MAX samples all packets and intermediate values sample 
 intermediate
    * fractions of packets.
    * @OVS_SAMPLE_ATTR_ACTIONS: Set of actions to execute in sampling event.
 - * Actions are passed as nested attributes.
 + * Actions are passed as nested attributes. Optional if
 + * OVS_SAMPLE_ATTR_PSAMPLE_GROUP is set.
 + * @OVS_SAMPLE_ATTR_PSAMPLE_GROUP: A 32-bit number to be used as psample 
 group.
 + * Optional if OVS_SAMPLE_ATTR_ACTIONS is set.
 + * @OVS_SAMPLE_ATTR_PSAMPLE_COOKIE: A variable-length binary cookie that, 
 if
 + * provided, will be copied to the psample cookie.
>>>
>>> Should we mention any limit on the actual length of the cookie?
>>>
>>> Any reason we explicitly call this psample? From an OVS perspective, this 
>>> could just be 
>>> SAMPLE_ATTR_FORWARD/OFFLOAD/DESTINATION/ENDPOINT/COLLECTOR/TARGET_COOKIE 
>>> and 
>>> _GROUP. Other datapaths, do not have PSAMPLE.
>>>
>>
>> See my response to your comment on the cover-letter for possible name 
>> alternatives.
>>
>>
>>> Thinking about it more, from an OVS perspective we should probably not even 
>>> send down a COOKIE, but we should send down an obs_domain_id and 
>>> obs_point_id, 
>>> and then the kernel can move this into a cookie.
>>>
>>
>> I did consider this. However, the opaque cookie fits well with the tc API 
>> which 
>> does not have two 32-bit values but an opaque 128-bit cookie. So if the 
>> action 
>> is offloaded OVS needs to "encode" the two 32-bit values into the cookie 
>> anyways.
>>
>>> The collector itself could be called system/local collector, or something 
>>> like 
>>> that. This way it can use for example psample for kernel and UDST probes 
>>> for 
>>> usespace. Both can pass the group and cookie (obs_domain_id and 
>>> obs_point_id).
>>>
>>> Also, the presence of any of this should not dictate the psample action, we 
>>> probably need a specific OVS_SAMPLE_ATTR_SYSTEM_TARGET type nl flag.
>>>
>>
>> It clearer and it might simplify option verification a bit, but isn't a bit 
>> redundant? There is no flag for action execution for instance, the presence 
>> of 
>> the attribute is enough.
>>
>> An alternative would be to have nested attributes, e.g:
>>
>> enum ovs_sample_attr {
>>  OVS_SAMPLE_ATTR_UNSPEC,
>>  OVS_SAMPLE_ATTR_PROBABILITY, /* u32 number */
>>  OVS_SAMPLE_ATTR_ACTIONS, /* Nested OVS_ACTION_ATTR_* attributes. */
>>  OVS_SAMPLE_ATTR_SYSTEM, /* Nested OVS_SAMPLE_SYSTEM_ATTR_* 
>> attributes. */
>>
>>  __OVS_SAMPLE_ATTR_MAX,
>> };
>>
>> enum ovs_sample_system_attr {
>>  OVS_SAMPLE_SYSTEM_ATTR_OBS_DOMAIN,  /* u32 number */
>>  OVS_SAMPLE_SYSTEM_ATTR_OBS_POINT, /* u32 number */
>>  OVS_SAMPLE_SYSTEM_ATTR_COOKIE, /* Nested OVS_ACTION_ATTR_*
>>
>>  __OVS_SSAMPLE_SYSTEM_ATTR_MAX,
>> };
>>
>> WDYT?
>>
> 
> Another benefit of nested attributes is the easier addition of other 
> sample-related attributes, I already have a candidate: OVS_SAMPLE_SYSTEM_TRUNC
> 
> This will be passed to psample (md->trunc_size) who will truncate the packet 
> size to the specified value. This can be useful for large packets for which 
> we 
> are only interested in the headers.
> 
> WDYT?
> 
> [snip]
> 

Hrm.  This makes me think these should not be attributes of a sample action at 
all,
but rather we should have a separate EMIT_SAMPLE action that just calls 
psample, so
we can combine it with other actions:

  sample(..., actions(userspa

[ovs-dev] [PATCH ovn 5/7] ci: Add missing packages to run Fedora image in GH CI.

There were two things missing for the Fedora builds, 32-bit
version of glibc to allows the -m32 compilation on Fedora
and numactl-devel package.

Signed-off-by: Ales Musil 
---
 .ci/linux-build.sh | 3 +++
 utilities/containers/fedora/Dockerfile | 1 +
 2 files changed, 4 insertions(+)

diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
index 78f17f8bd..12966f532 100755
--- a/.ci/linux-build.sh
+++ b/.ci/linux-build.sh
@@ -83,6 +83,9 @@ function configure_gcc()
 # do it directly because gcc-multilib is not available
 # for arm64
 sudo apt update && sudo apt install -y gcc-multilib
+elif which dnf; then
+# Install equivalent of gcc-multilib for Fedora.
+sudo dnf -y update && sudo dnf -y install glibc-devel.i686
 fi
 fi
 }
diff --git a/utilities/containers/fedora/Dockerfile 
b/utilities/containers/fedora/Dockerfile
index 9b8386aae..d40a7b31f 100755
--- a/utilities/containers/fedora/Dockerfile
+++ b/utilities/containers/fedora/Dockerfile
@@ -28,6 +28,7 @@ RUN dnf -y update \
 libtool \
 net-tools \
 nmap-ncat \
+numactl-devel \
 openssl \
 openssl-devel \
 procps-ng \
-- 
2.44.0

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

[ovs-dev] [PATCH ovn 7/7] ci: Bump the Fedora container to 40.

Now that all failures were resolved for Fedora 40
we can bump the version in the container.

Signed-off-by: Ales Musil 
---
 utilities/containers/fedora/Dockerfile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/utilities/containers/fedora/Dockerfile 
b/utilities/containers/fedora/Dockerfile
index d40a7b31f..019e9f138 100755
--- a/utilities/containers/fedora/Dockerfile
+++ b/utilities/containers/fedora/Dockerfile
@@ -1,4 +1,4 @@
-FROM quay.io/fedora/fedora:39
+FROM quay.io/fedora/fedora:40
 
 ARG CONTAINERS_PATH
 
-- 
2.44.0

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

[ovs-dev] [PATCH ovn 4/7] tests: Replace wget with curl for failing commands.

wget2 has a bug and doesn't return proper exit code on error [0].
Replace wget with curl in places where we expect exit code to
be different from 0.

[0] https://gitlab.com/gnuwget/wget2/-/issues/652
Signed-off-by: Ales Musil 
---
 tests/system-ovn.at| 15 +++
 utilities/containers/ubuntu/Dockerfile |  1 +
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index 86fd240d2..f49330a1e 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -1439,7 +1439,7 @@ OVS_START_L7([bar3], [http])
 check ovn-nbctl --apply-after-lb acl-add foo from-lport 1002 "ip4 && ip4.dst 
== {172.16.1.2,172.16.1.3,172.16.1.4} && ct.new" drop
 check ovn-nbctl --wait=hv sync
 
-AT_CHECK([ip netns exec foo1 wget 30.0.0.1 -t 3 -T 1], [4], [ignore], [ignore])
+AT_CHECK([ip netns exec foo1 curl 30.0.0.1 --retry 3 --max-time 1], [28], 
[ignore], [ignore])
 
 AT_CHECK([ovs-appctl dpctl/flush-conntrack])
 
@@ -1603,7 +1603,7 @@ ovn-nbctl --reject lb-add lb3 30.0.0.10:80 ""
 ovn-nbctl ls-lb-add foo lb3
 # Filter reset segments
 NETNS_START_TCPDUMP([foo1], [-c 1 -neei foo1 ip[[33:1]]=0x14], [rst])
-NS_CHECK_EXEC([foo1], [wget -q 30.0.0.10],[4])
+NS_CHECK_EXEC([foo1], [curl 30.0.0.10 -s --retry 3 --max-time 1], [7])
 
 OVS_WAIT_UNTIL([
 n_reset=$(cat rst.tcpdump | wc -l)
@@ -4627,7 +4627,7 @@ NS_CHECK_EXEC([sw1-p1], [kill $(cat $pid_file)])
 NETNS_START_TCPDUMP([sw0-p2], [-c 1 -neei sw0-p2 ip[[33:1]]=0x14], [rst])
 OVS_WAIT_UNTIL([test 2 = `ovn-sbctl --bare --columns status find \
 service_monitor protocol=tcp | sed '/^$/d' | grep offline | wc -l`])
-NS_CHECK_EXEC([sw0-p2], [wget 10.0.0.10 -v -o wget$i.log],[4])
+NS_CHECK_EXEC([sw0-p2], [curl 10.0.0.10 -v > curl$i.log 2>&1],[7])
 
 OVS_WAIT_UNTIL([
 n_reset=$(cat rst.tcpdump | wc -l)
@@ -9770,7 +9770,7 @@ OVS_WAIT_UNTIL([ovs-ofctl -O OpenFlow13 dump-groups 
br-int | \
 grep 'nat(dst=192.168.2.2:80)'])
 
 # should not dnat so will not be able to connect
-AT_CHECK([ip netns exec foo1 wget   30.30.30.30  -t 3 -T 1], [4], [ignore], 
[ignore])
+AT_CHECK([ip netns exec foo1 curl 30.30.30.30 --retry 3 --max-time 1], [28], 
[ignore], [ignore])
 
 # check conntrack zone has no tcp entry
 AT_CHECK([ovs-appctl dpctl/dump-conntrack zone=$zone_id | \
@@ -9840,14 +9840,14 @@ ovn-nbctl lsp-add bar rp-bar -- set Logical_Switch_Port 
rp-bar \
 # Logical port 'foo1' in switch 'foo'.
 ADD_NAMESPACES(foo1)
 ADD_VETH(foo1, foo1, br-int, "fd11::2/64", "f0:00:00:01:02:03", \
- "fd11::1")
+ "fd11::1", "nodad")
 ovn-nbctl lsp-add foo foo1 \
 -- lsp-set-addresses foo1 "f0:00:00:01:02:03 fd11::2"
 
 # Logical port 'bar1' in switch 'bar'.
 ADD_NAMESPACES(bar1)
 ADD_VETH(bar1, bar1, br-int, "fd12::2/64", "f0:00:00:01:02:05", \
-"fd12::1")
+ "fd12::1",  "nodad")
 ovn-nbctl lsp-add bar bar1 \
 -- lsp-set-addresses bar1 "f0:00:00:01:02:05 fd12::2"
 
@@ -9864,7 +9864,6 @@ grep 'nat(dst=\[[fd12::2\]]:80)'])
 zone_id=$(ovn-appctl -t ovn-controller ct-zone-list | grep foo1 | cut -d ' ' 
-f2)
 
 OVS_START_L7([bar1], [http6])
-
 AT_CHECK([ip netns exec foo1  wget http://[[fd12::2]] -t 3 -T 1], [0], 
[ignore], [ignore])
 
 # check conntrack zone has tcp entry
@@ -9915,7 +9914,7 @@ OVS_WAIT_UNTIL([ovs-ofctl -O OpenFlow13 dump-groups 
br-int | \
 grep 'nat(dst=\[[fd12::2\]]:80)'])
 
 # should not dnat so will not be able to connect
-AT_CHECK([ip netns exec foo1 wget  http://[[fd30::2]]  -t 3 -T 1], [4], 
[ignore], [ignore])
+AT_CHECK([ip netns exec foo1 curl http://[[fd30::2]] --retry 3 --max-time 1], 
[28], [ignore], [ignore])
 #
 # check conntrack zone has no tcp entry
 AT_CHECK([ovs-appctl dpctl/dump-conntrack zone=$zone_id | \
diff --git a/utilities/containers/ubuntu/Dockerfile 
b/utilities/containers/ubuntu/Dockerfile
index c1ff711c5..ce7ce16c6 100755
--- a/utilities/containers/ubuntu/Dockerfile
+++ b/utilities/containers/ubuntu/Dockerfile
@@ -11,6 +11,7 @@ RUN apt update -y \
 automake \
 bc \
 clang \
+curl \
 ethtool \
 gcc \
 git \
-- 
2.44.0

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

[ovs-dev] [PATCH ovn 6/7] ci: Make sure that we are using proper image.

The container image for scheduled jobs was supposed to be Fedora,
however there was already Ubuntu image in the cache. Make sure
the cache is distinguished also by the event name.

Signed-off-by: Ales Musil 
---
 .github/workflows/test.yml | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
index 22e4d339d..efe2dac25 100644
--- a/.github/workflows/test.yml
+++ b/.github/workflows/test.yml
@@ -131,7 +131,7 @@ jobs:
 uses: actions/cache@v4
 with:
   path: /tmp/image.tar
-  key: ${{ github.sha }}
+  key: ${{ github.sha }}/${{ github.event_name }}
 
   build-linux:
 needs: [build-dpdk, prepare-container]
@@ -212,10 +212,11 @@ jobs:
 key: ${{ needs.build-dpdk.outputs.dpdk_key }}
 
 - name: image cache
+  id: image_cache
   uses: actions/cache@v4
   with:
 path: /tmp/image.tar
-key: ${{ github.sha }}
+key: ${{ github.sha }}/${{ github.event_name }}
 
 # XXX This should be removed when native crun >=1.9.1
 - name: update crun script
-- 
2.44.0

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

[ovs-dev] [PATCH ovn 0/7] Bump of CI Ubuntu and Fedora versions

The series is pretty small however it is required for the
bump to Ubuntu 24.04 and Fedora 40. Both have newer GCC and
Clang which brought up some issues that needed to be fixed.

The series also includes fix for weekly runs to use Fedora
because the cache string wasn't specific enough.

Ales Musil (7):
  ci: Pin Fedora version for the build-rpm job.
  ovs: Bump the submodule to the tip of branch-3.3.
  ci: Update the Ubuntu container to 24.04.
  tests: Replace wget with curl for failing commands.
  ci: Add missing packages to run Fedora image in GH CI.
  ci: Make sure that we are using proper image.
  ci: Bump the Fedora container to 40.

 .ci/linux-build.sh |  3 +++
 .github/workflows/test.yml |  7 ---
 ovs|  2 +-
 tests/system-ovn.at| 15 +++
 utilities/containers/fedora/Dockerfile |  3 ++-
 utilities/containers/ubuntu/Dockerfile | 11 ++-
 6 files changed, 27 insertions(+), 14 deletions(-)

-- 
2.44.0

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

[ovs-dev] [PATCH ovn 3/7] ci: Update the Ubuntu container to 24.04.

The Ubuntu 24.04 marks the Python installation as externally managed
this prevents pip from installing system-wide packages. Set the
PIP_BREAK_SYSTEM_PACKAGES env variable that allows pip to ignore
this and install the packages anyway.

At the same time the Python Babel fails to detect timezone when
it is just set to UTC. Setting it to Etc/UTC fixes the issue:

ValueError: ZoneInfo keys may not be absolute paths, got: /UTC

Signed-off-by: Ales Musil 
---
 utilities/containers/ubuntu/Dockerfile | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/utilities/containers/ubuntu/Dockerfile 
b/utilities/containers/ubuntu/Dockerfile
index ac1e6a5bf..c1ff711c5 100755
--- a/utilities/containers/ubuntu/Dockerfile
+++ b/utilities/containers/ubuntu/Dockerfile
@@ -1,4 +1,4 @@
-FROM registry.hub.docker.com/library/ubuntu:22.04
+FROM registry.hub.docker.com/library/ubuntu:24.04
 
 ARG CONTAINERS_PATH
 
@@ -37,6 +37,7 @@ RUN apt update -y \
 selinux-policy-dev \
 sudo \
 tcpdump \
+tzdata \
 wget \
 && \
 apt autoremove \
@@ -73,6 +74,10 @@ WORKDIR /workspace
 
 COPY $CONTAINERS_PATH/py-requirements.txt /tmp/py-requirements.txt
 
+# Ubuntu 24.04 marks the Python installation as externally managed, allow pip
+# to install the packages despite that.
+ENV PIP_BREAK_SYSTEM_PACKAGES 1
+
 # Update and install pip dependencies
 RUN python3 -m pip install --upgrade pip \
 && \
@@ -80,4 +85,7 @@ RUN python3 -m pip install --upgrade pip \
 && \
 python3 -m pip install -r /tmp/py-requirements.txt
 
+# The Python Babel fails to detect timezone when it is set to UTC only.
+ENV TZ Etc/UTC
+
 CMD ["/sbin/init"]
-- 
2.44.0

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

[ovs-dev] [PATCH ovn 2/7] ovs: Bump the submodule to the tip of branch-3.3.

The branch-3.3 includes fixes that are required for Fedora 40
and Ubuntu 24.04 bump.

cf461fe282c9 ("conntrack: Do not use {0} to initialize unions.")
4756bf4baf1e ("ofproto-dpif-trace: Fix access to an out-of-scope stack memory.")
01eca18be187 ("hash, jhash: Fix unaligned access to the hash remainder.")
4f61523c0d50 ("sparse: Add additional define for sparse on GCC >= 14.")
9a5c24d70fb6 ("sparse: Add immintrin.h header.")
3528cc6f452a ("tc: Fix -Wgnu-variable-sized-type-not-at-end warning with Clang 
18.")
5814de56878f ("tests: Fix build failure with Clang 18 due to 
-Wformat-truncation.")

Signed-off-by: Ales Musil 
---
 ovs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ovs b/ovs
index f19448b86..bf1b16364 16
--- a/ovs
+++ b/ovs
@@ -1 +1 @@
-Subproject commit f19448b8618967a108ec6f34713dd811ce1d1334
+Subproject commit bf1b16364b3f01b0ff5f2f6e76842e666226a17b
-- 
2.44.0

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

[ovs-dev] [PATCH ovn 1/7] ci: Pin Fedora version for the build-rpm job.

Keep the Fedora version pinned to 40 for the build-rpm.

Fixes: 8f18b3b6c52f ("ci: Keep the container version pinned.")
Signed-off-by: Ales Musil 
---
 .github/workflows/test.yml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
index 456ab5c69..22e4d339d 100644
--- a/.github/workflows/test.yml
+++ b/.github/workflows/test.yml
@@ -315,7 +315,7 @@ jobs:
   build-linux-rpm:
 name: linux rpm fedora
 runs-on: ubuntu-22.04
-container: fedora:latest
+container: fedora:40
 timeout-minutes: 30
 
 strategy:
-- 
2.44.0

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

Re: [ovs-dev] [RFC 00/11] Add psample support to NXAST_SAMPLE action.

On 5/10/24 12:14 PM, Eelco Chaudron wrote:

On 10 May 2024, at 10:23, Adrian Moreno wrote:

On 5/10/24 9:14 AM, Eelco Chaudron wrote:

On 24 Apr 2024, at 21:53, Adrian Moreno wrote:

This is the userspace counterpart of the work being done in the kernel
[1]. Sending it as RFC to get some early feedback on the overall
solution.

** Problem description **
Currently, OVS supports several observability features, such as
per-bridge IPFIX, per-flow IPFIX and sFlow. However, given the
complexity of OVN-generated pipelines, a single sample is typically not
enough to troubleshoot what is OVN/OVS is doing with the packet. We need
highler level metadata alongside the packet sample.

This can be achieved by the means of per-flow IPFIX sampling and
NXAST_SAMPLE action, through which OVN can add two arbitrary 32-bit
values (observation_domain_id and observation_point_id) that are sent
alongside the packet information in the IPFIX frames.

However, there is a fundamental limitation of the existing sampling
infrastructure, specially in the kernel datapath: samples are handled by
ovs-vswitchd, forcing the need for an upcall (userspace action). The
fact that samples share the same netlink sockets and handler thread cpu
time with actual packet misse, can easily cause packet drops making this
solution unfit for use in highly loaded production systems.

Users are left with little option than guessing what sampling rate will
be OK for their traffic pattern and dealing with the lost accuracy.

** Feature Description **
In order to solve this situation and enable this feature to be safely
turned on in production, this RFC uses the psample support proposed in
[1] to send NXAST_SAMPLE samples to psample adding the observability
domain and point information as metadata.

~~ API ~~
The API is simply a new field called "psample_group" in the
Flow_Sample_Collector_Set (FSCS) table. Configuring this value is
orthogonal to also associating the FSCS entry to an entry in the IPFIX
table.

Apart from that configuration, the controller needs to add NXAST_SAMPLE
actions that refer the entry's id.

~~ HW Offload ~~
psample is already supported by tc using the act_sample action. The
metadata is taken from the actions' cookie.
This series also adds support for offloading the odp sample action,
only when it includes psample information but not nested actions.

A bit of special care has to be taken in the tc layer to not store the
ufid in the sample's cookie as it's used to carry action-specific data.

~~ Datapath support ~~
This new behavior of the datapth sample action is only supported on the
kernel datapath. A more detailed analysis is needed (and planned) to
find a way to also optimize the userspace datapath. However, although
potentially inefficient, there is not that much risk of dropping packets
with the existing IPFIX infrastructure.

~~ Testing ~~
The series includes an utility program called "ovs-psample" that listens
to packets multicasted by the psample module and prints them (also
printing the obs_domain and obs_point ids). In addition the kernel
series includes a tracepoint for easy testing and troubleshooting.

[1] https://patchwork.kernel.org/project/netdevbpf/list/?series=847473

Hi Andrew,

Who is Andrew? :-D

Sorry :( Handling too many things in parallel...

Thanks for sending out this RFC series. I did a code-only review (no testing),
and the main concern I have (besides some locking) is the direct mapping to
psample. If we decide that for userspace we are going to use USDT probes, we
need another target, and we will duplicate a lot of code. My proposal would be
to have a more general target name like system, or local (or a better name ;).
This will be a system-local dpif target with a 32-bit group id.

Regarding the name, I agree adding "psample" to the API can be confusing for
userspace datapath. Besides, I think it should also be in sync with the odp naming:

"system": My initial thought was it could be confused with the "system@" name
of the kernel datapath, but that is not present in the OVSDB level so it could work.
SAMPLE_ATTR_{SYSTEM_GROUP,SYSTEM_USER_COOKIE}?

"offload": I did consider this but I discarded it as it could be confused with
actual hw/tc offloading of the sample action.

"local": Funny, I initially thought of the opposite, i.e: "external" as the sample collection and
potential encoding (which is currently "local" to ovs-vswitchd) is now some "external" process.
Still I guess the fact you thought of the opposite term suggests it's not expressive enough.

"raw": As in, samples are not IPFIX any more (unless the external collector
decides to export them in IPFIX) but we're exporting the raw packet (alongside some
metadata). SAMPLE_ATTR_{RAW_GROUP, RAW_USER_COOKIE}?

"dpif": As in, it depends on the dpif implementation. However, this term kind of internal to OVS
(it appears only once in ovs-vswitchd.conf.db(5) without much clarification). Its more human synonym
"datapath", or "d

Re: [ovs-dev] [PATCH] table: Fix freeing global variable.

2024-05-14 Thread wangyunjian via dev




> -Original Message-
> From: Ilya Maximets [mailto:i.maxim...@ovn.org]
> Sent: Tuesday, May 14, 2024 3:48 PM
> To: wangyunjian ; d...@openvswitch.org
> Cc: i.maxim...@ovn.org; luyicai ; sunpengfei (G)
> 
> Subject: Re: [ovs-dev][PATCH] table: Fix freeing global variable.
> 
> On 5/14/24 05:25, Yunjian Wang wrote:
> > From: Pengfei Sun 
> >
> > In function shash_replace_nocopy, argument to free() is the address of
> > a global variable (argument passed by function table_print_json__),
> > which is not memory allocated by malloc().
> >
> > ovsdb-client -f json monitor Open_vSwitch --timestamp
> >
> > ASan reports:
> >
> 
> =
> > ==1443083==ERROR: AddressSanitizer: attempting free on address which
> > was not malloc()-ed: 0x00535980 in thread T0
> > #0 0xaf1c9eac in __interceptor_free (/usr/lib64/libasan.so.6+
> > 0xa4eac)
> > #1 0x4826e4 in json_destroy_object lib/json.c:445
> > #2 0x4826e4 in json_destroy__ lib/json.c:403
> > #3 0x4cc4e4 in table_print lib/table.c:633
> > #4 0x410650 in monitor_print_table ovsdb/ovsdb-client.c:1019
> > #5 0x410650 in monitor_print ovsdb/ovsdb-client.c:1040
> > #6 0x4110cc in monitor_print ovsdb/ovsdb-client.c:1030
> > #7 0x4110cc in do_monitor__ ovsdb/ovsdb-client.c:1503
> > #8 0x40743c in main ovsdb/ovsdb-client.c:283
> > #9 0xaeb50038  (/usr/lib64/libc.so.6+0x2b038)
> > #10 0xaeb50110 in __libc_start_main (/usr/lib64/libc.so.6+
> > 0x2b110)
> > #11 0x40906c in _start (/usr/local/bin/ovsdb-client+0x40906c)
> >
> > Use xstrdup to allocate memory for argument "time".
> >
> > Fixes: cb139fa8b3a1 ("table: New function table_format() for
> > formatting a table as a string.")
> > Signed-off-by: Pengfei Sun 
> > ---
> >  lib/table.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Hi.  Thanks for the patch!
> 
> >
> > diff --git a/lib/table.c b/lib/table.c index 48d18b6..bcc6fb0 100644
> > --- a/lib/table.c
> > +++ b/lib/table.c
> > @@ -523,7 +523,7 @@ table_print_json__(const struct table *table, const
> struct table_style *style,
> >  }
> >  if (table->timestamp) {
> >  json_object_put_nocopy(
> 
> I think, we should just remove 'nocopy' part here instead of adding xstrdup()
> below.  json_object_put() will copy the name.

Thanks for your suggestion, will include it in next version.

> 
> > -json, "time",
> > +json, xstrdup("time"),
> >  json_string_create_nocopy(table_format_timestamp__()));
> >  }
> >
> 
> Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] fix ct tp policy when create zone.

2024-05-14 Thread Chandler Wu

Hi, IIya, thanks for the attention.

If I create a zone for the first time, the new tp_cfg will be copied to the
zone, see `ct_zone_alloc`. Then update_timeout_policy will check that the
new copied tp equals to tp_cfg, so ofproto_ct_set_zone_timeout_policy will
not got called. Or you can check tag v3.0.0 or before. There's no such
issue for these versions.

Best regards.

On Tue, May 14, 2024 at 5:11 AM Ilya Maximets  wrote:

> On 5/6/24 06:05, Chandler Wu wrote:
> > From 5b4d479a9083272e56c51ef9521e6ef665dd534d Mon Sep 17 00:00:00 2001
> > From: chandlerwu 
> > Date: Mon, 6 May 2024 11:58:21 +0800
> > Subject: [PATCH] Subject:[PATCH] fix ct tp policy when create zone.
> >
> > Signed-off-by: chandlerwu 
> > ---
> >  vswitchd/bridge.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> > index 95a65fcdc..e60359b59 100644
> > --- a/vswitchd/bridge.c
> > +++ b/vswitchd/bridge.c
> > @@ -766,6 +766,8 @@ ct_zones_reconfigure(struct datapath *dp, struct
> ovsrec_datapath *dp_cfg)
> >  if (!ct_zone) {
> >  ct_zone = ct_zone_alloc(zone_id, tp_cfg);
> >  hmap_insert(&dp->ct_zones, &ct_zone->node,
> hash_int(zone_id, 0));
> > +ofproto_ct_set_zone_timeout_policy(dp->type,
> ct_zone->zone_id,
> > +   &ct_zone->tp);
> >  }
> >
> >  struct simap new_tp = SIMAP_INITIALIZER(&new_tp);
>
> Hi, Chandler Wu.  Thanks for the patch!
>
> But could you, please, explain what is the issue you're trying to fix?
>
> Reading the code, it seems that update_timeout_policy() call later in
> the function should correctly set the policy.  Is that not happening?
>
> Best regards, Ilya Maximets.
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] table: Fix freeing global variable.

On 5/14/24 05:25, Yunjian Wang wrote:
> From: Pengfei Sun 
> 
> In function shash_replace_nocopy, argument to free() is the address
> of a global variable (argument passed by function table_print_json__),
> which is not memory allocated by malloc().
> 
> ovsdb-client -f json monitor Open_vSwitch --timestamp
> 
> ASan reports:
> =
> ==1443083==ERROR: AddressSanitizer: attempting free on address
> which was not malloc()-ed: 0x00535980 in thread T0
> #0 0xaf1c9eac in __interceptor_free (/usr/lib64/libasan.so.6+
> 0xa4eac)
> #1 0x4826e4 in json_destroy_object lib/json.c:445
> #2 0x4826e4 in json_destroy__ lib/json.c:403
> #3 0x4cc4e4 in table_print lib/table.c:633
> #4 0x410650 in monitor_print_table ovsdb/ovsdb-client.c:1019
> #5 0x410650 in monitor_print ovsdb/ovsdb-client.c:1040
> #6 0x4110cc in monitor_print ovsdb/ovsdb-client.c:1030
> #7 0x4110cc in do_monitor__ ovsdb/ovsdb-client.c:1503
> #8 0x40743c in main ovsdb/ovsdb-client.c:283
> #9 0xaeb50038  (/usr/lib64/libc.so.6+0x2b038)
> #10 0xaeb50110 in __libc_start_main (/usr/lib64/libc.so.6+
> 0x2b110)
> #11 0x40906c in _start (/usr/local/bin/ovsdb-client+0x40906c)
> 
> Use xstrdup to allocate memory for argument "time".
> 
> Fixes: cb139fa8b3a1 ("table: New function table_format() for formatting a 
> table as a string.")
> Signed-off-by: Pengfei Sun 
> ---
>  lib/table.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Hi.  Thanks for the patch!

> 
> diff --git a/lib/table.c b/lib/table.c
> index 48d18b6..bcc6fb0 100644
> --- a/lib/table.c
> +++ b/lib/table.c
> @@ -523,7 +523,7 @@ table_print_json__(const struct table *table, const 
> struct table_style *style,
>  }
>  if (table->timestamp) {
>  json_object_put_nocopy(

I think, we should just remove 'nocopy' part here instead of
adding xstrdup() below.  json_object_put() will copy the name.

> -json, "time",
> +json, xstrdup("time"),
>  json_string_create_nocopy(table_format_timestamp__()));
>  }
>  

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

Re: [ovs-dev] [PATCH v2] conntrack: Fully initialize conn struct before insertion.

On 5/13/24 14:38, Simon Horman wrote:
> On Fri, May 10, 2024 at 05:45:54PM +0200, Paolo Valerio wrote:
>> From: Mike Pattrick 
>>
>> In case packets are concurrently received in both directions, there's
>> a chance that the ones in the reverse direction get received right
>> after the connection gets added to the connection tracker but before
>> some of the connection's fields are fully initialized.
>> This could cause OVS to access potentially invalid, as the lookup may
>> end up retrieving the wrong offsets during CONTAINER_OF(), or
>> uninitialized memory.
>>
>> This may happen in case of regular NAT or all-zero SNAT.
>>
>> Fix it by initializing early the connections fields.
>>
>> Fixes: 1116459b3ba8 ("conntrack: Remove nat_conn introducing key 
>> directionality.")
>> Reported-at: https://issues.redhat.com/browse/FDP-616
>> Signed-off-by: Mike Pattrick 
>> Co-authored-by: Paolo Valerio 
>> Signed-off-by: Paolo Valerio 
> 
> Acked-by: Simon Horman 

Thanks, Paolo, Mike and Simon!

Applied and backported down to 2.17.

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

Re: [ovs-dev] [PATCH v2] conntrack: Do not use {0} to initialize unions.

On 5/9/24 13:37, Paolo Valerio wrote:
> Xavier Simonart  writes:
> 
>> In the following case:
>> union ct_addr {
>> unsigned int ipv4;
>> struct in6_addr ipv6;
>> };
>> union ct_addr zero_ip = {0};
>>
>> The ipv6 field might not be properly initialized.
>> For instance, clang 18.1.1 does not initialize the ipv6 field.
>>
>> Reported-at: https://issues.redhat.com/browse/FDP-608
>> Signed-off-by: Xavier Simonart 
>> ---
>> v2: updated based on nit from Paolo.
>> ---
> 
> Thanks Xavier.
> 
> Acked-by: Paolo Valerio 

Thanks, Xavier and Paolo!

Applied and backported down to 2.17.

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

Re: [ovs-dev] [RFC 01/11] odp-util: Add support to new psample uAPI.