Re: [ovs-dev] [FYI][PATCH] tracing/treewide: Remove second parameter of __assign_str()
On Thu, 14 Mar 2024 09:57:57 -0700
Alison Schofield wrote:
> On Fri, Feb 23, 2024 at 12:56:34PM -0500, Steven Rostedt wrote:
> > From: "Steven Rostedt (Google)"
> >
> > [
> >This is a treewide change. I will likely re-create this patch again in
> >the second week of the merge window of v6.9 and submit it then. Hoping
> >to keep the conflicts that it will cause to a minimum.
> > ]
Note, change of plans. I plan on sending this in the next merge window, as
this merge window I have this patch:
https://lore.kernel.org/linux-trace-kernel/20240312113002.00031...@gandalf.local.home/
That will warn if the source string of __string() is different than the
source string of __assign_str(). I want to make sure they are identical
before just dropping one of them.
>
> > diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h
> > index bdf117a33744..07ba4e033347 100644
> > --- a/drivers/cxl/core/trace.h
> > +++ b/drivers/cxl/core/trace.h
>
> snip to poison
>
> > @@ -668,8 +668,8 @@ TRACE_EVENT(cxl_poison,
> > ),
> >
> > TP_fast_assign(
> > - __assign_str(memdev, dev_name(>dev));
> > - __assign_str(host, dev_name(cxlmd->dev.parent));
> > + __assign_str(memdev);
> > + __assign_str(host);
>
> I think I get that the above changes work because the TP_STRUCT__entry for
> these did:
> __string(memdev, dev_name(>dev))
> __string(host, dev_name(cxlmd->dev.parent))
That's the point. They have to be identical or you will likely bug.
The __string(name, src) is used to find the string length of src which
allocates the necessary length on the ring buffer. The __assign_str(name, src)
will copy src into the ring buffer.
Similar to:
len = strlen(src);
buf = malloc(len);
strcpy(buf, str);
Where __string() is strlen() and __assign_str() is strcpy(). It doesn't
make sense to use two different strings, and if you did, it would likely be
a bug.
But the magic behind __string() does much more than just get the length of
the string, and it could easily save the pointer to the string (along with
its length) and have it copy that in the __assign_str() call, making the
src parameter of __assign_str() useless.
>
> > __entry->serial = cxlmd->cxlds->serial;
> > __entry->overflow_ts = cxl_poison_overflow(flags, overflow_ts);
> > __entry->dpa = cxl_poison_record_dpa(record);
> > @@ -678,12 +678,12 @@ TRACE_EVENT(cxl_poison,
> > __entry->trace_type = trace_type;
> > __entry->flags = flags;
> > if (region) {
> > - __assign_str(region, dev_name(>dev));
> > + __assign_str(region);
> > memcpy(__entry->uuid, >params.uuid, 16);
> > __entry->hpa = cxl_trace_hpa(region, cxlmd,
> > __entry->dpa);
> > } else {
> > - __assign_str(region, "");
> > + __assign_str(region);
> > memset(__entry->uuid, 0, 16);
> > __entry->hpa = ULLONG_MAX;
>
> For the above 2, there was no helper in TP_STRUCT__entry. A recently
> posted patch is fixing that up to be __string(region, NULL) See [1],
> with the actual assignment still happening in TP_fast_assign.
__string(region, NULL) doesn't make sense. It's like:
len = strlen(NULL);
buf = malloc(len);
strcpy(buf, NULL);
??
I'll reply to that email.
-- Steve
>
> Does that assign logic need to move to the TP_STRUCT__entry definition
> when you merge these changes? I'm not clear how much logic is able to be
> included, ie like 'C' style code in the TP_STRUCT__entry.
>
> [1]
> https://lore.kernel.org/linux-cxl/20240314044301.2108650-1-alison.schofi...@intel.com/
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH OVN] Add support to make fdb table local to the chassis.
> On 14-Mar-2024, at 9:07 PM, Dumitru Ceara wrote:
>
> On 3/14/24 15:21, Naveen Yerramneni wrote:
>>
>>
>>> On 08-Mar-2024, at 2:37 PM, Ales Musil wrote:
>>>
>>>
>>>
>>> On Wed, Mar 6, 2024 at 8:24 PM Naveen Yerramneni
>>> wrote:
>>>
>>>
On 18-Dec-2023, at 8:53 PM, Dumitru Ceara wrote:
On 12/18/23 16:17, Naveen Yerramneni wrote:
>
>
>> On 18-Dec-2023, at 7:26 PM, Dumitru Ceara wrote:
>>
>> On 11/30/23 16:32, Dumitru Ceara wrote:
>>> On 11/30/23 15:54, Naveen Yerramneni wrote:
> On 30-Nov-2023, at 6:06 PM, Dumitru Ceara wrote:
>
> On 11/30/23 09:45, Naveen Yerramneni wrote:
>>
>>
>>> On 29-Nov-2023, at 2:24 PM, Dumitru Ceara wrote:
>>>
>>> On 11/29/23 07:45, naveen.yerramneni wrote:
This functionality can be enabled at the logical switch level:
- "other_config:fdb_local" can be used to enable/disable this
functionality, it is disabled by default.
- "other_config:fdb_local_idle_timeout" sepcifies idle timeout
for locally learned fdb flows, default timeout is 300 secs.
If enabled, below lflow is added for each port that has unknown
addr set.
- table=2 (ls_in_lookup_fdb), priority=100, match=(inport ==
),
action=(commit_fdb_local(timeout=); next;
New OVN action: "commit_fdb_local". This sets following OVS action.
-
learn(table=71,idle_timeout=,delete_learned,OXM_OF_METADATA[],
NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[],load:NXM_NX_REG14[]->NXM_NX_REG15[])
This is useful when OVN is managing VLAN network that has multiple
ports
set with unknown addr and localnet_learn_fdb is enabled. With this
config,
if there is east-west traffic flowing between VMs part of same VLAN
deployed on different hypervisors then, MAC addrs of the source and
destination VMs keeps flapping between VM port and localnet port
in
Southbound FDB table. Enabling fdb_local config makes fdb table
local to
the chassis and avoids MAC flapping.
Signed-off-by: Naveen Yerramneni
---
>>>
>>> Hi Naveen,
>>>
>>> Thanks a lot for the patch!
>>>
>>> Just a note, we already have a fix for the east-west traffic that
>>> causes
>>> FDB flapping when localnet is used:
>>>
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_ovn-2Dorg_ovn_commit_2acf91e9628e9481c48e4a6cec8ad5159fdd6d2e=DwICaQ=s883GpUCOChKOHiocYtGcg=2PQjSDR7A28z1kXE1ptSm6X36oL_nCq1XxeEt7FkLmA=kPuq992rikXYk63APGxlIpfqY3lPpreN9f4ha9pZKpodnVgE9KfjEUNozpPUFzUu=LP9_zs2Rj34vMx20ntbu-A3taXqKMJNVH2TLQyOXCh0=
>>>
>>>
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_ovn-2Dorg_ovn_commit_f3a14907fe2b1ecdcfddfbed595cd097b6efbe14=DwICaQ=s883GpUCOChKOHiocYtGcg=2PQjSDR7A28z1kXE1ptSm6X36oL_nCq1XxeEt7FkLmA=kPuq992rikXYk63APGxlIpfqY3lPpreN9f4ha9pZKpodnVgE9KfjEUNozpPUFzUu=gsUGtjyf9gSOr1LkcCH0O6MB1_tjXi9fuTgwEFgbRx8=
>>>
>>>
>>> In general, however, I think it's a very good idea to move the FDB
>>> away
>>> from the Southbound and make it local to each hypervisor. That
>>> reduces
>>> load on the Southbound among other things.
>>>
>>
>> Hi Dumitru,
>>
>> Thanks for informing about the patches.
>> Yes, local FDB reduces load on southbound.
>>
>>
include/ovn/actions.h | 7 +++
lib/actions.c | 94
northd/northd.c | 26 ++
ovn-nb.xml| 14 ++
tests/ovn.at [ovn.at] | 108
++
utilities/ovn-trace.c | 2 +
6 files changed, 251 insertions(+)
diff --git a/include/ovn/actions.h b/include/ovn/actions.h
index 49cfe0624..85ac92cd3 100644
--- a/include/ovn/actions.h
+++ b/include/ovn/actions.h
@@ -127,6 +127,7 @@ struct collector_set_ids;
OVNACT(CHK_LB_AFF,ovnact_result) \
OVNACT(SAMPLE,ovnact_sample) \
OVNACT(MAC_CACHE_USE, ovnact_null)\
+OVNACT(COMMIT_FDB_LOCAL, ovnact_commit_fdb_local) \
/* enum ovnact_type, with a member OVNACT_ for each action.
*/
enum OVS_PACKED_ENUM ovnact_type {
@@ -514,6 +515,12 @@ struct
Re: [ovs-dev] [PATCH OVN] Add support to make fdb table local to the chassis.
On 3/14/24 15:21, Naveen Yerramneni wrote:
>
>
>> On 08-Mar-2024, at 2:37 PM, Ales Musil wrote:
>>
>>
>>
>> On Wed, Mar 6, 2024 at 8:24 PM Naveen Yerramneni
>> wrote:
>>
>>
>>> On 18-Dec-2023, at 8:53 PM, Dumitru Ceara wrote:
>>>
>>> On 12/18/23 16:17, Naveen Yerramneni wrote:
> On 18-Dec-2023, at 7:26 PM, Dumitru Ceara wrote:
>
> On 11/30/23 16:32, Dumitru Ceara wrote:
>> On 11/30/23 15:54, Naveen Yerramneni wrote:
>>>
>>>
On 30-Nov-2023, at 6:06 PM, Dumitru Ceara wrote:
On 11/30/23 09:45, Naveen Yerramneni wrote:
>
>
>> On 29-Nov-2023, at 2:24 PM, Dumitru Ceara wrote:
>>
>> On 11/29/23 07:45, naveen.yerramneni wrote:
>>> This functionality can be enabled at the logical switch level:
>>> - "other_config:fdb_local" can be used to enable/disable this
>>> functionality, it is disabled by default.
>>> - "other_config:fdb_local_idle_timeout" sepcifies idle timeout
>>> for locally learned fdb flows, default timeout is 300 secs.
>>>
>>> If enabled, below lflow is added for each port that has unknown
>>> addr set.
>>> - table=2 (ls_in_lookup_fdb), priority=100, match=(inport ==
>>> ),
>>> action=(commit_fdb_local(timeout=); next;
>>>
>>> New OVN action: "commit_fdb_local". This sets following OVS action.
>>> -
>>> learn(table=71,idle_timeout=,delete_learned,OXM_OF_METADATA[],
>>>
>>> NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[],load:NXM_NX_REG14[]->NXM_NX_REG15[])
>>>
>>> This is useful when OVN is managing VLAN network that has multiple
>>> ports
>>> set with unknown addr and localnet_learn_fdb is enabled. With this
>>> config,
>>> if there is east-west traffic flowing between VMs part of same VLAN
>>> deployed on different hypervisors then, MAC addrs of the source and
>>> destination VMs keeps flapping between VM port and localnet port in
>>> Southbound FDB table. Enabling fdb_local config makes fdb table
>>> local to
>>> the chassis and avoids MAC flapping.
>>>
>>> Signed-off-by: Naveen Yerramneni
>>> ---
>>
>> Hi Naveen,
>>
>> Thanks a lot for the patch!
>>
>> Just a note, we already have a fix for the east-west traffic that
>> causes
>> FDB flapping when localnet is used:
>>
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_ovn-2Dorg_ovn_commit_2acf91e9628e9481c48e4a6cec8ad5159fdd6d2e=DwICaQ=s883GpUCOChKOHiocYtGcg=2PQjSDR7A28z1kXE1ptSm6X36oL_nCq1XxeEt7FkLmA=kPuq992rikXYk63APGxlIpfqY3lPpreN9f4ha9pZKpodnVgE9KfjEUNozpPUFzUu=LP9_zs2Rj34vMx20ntbu-A3taXqKMJNVH2TLQyOXCh0=
>>
>>
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_ovn-2Dorg_ovn_commit_f3a14907fe2b1ecdcfddfbed595cd097b6efbe14=DwICaQ=s883GpUCOChKOHiocYtGcg=2PQjSDR7A28z1kXE1ptSm6X36oL_nCq1XxeEt7FkLmA=kPuq992rikXYk63APGxlIpfqY3lPpreN9f4ha9pZKpodnVgE9KfjEUNozpPUFzUu=gsUGtjyf9gSOr1LkcCH0O6MB1_tjXi9fuTgwEFgbRx8=
>>
>>
>> In general, however, I think it's a very good idea to move the FDB
>> away
>> from the Southbound and make it local to each hypervisor. That
>> reduces
>> load on the Southbound among other things.
>>
>
> Hi Dumitru,
>
> Thanks for informing about the patches.
> Yes, local FDB reduces load on southbound.
>
>
>>> include/ovn/actions.h | 7 +++
>>> lib/actions.c | 94
>>> northd/northd.c | 26 ++
>>> ovn-nb.xml| 14 ++
>>> tests/ovn.at [ovn.at] | 108
>>> ++
>>> utilities/ovn-trace.c | 2 +
>>> 6 files changed, 251 insertions(+)
>>>
>>> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
>>> index 49cfe0624..85ac92cd3 100644
>>> --- a/include/ovn/actions.h
>>> +++ b/include/ovn/actions.h
>>> @@ -127,6 +127,7 @@ struct collector_set_ids;
>>> OVNACT(CHK_LB_AFF,ovnact_result) \
>>> OVNACT(SAMPLE,ovnact_sample) \
>>> OVNACT(MAC_CACHE_USE, ovnact_null)\
>>> +OVNACT(COMMIT_FDB_LOCAL, ovnact_commit_fdb_local) \
>>>
>>> /* enum ovnact_type, with a member OVNACT_ for each action. */
>>> enum OVS_PACKED_ENUM ovnact_type {
>>> @@ -514,6 +515,12 @@ struct ovnact_commit_lb_aff {
>>> uint16_t timeout;
>>> };
>>>
>>> +/* OVNACT_COMMIT_FBD_LOCAL. */
>>> +struct ovnact_commit_fdb_local{
>>> +struct ovnact ovnact;
>>> +
Re: [ovs-dev] [PATCH OVN v2] DHCP Relay Agent support for overlay subnets.
On Sun, Mar 3, 2024 at 11:20 PM Naveen Yerramneni wrote: > > This patch contains changes to enable DHCP Relay Agent support for > overlay subnets. > > USE CASE: > -- > - Enable IP address assignment for overlay subnets from the centralized > DHCP server present in the underlay network. > > PREREQUISITES > -- > - Logical Router Port IP should be assigned (statically) from the same > overlay subnet which is managed by DHCP server. > - LRP IP is used for GIADRR field when relaying the DHCP packets and > also same IP needs to be configured as default gateway for the overlay subnet. > - Overlay subnets managed by external DHCP server are expected to be > directly reachable from the underlay network. > > EXPECTED PACKET FLOW: > -- > Following is the expected packet flow inorder to support DHCP rleay > functionality in OVN. > 1. DHCP client originates DHCP discovery (broadcast). > 2. DHCP relay (running on the OVN) receives the broadcast and forwards > the packet to the DHCP server by converting it to unicast. > While forwarding the packet, it updates the GIADDR in DHCP header to > its interface IP on which DHCP packet is received and increments hop count. > 3. DHCP server uses GIADDR field to decide the IP address pool from > which IP has to be assigned and DHCP offer is sent to the same IP (GIADDR). > 4. DHCP relay agent forwards the offer to the client. > 5. DHCP client sends DHCP request (broadcast) packet. > 6. DHCP relay (running on the OVN) receives the broadcast and forwards > the packet to the DHCP server by converting it to unicast. > While forwarding the packet, it updates the GIADDR in DHCP header to > its interface IP on which DHCP packet is received. > 7. DHCP Server sends the ACK packet. > 8. DHCP relay agent forwards the ACK packet to the client. > 9. All the future renew/release packets are directly exchanged between > DHCP client and DHCP server. > > OVN DHCP RELAY PACKET FLOW: > > To add DHCP Relay support on OVN, we need to replicate all the behavior > described above using distributed logical switch and logical router. > At, highlevel packet flow is distributed among Logical Switch and Logical > Router on source node (where VM is deployed) and redirect chassis(RC) node. > 1. Request packet gets processed on the source node where VM is > deployed and relays the packet to DHCP server. > 2. Response packet is first processed on RC node (which first recieves > the packet from underlay network). RC node forwards the packet to the right > node by filling in the dest MAC and IP. > > OVN Packet flow with DHCP relay is explained below. > 1. DHCP client (VM) sends the DHCP discover packet (broadcast). > 2. Logical switch converts the packet to L2 unicast by setting the > destination MAC to LRP's MAC > 3. Logical Router receives the packet and redirects it to the OVN > controller. > 4. OVN controller updates the required information(GIADDR, HOP count) > in the DHCP payload after doing the required checks. If any check fails, > packet is dropped. > 5. Logical Router converts the packet to L3 unicast and forwards it to > the server. This packets gets routed like any other packet (via RC node). > 6. Server replies with DHCP offer. > 7. RC node processes the DHCP offer and forwards it to the OVN > controller. > 8. OVN controller does sanity checks and updates the destination MAC > (available in DHCP header), destination IP (available in DHCP header) and > reinjects the packet to datapath. > If any check fails, packet is dropped. > 9. Logical router updates the source IP and port and forwards the > packet to logical switch. > 10. Logical switch delivers the packet to the DHCP client. > 11. Similar steps are performed for Request and Ack packets. > 12. All the future renew/release packets are directly exchanged between > DHCP client and DHCP server > > NEW OVN ACTIONS > --- > 1. dhcp_relay_req_chk(, ) > - This action executes on the source node on which the DHCP request > originated. > - This action relays the DHCP request coming from client to the > server. Relay-ip is used to update GIADDR in the DHCP header. > 2. dhcp_relay_resp_chk(, ) > - This action executes on the first node (RC node) which processes > the DHCP response from the server. > - This action updates the destination MAC and destination IP so > that the response can be forwarded to the appropriate node from which request > was originated. > - Relay-ip, server-ip are used to validate GIADDR and SERVER ID in > the DHCP payload. > > FLOWS > - > Following are the flows added when DHCP Relay is configured on one
Re: [ovs-dev] [PATCH OVN] Add support to make fdb table local to the chassis.
> On 08-Mar-2024, at 2:37 PM, Ales Musil wrote:
>
>
>
> On Wed, Mar 6, 2024 at 8:24 PM Naveen Yerramneni
> wrote:
>
>
> > On 18-Dec-2023, at 8:53 PM, Dumitru Ceara wrote:
> >
> > On 12/18/23 16:17, Naveen Yerramneni wrote:
> >>
> >>
> >>> On 18-Dec-2023, at 7:26 PM, Dumitru Ceara wrote:
> >>>
> >>> On 11/30/23 16:32, Dumitru Ceara wrote:
> On 11/30/23 15:54, Naveen Yerramneni wrote:
> >
> >
> >> On 30-Nov-2023, at 6:06 PM, Dumitru Ceara wrote:
> >>
> >> On 11/30/23 09:45, Naveen Yerramneni wrote:
> >>>
> >>>
> On 29-Nov-2023, at 2:24 PM, Dumitru Ceara wrote:
>
> On 11/29/23 07:45, naveen.yerramneni wrote:
> > This functionality can be enabled at the logical switch level:
> > - "other_config:fdb_local" can be used to enable/disable this
> > functionality, it is disabled by default.
> > - "other_config:fdb_local_idle_timeout" sepcifies idle timeout
> > for locally learned fdb flows, default timeout is 300 secs.
> >
> > If enabled, below lflow is added for each port that has unknown
> > addr set.
> > - table=2 (ls_in_lookup_fdb), priority=100, match=(inport ==
> > ),
> > action=(commit_fdb_local(timeout=); next;
> >
> > New OVN action: "commit_fdb_local". This sets following OVS action.
> > -
> > learn(table=71,idle_timeout=,delete_learned,OXM_OF_METADATA[],
> >
> > NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[],load:NXM_NX_REG14[]->NXM_NX_REG15[])
> >
> > This is useful when OVN is managing VLAN network that has multiple
> > ports
> > set with unknown addr and localnet_learn_fdb is enabled. With this
> > config,
> > if there is east-west traffic flowing between VMs part of same VLAN
> > deployed on different hypervisors then, MAC addrs of the source and
> > destination VMs keeps flapping between VM port and localnet port in
> > Southbound FDB table. Enabling fdb_local config makes fdb table
> > local to
> > the chassis and avoids MAC flapping.
> >
> > Signed-off-by: Naveen Yerramneni
> > ---
>
> Hi Naveen,
>
> Thanks a lot for the patch!
>
> Just a note, we already have a fix for the east-west traffic that
> causes
> FDB flapping when localnet is used:
>
> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_ovn-2Dorg_ovn_commit_2acf91e9628e9481c48e4a6cec8ad5159fdd6d2e=DwICaQ=s883GpUCOChKOHiocYtGcg=2PQjSDR7A28z1kXE1ptSm6X36oL_nCq1XxeEt7FkLmA=kPuq992rikXYk63APGxlIpfqY3lPpreN9f4ha9pZKpodnVgE9KfjEUNozpPUFzUu=LP9_zs2Rj34vMx20ntbu-A3taXqKMJNVH2TLQyOXCh0=
>
>
> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_ovn-2Dorg_ovn_commit_f3a14907fe2b1ecdcfddfbed595cd097b6efbe14=DwICaQ=s883GpUCOChKOHiocYtGcg=2PQjSDR7A28z1kXE1ptSm6X36oL_nCq1XxeEt7FkLmA=kPuq992rikXYk63APGxlIpfqY3lPpreN9f4ha9pZKpodnVgE9KfjEUNozpPUFzUu=gsUGtjyf9gSOr1LkcCH0O6MB1_tjXi9fuTgwEFgbRx8=
>
>
> In general, however, I think it's a very good idea to move the FDB
> away
> from the Southbound and make it local to each hypervisor. That
> reduces
> load on the Southbound among other things.
>
> >>>
> >>> Hi Dumitru,
> >>>
> >>> Thanks for informing about the patches.
> >>> Yes, local FDB reduces load on southbound.
> >>>
> >>>
> > include/ovn/actions.h | 7 +++
> > lib/actions.c | 94
> > northd/northd.c | 26 ++
> > ovn-nb.xml| 14 ++
> > tests/ovn.at [ovn.at] | 108
> > ++
> > utilities/ovn-trace.c | 2 +
> > 6 files changed, 251 insertions(+)
> >
> > diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> > index 49cfe0624..85ac92cd3 100644
> > --- a/include/ovn/actions.h
> > +++ b/include/ovn/actions.h
> > @@ -127,6 +127,7 @@ struct collector_set_ids;
> > OVNACT(CHK_LB_AFF,ovnact_result) \
> > OVNACT(SAMPLE,ovnact_sample) \
> > OVNACT(MAC_CACHE_USE, ovnact_null)\
> > +OVNACT(COMMIT_FDB_LOCAL, ovnact_commit_fdb_local) \
> >
> > /* enum ovnact_type, with a member OVNACT_ for each action. */
> > enum OVS_PACKED_ENUM ovnact_type {
> > @@ -514,6 +515,12 @@ struct ovnact_commit_lb_aff {
> > uint16_t timeout;
> > };
> >
> > +/* OVNACT_COMMIT_FBD_LOCAL. */
> > +struct ovnact_commit_fdb_local{
> > +struct ovnact ovnact;
> > +uint16_t timeout;
Re: [ovs-dev] [PATCH 3/3] netdev-dpdk: Fix tunnel type check during Tx offload preparation.
On Wed, Mar 13, 2024 at 1:29 PM Ilya Maximets wrote:
>
> Tunnel types are not flags, but 4-bit fields, so checking them with
> a simple binary 'and' is incorrect and may produce false-positive
> matches.
>
> While the current implementation is unlikely to cause any issues today,
> since both RTE_MBUF_F_TX_TUNNEL_VXLAN and RTE_MBUF_F_TX_TUNNEL_GENEVE
> only have 1 bit set, it is risky to have this code and it may lead
> to problems if we add support for other tunnel types in the future.
>
> Use proper field checks instead. Also adding a warning for unexpected
> tunnel types in case something goes wrong.
>
> Fixes: 084c8087292c ("userspace: Support VXLAN and GENEVE TSO.")
> Signed-off-by: Ilya Maximets
Acked-by: Mike Pattrick
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2/3] netdev-dpdk: Fix TCP check during Tx offload preparation.
On Wed, Mar 13, 2024 at 1:29 PM Ilya Maximets wrote:
>
> RTE_MBUF_F_TX_TCP_CKSUM is not a flag, but a 2-bit field, so checking
> it with a simple binary 'and' is incorrect. For example, this check
> will succeed for a packet with UDP checksum requested as well.
>
> Fix the check to avoid wrongly initializing tso_segz and potentially
> accessing UDP header via TCP structure pointer.
>
> The IPv4 checksum flag has to be set for any L4 checksum request,
> regardless of the type, so moving this check out of the TCP condition.
>
> Fixes: 8b5fe2dc6080 ("userspace: Add Generic Segmentation Offloading.")
> Signed-off-by: Ilya Maximets
Acked-by: Mike Pattrick
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 1/3] netdev-dpdk: Clear inner packet marks if no inner offloads requested.
On Wed, Mar 13, 2024 at 1:29 PM Ilya Maximets wrote:
>
> In some cases only outer offloads may be requested for a tunneled
> packet. In this case there is no need to mark the type of an
> inner packet. Clean these flags up to avoid potential confusion
> of DPDK drivers.
>
> Fixes: 084c8087292c ("userspace: Support VXLAN and GENEVE TSO.")
> Signed-off-by: Ilya Maximets
Acked-by: Mike Pattrick
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [Patch ovn v2 2/2] northd: Fix direct access to SNAT network on DR.
Hello all,
I have one more follow-up regarding the comments in v1. @amusil, you were
concerned about the impact this change would have on the performance so I
ran some tests to try to gauge it. I used following setup with two physical
hosts connected over switch:
|Physical ext. | Hypervisor1
| network|
alice1-|- switch -|-- DLR --- foo1
| |
* alice1 acts as external device.
* It doesn't run OVN
* It's connected to regular physical network.
* Hypervisor1 runs OVN chassis
* foo1 is a container behind Distributed LR with SNAT enabled.
Goal of this test was to measure whether the proposed patch affects
throughput of the traffic that flows from "foo1" to external network. I
used iperf3 and ran 3x 5 minute measurements with 10 streams ("iperf3 -i 0
-t 300 -P 10 -c alice1"). I used 5 minute for each measurement to smooth
out possible jitter and I ran each measurement 3 times to get feel for the
variance in the network throughput over longer period. I also did some
trial and error testing that showed that 10 parallel streams reached
highest throughput.
Following are the three (summary) results with this patch applied:
# run 1 (with patch)
[SUM] 0.00-300.00 sec 296 GBytes 8.48 Gbits/sec sender
[SUM] 0.00-300.00 sec 296 GBytes 8.48 Gbits/sec receiver
# run 2 (with patch)
[SUM] 0.00-300.00 sec 288 GBytes 8.24 Gbits/sec sender
[SUM] 0.00-300.00 sec 288 GBytes 8.23 Gbits/sec receiver
# run 3 (with patch)
[SUM] 0.00-300.00 sec 299 GBytes 8.55 Gbits/sec sender
[SUM] 0.00-300.00 sec 299 GBytes 8.55 Gbits/sec receiver
Next thing I did was to rebuild ovn-controller without these patches,
replaced the ovn-controller binary on the Hypervisor1 and restarted
ovn-controller daemon. I verified that the feature flag "ct-commit-to-zone"
was removed from the chassis (in the SB database), which forced the
recompute, and then I reran the tests:
# run 1 (clean)
[SUM] 0.00-300.00 sec 300 GBytes 8.59 Gbits/sec sender
[SUM] 0.00-300.00 sec 300 GBytes 8.59 Gbits/sec receiver
# run 2 (clean)
[SUM] 0.00-300.00 sec 285 GBytes 8.15 Gbits/sec sender
[SUM] 0.00-300.00 sec 285 GBytes 8.15 Gbits/sec receiver
# run 3 (clean)
[SUM] 0.00-300.00 sec 295 GBytes 8.46 Gbits/sec sender
[SUM] 0.00-300.00 sec 295 GBytes 8.46 Gbits/sec receiver
These tests show that while there was some fluctuation between each
individual test, when comparing patched and clean version, they come out
about the same. I'm happy to run more tests/scenarios if you have something
else in mind that should be tested.
On Wed, Mar 13, 2024 at 10:17 AM Martin Kalcok
wrote:
> Regarding the failed unstable test in the CI, I suspect that this is not
> related to the code change, I've had couple successful CI runs in my branch
> [0].
>
> Martin.
>
> [0] https://github.com/mkalcok/ovn/actions/runs/8256539983
>
> On Tue, Mar 12, 2024 at 8:45 PM Martin Kalcok
> wrote:
>
>> This change fixes bug that breaks ability of machines from external
>> networks to communicate with machines in SNATed networks (specifically
>> when using a Distributed router).
>>
>> Currently when a machine (S1) on an external network tries to talk
>> over TCP with a machine (A1) in a network that has enabled SNAT, the
>> connection is established successfully. However after the three-way
>> handshake, any packets that come from the A1 machine will have their
>> source address translated by the Distributed router, breaking the
>> communication.
>>
>> Existing rule in `build_lrouter_out_snat_flow` that decides which
>> packets should be SNATed already tries to avoid SNATing packets in
>> reply direction with `(!ct.trk || !ct.rpl)`. However, previous stages
>> in the distributed LR egress pipeline do not initiate the CT state.
>>
>> Additionally we need to commit new connections that originate from
>> external networks into CT, so that the packets in the reply direction
>> can be properly identified.
>>
>> Rationale:
>>
>> In my original RFC [0], there were questions about the motivation for
>> fixing this issue. I'll try to summarize why I think this is a bug
>> that should be fixed.
>>
>> 1. Current implementation for Distributed router already tries to
>>avoid SNATing packets in the reply direction, it's just missing
>>initialized CT states to make proper decisions.
>>
>> 2. This same scenario works with Gateway Router. I tested with
>>following setup:
>>
>> foo -- R1 -- join - R3 -- alice
>> |
>> bar --R2
>>
>> R1 is a Distributed router with SNAT for foo. R2 is a Gateway
>> router with SNAT for bar. R3 is a Gateway router with no SNAT.
>> Using 'alice1' as a client I was able to talk over TCP with
>> 'bar1' but connection with 'foo1' failed.
>>
>> 3. Regarding security and "leaking" of internal IPs. Reading through
>>RFC 4787 [1], 5382 [2] and their update in 7857 [3], the
>>specifications do not seem to
[ovs-dev] [PATCH] ofproto-dpif-upcall: try lock for umap iteration during sweep
A potential race condition happened with the following 3 threads:
* PMD thread replaced the old_ukey and transitioned the state to
UKEY_DELETED.
* RCU thread is freeing the old_ukey mutex.
* While the revalidator thread is trying to lock the old_ukey mutex.
Then vswitchd process aborts at the revalidator thread try_lock of
ukey->mutex because of the NULL pointer.
This patch adds the try_lock for the ukeys' basket umap to avoid
the PMD and revalidator access to the same umap for replacing the
ukey and transitioning the ukey state.
More details can be found at:
[1] https://mail.openvswitch.org/pipermail/ovs-discuss/2024-March/052973.html
[2] https://mail.openvswitch.org/pipermail/ovs-discuss/2024-February/052949.html
Signed-off-by: LIU Yulong
---
ofproto/ofproto-dpif-upcall.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 9a5c5c29c..ef13f820a 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -2974,6 +2974,10 @@ revalidator_sweep__(struct revalidator *revalidator,
bool purge)
struct umap *umap = >ukeys[i];
size_t n_ops = 0;
+if (ovs_mutex_trylock(>mutex)) {
+continue;
+}
+
CMAP_FOR_EACH(ukey, cmap_node, >cmap) {
enum ukey_state ukey_state;
@@ -3013,9 +3017,7 @@ revalidator_sweep__(struct revalidator *revalidator, bool
purge)
if (ukey_state == UKEY_EVICTED) {
/* The common flow deletion case involves deletion of the flow
* during the dump phase and ukey deletion here. */
-ovs_mutex_lock(>mutex);
ukey_delete(umap, ukey);
-ovs_mutex_unlock(>mutex);
}
if (n_ops == REVALIDATE_MAX_BATCH) {
@@ -3025,6 +3027,7 @@ revalidator_sweep__(struct revalidator *revalidator, bool
purge)
n_ops = 0;
}
}
+ovs_mutex_unlock(>mutex);
if (n_ops) {
push_ukey_ops(udpif, umap, ops, n_ops);
--
2.27.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
