Re: [ovs-dev] [v3] odp-execute: Fix AVX checksum calculation.
think 2) is what is being asked above, but wanted to make sure the tradeoffs > are understood. As above, please indicate which is the preferred approach and > a v4 will be on the way. > Option 4: Build a specific unit test that that will test for regression for this particular issue. Send the patch out. Then move all the existing fuzzing tests into a separate test target, e.g. 'make check-fuzz-autovalidator' (maybe there is a better name), add fuzzy actions test there. Make some CI (realistically, Intel CI, because other CI systems do not have AVX512) run it periodically or semi-continuously and report failures to the mailing list, so fixes can be developed and regression tests added to the main unit test suite. What do you think? FWIW, I collected a few samples that can be used for the regression test: eth_src=8a:bf:7e:2f:05:84,eth_dst=0a:8f:39:4f:e0:73,dl_type=0x0800, nw_src=9.77.254.180,nw_dst=216.53.194.20,nw_proto=6,nw_ttl=64,nw_frag=no, tp_src=14303,tp_dst=1044,tcp_flags=ack eth_src=8a:bf:7e:2f:05:84,eth_dst=0a:8f:39:4f:e0:73,dl_type=0x0800, nw_src=108.72.150.185,nw_dst=247.95.158.114,nw_proto=6,nw_ttl=64,nw_frag=no, tp_src=32598,tp_dst=7421,tcp_flags=ack eth_src=8a:bf:7e:2f:05:84,eth_dst=0a:8f:39:4f:e0:73,dl_type=0x0800, nw_src=106.187.156.70,nw_dst=117.142.150.202,nw_proto=6,nw_ttl=64,nw_frag=no, tp_src=10122,tp_dst=1462,tcp_flags=ack eth_src=8a:bf:7e:2f:05:84,eth_dst=0a:8f:39:4f:e0:73,dl_type=0x86dd, ipv6_src=2f8a:2076:3926:9e7:2d47:4bc9:9c7:17f3, ipv6_dst=7287:10dd:2fb9:41d5:3eb2:2c7a:11b0:6258, ipv6_label=0x51ac,nw_proto=6,nw_ttl=142,nw_frag=no, tp_src=20405,tp_dst=20662,tcp_flags=ack eth_src=8a:bf:7e:2f:05:84,eth_dst=0a:8f:39:4f:e0:73,dl_type=0x86dd, ipv6_src=630c:659b:2561:38e1:6989:4f2b:3386:51f6, ipv6_dst=658e:6f2:29fc:35b:5fdf:6558:119:40bc, ipv6_label=0x721b,nw_proto=6,nw_ttl=20,nw_frag=no, tp_src=2632,tp_dst=15427,tcp_flags=ack eth_src=8a:bf:7e:2f:05:84,eth_dst=0a:8f:39:4f:e0:73,dl_type=0x86dd, ipv6_src=16bf:5603:6d40:1620:2a8e:6a83:4348:6136, ipv6_dst=5003:74fd:47cd:6b65:353c:7fc1:5ac5:7d08, ipv6_label=0x7661,nw_proto=6,nw_ttl=214,nw_frag=no, tp_src=21024,tp_dst=23624,tcp_flags=ack Feeding them into 'ovs-ofctl compose-packet --bare' produces valid packets that trigger the issue. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] srv6: Fix misaligned writes to segment list.
On 5/20/24 03:54, Nobuhiro MIKI wrote:
> On 2024/05/18 3:33, Ilya Maximets wrote:
>> Segments list in SRv6 header is 16-bit aligned as most of other fields
>> in packet headers. A little counter-intuitively, compilers are allowed
>> to make alignment assumptions based on the pointer type passed to
>> memcpy(), so they can use copy instructions that require 32-bit alignment
>> in case of struct in6_addr pointer. Reported by UBsan in Clang 18:
>>
>> lib/netdev-native-tnl.c:985:16: runtime error: store to misaligned
>>address 0x7fd9e97351ce for type 'struct in6_addr *', which
>>requires 4 byte alignment
>> 0x7fd9e97351ce: note: pointer points here
>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> ^
>>0 0xc1de38 in netdev_srv6_build_header lib/netdev-native-tnl.c:985:9
>>1 0x6e794b in tnl_port_build_header ofproto/tunnel.c:751:11
>>2 0x6c9c0a in native_tunnel_output ofproto/ofproto-dpif-xlate.c:3887:11
>>3 0x6c9c0a in compose_output_action__ ofproto/ofproto-dpif-xlate.c:4502:13
>>4 0x6b6646 in compose_output_action ofproto/ofproto-dpif-xlate.c:4564:5
>>5 0x6b6646 in xlate_output_action ofproto/ofproto-dpif-xlate.c:5517:13
>>6 0x68cfee in do_xlate_actions ofproto/ofproto-dpif-xlate.c:7288:13
>>7 0x67fed0 in xlate_actions ofproto/ofproto-dpif-xlate.c:8314:13
>>8 0x6468bd in ofproto_trace__ ofproto/ofproto-dpif-trace.c:782:30
>>9 0x64484a in ofproto_trace ofproto/ofproto-dpif-trace.c:851:5
>> 10 0x647469 in ofproto_unixctl_trace ofproto/ofproto-dpif-trace.c:490:9
>> 11 0xc33771 in process_command lib/unixctl.c:310:13
>> 12 0xc33771 in run_connection lib/unixctl.c:344:17
>> 13 0xc33771 in unixctl_server_run lib/unixctl.c:395:21
>> 14 0x53e6ef in main vswitchd/ovs-vswitchd.c:131:9
>> 15 0x7f61c7 in __libc_start_call_main (/lib64/libc.so.6+0x2a1c7)
>> 16 0x7f628a in __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x2a28a)
>> 17 0x42ca24 in _start (vswitchd/ovs-vswitchd+0x42ca24)
>>
>> SUMMARY: UndefinedBehaviorSanitizer:
>> undefined-behavior lib/netdev-native-tnl.c:985:16
>>
>> Having misaligned pointers is also generally not allowed in C, let
>> alone accessing memory through them.
>>
>> Fix that by using an appropriate ovs_16aligned_in6_addr pointer instead.
>>
>> Fixes: 7381fd440a88 ("odp: Add SRv6 tunnel actions.")
>> Fixes: 03fc1ad78521 ("userspace: Add SRv6 tunnel support.")
>> Signed-off-by: Ilya Maximets
>> ---
>> lib/netdev-native-tnl.c | 5 ++---
>> lib/odp-util.c | 4 ++--
>> 2 files changed, 4 insertions(+), 5 deletions(-)
>
> Hi Ilya,
>
> Thanks for the fix!
>
> Reviewed-by: Nobuhiro MIKI
Thanks! Applied and backported down to 3.2.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2/2] atlocal: Replace deprecated pkg_resources.
On 5/21/24 08:32, Eelco Chaudron wrote: > > > On 17 May 2024, at 20:47, Ilya Maximets wrote: > >> 'pkg_resources' module is deprecated and no longer available in newer >> versions of python, so pytest tests are skipped: >> >> DeprecationWarning: pkg_resources is deprecated as an API. >> See https://setuptools.pypa.io/en/latest/pkg_resources.html >> >> Unfortunately, there is no direct replacement for it and functionality >> is scattered between different packages. >> >> Using a new standard library importlib.metadata to find installed >> packages and their versions. Using packaging.requirements to parse >> lines from the requirements file and compare versions. This covers >> all we need. >> >> The 'packaging' is a project used by pip and a dependency for many >> other libraries, so should be available for any supported verison of >> python. 'importlib' was introduced in python 3.8. Since we support >> older versions of python and 'packaging' is not part of the standard >> library, checking that import is possible and falling back to >> 'pkg_resources' if needed. We may remove the fallback when we stop >> supporting python below 3.8. >> >> Even though 'packaging' is a common dependency, added to the test >> requirements so it will not be missed in CI. >> >> Signed-off-by: Ilya Maximets > > Thanks for fixing this. The changes look good to me. > > Acked-by: Eelco Chaudron > Thanks! Applied and backported down to 3.0. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] OVS 3.3.1 release date
On 5/22/24 16:17, Vladislav Odintsov wrote: > Hi all, > > I’m wondering whether there is a planned date for OVS 3.3.1 release? > > Currently there are a lot of useful bugfixes in branch-3.3 above 3.3.1 > tag and latest release was on the 17th of February (>3 months ago). Hi. I plan to make a series of releases in the next couple of weeks, ideally by the end of May, but maybe the first week of June. The current plan is try to incorporate at least partially David's fixes: https://patchwork.ozlabs.org/project/openvswitch/list/?series=403694 And get DPDK update to the recently released v23.11.1. But I think we'll need to make stable releases even if we will not be able to incorporate changes above in time. Best regards, Ilya Maximets. ___ 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.
Adding the list back to CC. On 5/22/24 13:28, amore...@redhat.com wrote: > On Wed, May 22, 2024 at 12:11:37AM GMT, Ilya Maximets wrote: >> On 5/16/24 19:03, Adrian Moreno wrote: >>> >>> >>> On 4/24/24 9:53 PM, 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 all, >>> >>> I had an offline meeting with Eelco, Ilya and Aaron about this topic and >>> wanted >>> to put out what was discussed and hopefully get more feedback. >>> >>> In general, 3 options were considered: >>> >>> Option A: Reusing the sample action >>> === >>> This is essentially what this proposal (and it's kernel counterpart) >>> consists >>> in. The datapath action would look like this
Re: [ovs-dev] [PATCH v4 00/12] Add flow visualization utility.
Please, keep the list in CC. :) On 5/22/24 12:52, amore...@redhat.com wrote: > On Wed, May 22, 2024 at 12:17:37AM GMT, Ilya Maximets wrote: >> On 5/21/24 23:56, Ilya Maximets wrote: >>> On 5/21/24 20:56, amore...@redhat.com wrote: >>>> On Tue, May 21, 2024 at 08:03:49PM GMT, Ilya Maximets wrote: >>>>> On 5/21/24 18:13, amore...@redhat.com wrote: >>>>>> On Tue, May 21, 2024 at 03:49:03PM GMT, amore...@redhat.com wrote: >>>>>>> On Tue, May 21, 2024 at 01:51:33PM GMT, Ilya Maximets wrote: >>>>>>>> On 5/7/24 16:30, Adrian Moreno wrote: >>>>>>>>> The goal of this utility is to read both datapath and Openflow flows >>>>>>>>> (using the flow library already available) and print them in different >>>>>>>>> formats and styles to make it easier to understand them and >>>>>>>>> troubleshoot >>>>>>>>> issues. >>>>>>>>> >>>>>>>>> The formats are quite opinionated and so are the colors chosen so I'm >>>>>>>>> eager to hear what is the impression caused to the community. >>>>>>>>> >>>>>>>>> Here is a summary of the formats and features supported: >>>>>>>>> >>>>>>>>> - Openflow >>>>>>>>>- Console: Prints flows back to the console allowing filtering and >>>>>>>>> extensive formatting. >>>>>>>>>- Cookie: Arranges flows based on cookie (instead of table) to ease >>>>>>>>> visualization of OVN-generated flows. >>>>>>>>>- HTML: Prints an HTML file that shows the flows arranged by >>>>>>>>> tables. >>>>>>>>> resubmit actions have a hyperlinke to the target table to ease >>>>>>>>> navegation. >>>>>>>>>- Logic: Many times OVN generates many "logically-equivalent" >>>>>>>>> flows. >>>>>>>>> These are flows that have the same structure: match on the same >>>>>>>>> values and have the same actions. The only thing that changes is >>>>>>>>> the actual value they match against and maybe the arguments of >>>>>>>>> the >>>>>>>>> actions. This format collapses these flows so you can visualize >>>>>>>>> the >>>>>>>>> logical pipeline easier. >>>>>>>>>- JSON: JSON format. >>>>>>>>> >>>>>>>>> More OpenFlow features: >>>>>>>>>- OVN metadata: Both the "cookie" and the "logic" format allow to >>>>>>>>> connect to a running OVN NB/SB databases and enrich the flows >>>>>>>>> with >>>>>>>>> OVN context based on the flow's cookie. >>>>>>>>> >>>>>>>>> - Datapath: >>>>>>>>>- Console: Prints flows back to the console allowing filtering and >>>>>>>>> extensive formatting. >>>>>>>>>- Tree: Datapath flows use recirculation to further execute >>>>>>>>> additional actions. This format builds a tree of flows following >>>>>>>>> the recirculation identifiers and prints it in the console. >>>>>>>>>- HTML: Prints the datapath flow tree in HTML. It includes some >>>>>>>>> minimal JS to support expanding and collapsing of entire >>>>>>>>> branches. >>>>>>>>>- Graph: Following the "tree" format, this one prints the tree in >>>>>>>>> graphviz format. >>>>>>>>>- JSON: JSON format. >>>>>>>>> >>>>>>>>> Additional datapath features: >>>>>>>>>- Many datapath formats are based on the tree flow hierarchy. An >>>>>>>>> interesting feature of this structure is that filtering is done >>>>>>>>> at >>>>>>>>> the branch level. This means that when a flow satisfies the >>>>>>>>> filter, &g
Re: [ovs-dev] [PATCH] netdev: Padding runt packet on VXLAN and DPDK ports.
On 5/21/24 13:29, Kumar, Rohit wrote:
> Hi Ilya,
>
> Thanks for your comments.
>
> Yes, it's the hardware switch that doesn't do zero padding. Unfortunately,
> the switch vendor claims that the padding is added after the FCS of the
> original packet, and there is no way around it. And if it's a requirement
> to zero pad, then RFC 1042 should use the word "must". As for the "valid
> reason in special circumstances", the reason is that generating zero padding
> would have greatly complicated their switch architecture, resulting in
> additional power consumption and latency. So OVS has to do it.
This is not a good argument for OVS to do that. We can't workaround every
possible thing hardware vendors implement differently. Adding extra padding
into tunnel packets beside performance impact will break communication with
older Linux kernels, so we can't do that. Also, as I already said, this will
introduce the difference between Userpsace and Kernel datapaths, i.e your
setup will still not work if you use Linux kernel tunnels for encapsulation.
Same is likely true for other operating systems.
>
> For the other part, where zero padding is to be added to the DPDK interface,
> no PMD except a few (Atomic Rules, Broadcom, HiSilicon PMDs) do this in the
> driver, and we use Intel, so that's the reason for doing it in OVS.
Most modern NICs support padding on the hardware level. Intel supports it
by enabling IXGBE_HLREG0_TXPADEN for ixgbe cards for example. So, even if
the padding is not performed in software it doesn't mean it's not performed.
Doing this in software will be a waste of CPU resources in most cases.
Best regards, Ilya Maximets.
>
> Regards,
> Rohit Kumar
> ----
> *From:* Ilya Maximets
> *Sent:* Monday, May 20, 2024 11:30 PM
> *To:* Kumar, Rohit ; d...@openvswitch.org
>
> *Cc:* i.maxim...@ovn.org
> *Subject:* Re: [ovs-dev] [PATCH] netdev: Padding runt packet on VXLAN and
> DPDK ports.
>
> ***WARNING! THIS EMAIL ORIGINATES FROM OUTSIDE ST ENGINEERING IDIRECT.***
>
> On 5/18/24 00:55, Kumar, Rohit via dev wrote:
>> The basic idea behind this patch is to pad the runt packet before vxlan
>> encapsulation and on a DPDK port.
>>
>> topology:
>> IXIA <> SWITCH <--vxlan--> OVS <--dpdk--> HOST
>>
>> The host sends a runt packet (54 bytes) with a size less than 64 bytes
>> to the OVS. The OVS receives this runt packet and sends it further down
>> the VXLAN tunnel (original packet 54 + 50 VXLAN = 104 B total packet size)
>> to a physical switch. At the switch, after decapsulation, the original
>> packet size is 54B and it then sends the packet on to the ixia. However,
>> the switch adds 2B of padding with random content (not zero as RFC 1042
>> mentions) and due to this random byte padding, ixia drops the packet.
>>
>> Switch vendors claim to be compliant with both RFC 1042 and RFC 2119 to
>> fix it in the switch.
>>
>> RFC 1042 defines:
>> “IEEE 802 packets may have a minimum size restriction. When
>> necessary, the data field should be padded (with octets of zero)
>> to meet the IEEE 802 minimum frame size requirements. This
>> padding is not part of the IP datagram and is not included in the
>> total length field of the IP header.”
>>
>> RFC 2119 defines:
>> "SHOULD This word, or the adjective "RECOMMENDED", mean that there
>> may exist valid reasons in particular circumstances to ignore a
>> particular item, but the full implications must be understood and
>> carefully weighed before choosing a different course."
>>
>> So, a fix is needed in the OVS. The OVS is connected to the switch on a
>> VXLAN port and to the host on a DPDK port. The padding fix is applied to
>> both netdev types. I tested the fix in the same setup and below are the
>> captures after padding on both VXLAN and DPDK ports from OVS UT.
>>
>> VXLAN (46B zero pad):
>> aa 55 aa 66 00 00 aa 55 aa 55 00 00 08 00 45 00 .U.f...U.UE.
>> 0010 00 60 00 00 40 00 40 11 26 81 0a 00 00 02 0a 00 .`..@.@.&...
>> 0020 00 0b 9b 77 12 b5 00 4c 00 00 08 00 00 00 00 00 ...w...L
>> 0030 7b 00 50 54 00 00 00 0a 50 54 00 00 00 09 12 34 {.PTPT.4
>> 0040 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> 0050 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> 0060 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ..
>>
>> DPDK (16B zero pad):
>> ff ff ff ff ff ff 5e fb 90 96 7d b7 08 06 00 01 ..^...}.
>&
Re: [ovs-dev] [PATCH v4 00/12] Add flow visualization utility.
On 5/21/24 23:56, Ilya Maximets wrote: > On 5/21/24 20:56, amore...@redhat.com wrote: >> On Tue, May 21, 2024 at 08:03:49PM GMT, Ilya Maximets wrote: >>> On 5/21/24 18:13, amore...@redhat.com wrote: >>>> On Tue, May 21, 2024 at 03:49:03PM GMT, amore...@redhat.com wrote: >>>>> On Tue, May 21, 2024 at 01:51:33PM GMT, Ilya Maximets wrote: >>>>>> On 5/7/24 16:30, Adrian Moreno wrote: >>>>>>> The goal of this utility is to read both datapath and Openflow flows >>>>>>> (using the flow library already available) and print them in different >>>>>>> formats and styles to make it easier to understand them and troubleshoot >>>>>>> issues. >>>>>>> >>>>>>> The formats are quite opinionated and so are the colors chosen so I'm >>>>>>> eager to hear what is the impression caused to the community. >>>>>>> >>>>>>> Here is a summary of the formats and features supported: >>>>>>> >>>>>>> - Openflow >>>>>>>- Console: Prints flows back to the console allowing filtering and >>>>>>> extensive formatting. >>>>>>>- Cookie: Arranges flows based on cookie (instead of table) to ease >>>>>>> visualization of OVN-generated flows. >>>>>>>- HTML: Prints an HTML file that shows the flows arranged by tables. >>>>>>> resubmit actions have a hyperlinke to the target table to ease >>>>>>> navegation. >>>>>>>- Logic: Many times OVN generates many "logically-equivalent" flows. >>>>>>> These are flows that have the same structure: match on the same >>>>>>> values and have the same actions. The only thing that changes is >>>>>>> the actual value they match against and maybe the arguments of the >>>>>>> actions. This format collapses these flows so you can visualize the >>>>>>> logical pipeline easier. >>>>>>>- JSON: JSON format. >>>>>>> >>>>>>> More OpenFlow features: >>>>>>>- OVN metadata: Both the "cookie" and the "logic" format allow to >>>>>>> connect to a running OVN NB/SB databases and enrich the flows with >>>>>>> OVN context based on the flow's cookie. >>>>>>> >>>>>>> - Datapath: >>>>>>>- Console: Prints flows back to the console allowing filtering and >>>>>>> extensive formatting. >>>>>>>- Tree: Datapath flows use recirculation to further execute >>>>>>> additional actions. This format builds a tree of flows following >>>>>>> the recirculation identifiers and prints it in the console. >>>>>>>- HTML: Prints the datapath flow tree in HTML. It includes some >>>>>>> minimal JS to support expanding and collapsing of entire branches. >>>>>>>- Graph: Following the "tree" format, this one prints the tree in >>>>>>> graphviz format. >>>>>>>- JSON: JSON format. >>>>>>> >>>>>>> Additional datapath features: >>>>>>>- Many datapath formats are based on the tree flow hierarchy. An >>>>>>> interesting feature of this structure is that filtering is done at >>>>>>> the branch level. This means that when a flow satisfies the filter, >>>>>>> the entire sub-tree leading to that flow is shown. >>>>>>> >>>>>>> Additional common features: >>>>>>>- Styling: Styles for both console and HTML formats can be defined >>>>>>> using a configuration file. >>>>>>>- Heat maps: Some formats support heat-maps. A color pallete ranging >>>>>>> from blue (cold) to red (hot) is used to print the number of >>>>>>> packets and bytes of the flows. That way, the flows that are >>>>>>> handling more traffic are much easier to spot >>>>>>> >>>>>>> -- >>>>>>> V3 -> V4: >>>>>>> - Add manpage to rpm package >>>>>>> - Fix Eelco's comments >
Re: [ovs-dev] [RFC 00/11] Add psample support to NXAST_SAMPLE action.
were are no listeners in the > multicast group. This nulls-out this potential optimization. > > - Although not super-expensive (if skb is not modified), it _always_ requires > an > skb_clone (even without "trunc"). > > - It requires a very big amount of work in ovs-vswitchd: >- We'd need to decide who creates the port, if it's the user via Openflow > and > it's exposed as a port in a bridge or if it's a hidden vport created by the > dpif > layer. >- Controls need to be established to limit OFP actions to send traffic to > this port or it receiving traffic. >- DPDK datapath would probably require a new netdev_class as well. > > - tc offload is more complicated. If we want to use act_sample (I cannot > think > of a way that doesn't involve act_sample), we'd need to track "sample" > actions > and "set_observability_metadata) to be able to build an act_sample for this. > > Ilya, Aaron, Eelco, did I miss something? > > Thoughts? Preferences? I think, option C is a little too heavy for the end functionality for the user. And I'm not sure if we'll be able to reuse this infrastructure for anything else. I'm not a fan of the design breaking part of the A and duplication of attributes. So, I'd prefer the B here, but I'm a little biased. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4 00/12] Add flow visualization utility.
On 5/21/24 20:56, amore...@redhat.com wrote: > On Tue, May 21, 2024 at 08:03:49PM GMT, Ilya Maximets wrote: >> On 5/21/24 18:13, amore...@redhat.com wrote: >>> On Tue, May 21, 2024 at 03:49:03PM GMT, amore...@redhat.com wrote: >>>> On Tue, May 21, 2024 at 01:51:33PM GMT, Ilya Maximets wrote: >>>>> On 5/7/24 16:30, Adrian Moreno wrote: >>>>>> The goal of this utility is to read both datapath and Openflow flows >>>>>> (using the flow library already available) and print them in different >>>>>> formats and styles to make it easier to understand them and troubleshoot >>>>>> issues. >>>>>> >>>>>> The formats are quite opinionated and so are the colors chosen so I'm >>>>>> eager to hear what is the impression caused to the community. >>>>>> >>>>>> Here is a summary of the formats and features supported: >>>>>> >>>>>> - Openflow >>>>>>- Console: Prints flows back to the console allowing filtering and >>>>>> extensive formatting. >>>>>>- Cookie: Arranges flows based on cookie (instead of table) to ease >>>>>> visualization of OVN-generated flows. >>>>>>- HTML: Prints an HTML file that shows the flows arranged by tables. >>>>>> resubmit actions have a hyperlinke to the target table to ease >>>>>> navegation. >>>>>>- Logic: Many times OVN generates many "logically-equivalent" flows. >>>>>> These are flows that have the same structure: match on the same >>>>>> values and have the same actions. The only thing that changes is >>>>>> the actual value they match against and maybe the arguments of the >>>>>> actions. This format collapses these flows so you can visualize the >>>>>> logical pipeline easier. >>>>>>- JSON: JSON format. >>>>>> >>>>>> More OpenFlow features: >>>>>>- OVN metadata: Both the "cookie" and the "logic" format allow to >>>>>> connect to a running OVN NB/SB databases and enrich the flows with >>>>>> OVN context based on the flow's cookie. >>>>>> >>>>>> - Datapath: >>>>>>- Console: Prints flows back to the console allowing filtering and >>>>>> extensive formatting. >>>>>>- Tree: Datapath flows use recirculation to further execute >>>>>> additional actions. This format builds a tree of flows following >>>>>> the recirculation identifiers and prints it in the console. >>>>>>- HTML: Prints the datapath flow tree in HTML. It includes some >>>>>> minimal JS to support expanding and collapsing of entire branches. >>>>>>- Graph: Following the "tree" format, this one prints the tree in >>>>>> graphviz format. >>>>>>- JSON: JSON format. >>>>>> >>>>>> Additional datapath features: >>>>>>- Many datapath formats are based on the tree flow hierarchy. An >>>>>> interesting feature of this structure is that filtering is done at >>>>>> the branch level. This means that when a flow satisfies the filter, >>>>>> the entire sub-tree leading to that flow is shown. >>>>>> >>>>>> Additional common features: >>>>>>- Styling: Styles for both console and HTML formats can be defined >>>>>> using a configuration file. >>>>>>- Heat maps: Some formats support heat-maps. A color pallete ranging >>>>>> from blue (cold) to red (hot) is used to print the number of >>>>>> packets and bytes of the flows. That way, the flows that are >>>>>> handling more traffic are much easier to spot >>>>>> >>>>>> -- >>>>>> V3 -> V4: >>>>>> - Add manpage to rpm package >>>>>> - Fix Eelco's comments >>>>>> V2 -> V3: >>>>>> - Fix grammar thanks to Eelco's review >>>>>> V1 -> V2: >>>>>> - Fix typos and nits on documentation >>>>>> - Split documentation in two: ovs-flowviz.8 man page and a topic >>>>>>
Re: [ovs-dev] [PATCH v4 00/12] Add flow visualization utility.
On 5/21/24 18:13, amore...@redhat.com wrote: > On Tue, May 21, 2024 at 03:49:03PM GMT, amore...@redhat.com wrote: >> On Tue, May 21, 2024 at 01:51:33PM GMT, Ilya Maximets wrote: >>> On 5/7/24 16:30, Adrian Moreno wrote: >>>> The goal of this utility is to read both datapath and Openflow flows >>>> (using the flow library already available) and print them in different >>>> formats and styles to make it easier to understand them and troubleshoot >>>> issues. >>>> >>>> The formats are quite opinionated and so are the colors chosen so I'm >>>> eager to hear what is the impression caused to the community. >>>> >>>> Here is a summary of the formats and features supported: >>>> >>>> - Openflow >>>>- Console: Prints flows back to the console allowing filtering and >>>> extensive formatting. >>>>- Cookie: Arranges flows based on cookie (instead of table) to ease >>>> visualization of OVN-generated flows. >>>>- HTML: Prints an HTML file that shows the flows arranged by tables. >>>> resubmit actions have a hyperlinke to the target table to ease >>>> navegation. >>>>- Logic: Many times OVN generates many "logically-equivalent" flows. >>>> These are flows that have the same structure: match on the same >>>> values and have the same actions. The only thing that changes is >>>> the actual value they match against and maybe the arguments of the >>>> actions. This format collapses these flows so you can visualize the >>>> logical pipeline easier. >>>>- JSON: JSON format. >>>> >>>> More OpenFlow features: >>>>- OVN metadata: Both the "cookie" and the "logic" format allow to >>>> connect to a running OVN NB/SB databases and enrich the flows with >>>> OVN context based on the flow's cookie. >>>> >>>> - Datapath: >>>>- Console: Prints flows back to the console allowing filtering and >>>> extensive formatting. >>>>- Tree: Datapath flows use recirculation to further execute >>>> additional actions. This format builds a tree of flows following >>>> the recirculation identifiers and prints it in the console. >>>>- HTML: Prints the datapath flow tree in HTML. It includes some >>>> minimal JS to support expanding and collapsing of entire branches. >>>>- Graph: Following the "tree" format, this one prints the tree in >>>> graphviz format. >>>>- JSON: JSON format. >>>> >>>> Additional datapath features: >>>>- Many datapath formats are based on the tree flow hierarchy. An >>>> interesting feature of this structure is that filtering is done at >>>> the branch level. This means that when a flow satisfies the filter, >>>> the entire sub-tree leading to that flow is shown. >>>> >>>> Additional common features: >>>>- Styling: Styles for both console and HTML formats can be defined >>>> using a configuration file. >>>>- Heat maps: Some formats support heat-maps. A color pallete ranging >>>> from blue (cold) to red (hot) is used to print the number of >>>> packets and bytes of the flows. That way, the flows that are >>>> handling more traffic are much easier to spot >>>> >>>> -- >>>> V3 -> V4: >>>> - Add manpage to rpm package >>>> - Fix Eelco's comments >>>> V2 -> V3: >>>> - Fix grammar thanks to Eelco's review >>>> V1 -> V2: >>>> - Fix typos and nits on documentation >>>> - Split documentation in two: ovs-flowviz.8 man page and a topic >>>>article with more detailed examples. >>>> RFC -> V1: >>>> - Addressed Eelco's comments >>>> - Added a documentation page >>>> - Added support for dark style HTML pages >>>> - Patch 3. Small fix in the way a style is looked up. Use the key in >>>>the KV instead of the metadata string. This helps with "free" values >>>>such as "output". >>>> >>>> Adrian Moreno (12): >>>> python: ovs: Add flowviz scheleton. >>>> python: ovs: flowviz: Add file processing infra. >>>> python: ovs: flowviz: A
Re: [ovs-dev] [PATCH v4 00/12] Add flow visualization utility.
, line 158, in create_flow return ODPFlow(line, idx) ^^ File "/python/ovs/flow/odp.py", line 153, in __init__ aparser.parse() File "/python/ovs/flow/kv.py", line 296, in parse raise ParseError( ovs.flow.kv.ParseError: Error parsing key-value (check_pkt_len, size=8514,gt(sample(sample=100.0%,actions(meter(4),userspace(pid=4294967295,controller(reason=1,dont_send=0,continuation=0,recirc_id=29551991,rule_cookie=0x769b5340,controller_id=0,max_len=65535),le(set(eth(src=0a:0b:0c:0d:08:08,dst=aa:bb:cc:dd:ee:ff)),set(ipv4(ttl=62)),check_pkt_len(size=8514,gt(sample(sample=100.0%,actions(meter(4),userspace(pid=4294967295,controller(reason=1,dont_send=0,continuation=0,recirc_id=33861757,rule_cookie=0x9ac6aa33,controller_id=0,max_len=65535),le(ct(zone=12,nat),recirc(0x204b07e) Not the exact same line, but a similar crash happens while parsing: recirc_id(0x204ae73),in_port(20),ct_state(+new-est-rel-rpl-inv+trk),ct_mark(0/0x000f),eth(src=1a:1b:1c:1d:18:18,dst=2a:2b:2c:2d:2e:2f),eth_type(0x0800),ipv4(src=192.168.10.1/255.255.255.254,dst=10.64.0.0/255.192.0.0,proto=6,ttl=64,frag=no), packets:7, bytes:518, used:9.077s, flags:S, actions:ct(commit,zone=269,mark=0/0x1,nat(src)),set(eth(src=3a:3b:3c:3d:3e:3f,dst=4a:4b:4c:4d:4e:4f)),set(ipv4(ttl=63)),check_pkt_len(size=8514,gt(sample(sample=100.0%,actions(meter(4),userspace(pid=4294967295,controller(reason=1,dont_send=0,continuation=0,recirc_id=29551991,rule_cookie=0x769b5340,controller_id=0,max_len=65535),le(set(eth(src=0a:0b:0c:0d:08:08,dst=aa:bb:cc:dd:ee:ff)),set(ipv4(ttl=62)),check_pkt_len(size=8514,gt(sample(sample=100.0%,actions(meter(4),userspace(pid=4294967295,controller(reason=1,dont_send=0,continuation=0,recirc_id=33861236,rule_cookie=0x9ac6aa33,controller_id=0,max_len=65535),le(ct(zone=12,nat),recirc(0x204ae75) My guess is that it has some trouble with nested check_pkt_len. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] GitHub: Add Coverity scan as a daily GitHub action.
On 5/1/24 14:33, Eelco Chaudron wrote: > > > On 29 Apr 2024, at 14:59, Ilya Maximets wrote: > >> On 4/16/24 09:44, Eelco Chaudron wrote: >>> This patch adds a daily Coverity run for the OVS main branch >>> to the GitHub actions. The result of the runs can be found here: >>> >>> https://scan.coverity.com/projects/openvswitch >>> >>> Before applying, we need to add the following two actions secrets >>> to the GitHub openvswitch project: >>> >>> - COVERITY_SCAN_TOKEN; The secret token from the project page >>> - COVERITY_SCAN_EMAIL; The maintainer's email alias >>> >>> Signed-off-by: Eelco Chaudron >>> --- >>> .github/workflows/coverity.yml | 131 + >>> Makefile.am| 1 + >>> README.rst | 2 + >>> 3 files changed, 134 insertions(+) >>> create mode 100644 .github/workflows/coverity.yml >> >> Thanks, Eelco for bringing this up. Though, continuing the >> offline discussion, I'm not sure having coverity scans as >> a GitHub action has any benefits for OVS community. >> >> A few issues: >> >> 1. Reports are not available to people outside of the OVS >>Coverity project. Adding everyone there doesn't seem >>reasonable. Also a potential security concern. > > I do not see a security concern, as anyone can set up the coverity scan > on his fork of the project and has all the insights. I do agree that we > should not try to add everybody to the Coverity project, and this might > be more for maintainers (who can opt-in to get emails on new issues). Fair, not a security concern indeed. But it's still not nice that reports are not publicly available. > >> 2. Workflow cannot be used in forks due to organization secrets. >>(IIUC, current implementation will just fail once a day >>annoying people who didn't disable that workflow.) > > I guess this can be fixed, we could check for the variables. This way if > someone wants to add Coverity scans to his fork, all they need to do is > add the secrets. > >> 3. Necessity to have secrets in the organization. We currently >>don't have any, and from a security standpoint not having >>any secrets is always better. > > Is having the secrets in a 3rd party located robot more secure? It's not more secure per se, but it makes security concerns someone else's problem. :) GitHub been leaking secrets multiple times in the past few years. And robot likely has lower attack surface. At least we don't need to care about executing binary blobs downloaded from the internet in the context of the main OVS organization - robot can be better isolated. Also, using non-official actions from some random person is a bit sketchy. > >> 4. A decent amount of code duplication for this workflow. >>(Reusable workflows maybe?) > > Yes, I can take a look at composition in GitHub actions, as there is > no real ‘include’ like feature. There are 'reusable workflows', but they are a not very intuitive. > >> In its current form, I think, the same functionality can be >> provided by the 0-day robot that could submit sources once >> a day for the scan. And it could also send some emails with >> build details if necessary (again, there may be some security >> concerns in case coverity discovers a genuine security issue). > > Guess results are only available in the GUI and through an email, > don’t think you can query new items through an API. > > @Aaron do you know how this is handled for DPDK? > >> With the total amount of code in the project, IIUC, we could >> submit up to 21 builds a week with no more than 3 per day >> (Coverity claims that it checked just under 300K lines in OVS). >> So, technically, we could create a bit more complex logic >> for 0-day bot to run a check per patch set (probably, not per >> patch though). In most cases we would fit within the limit >> with the current patch rate. Maybe reserving one run a week >> for the main branch. Such logic would be harder to implement >> with GHA. >> >> All in all, using GHA just for scheduling of a job that wider >> community can't take advantage of seems a little unjustified. >> >> Thoughts? > > I think it would benefit the maintainers to start with, and if implemented > right people who fork should not see any side effects. There will be no difference in this aspect if implemented as part of the 0day robot. > In addition, it also > enables them to integrate Coverity for their own fork if they want to. > > Others? > > //Eelco > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] netdev: Padding runt packet on VXLAN and DPDK ports.
On 5/18/24 00:55, Kumar, Rohit via dev wrote:
> The basic idea behind this patch is to pad the runt packet before vxlan
> encapsulation and on a DPDK port.
>
> topology:
> IXIA <> SWITCH <--vxlan--> OVS <--dpdk--> HOST
>
> The host sends a runt packet (54 bytes) with a size less than 64 bytes
> to the OVS. The OVS receives this runt packet and sends it further down
> the VXLAN tunnel (original packet 54 + 50 VXLAN = 104 B total packet size)
> to a physical switch. At the switch, after decapsulation, the original
> packet size is 54B and it then sends the packet on to the ixia. However,
> the switch adds 2B of padding with random content (not zero as RFC 1042
> mentions) and due to this random byte padding, ixia drops the packet.
>
> Switch vendors claim to be compliant with both RFC 1042 and RFC 2119 to
> fix it in the switch.
>
> RFC 1042 defines:
> “IEEE 802 packets may have a minimum size restriction. When
> necessary, the data field should be padded (with octets of zero)
> to meet the IEEE 802 minimum frame size requirements. This
> padding is not part of the IP datagram and is not included in the
> total length field of the IP header.”
>
> RFC 2119 defines:
> "SHOULD This word, or the adjective "RECOMMENDED", mean that there
>may exist valid reasons in particular circumstances to ignore a
>particular item, but the full implications must be understood and
>carefully weighed before choosing a different course."
>
> So, a fix is needed in the OVS. The OVS is connected to the switch on a
> VXLAN port and to the host on a DPDK port. The padding fix is applied to
> both netdev types. I tested the fix in the same setup and below are the
> captures after padding on both VXLAN and DPDK ports from OVS UT.
>
> VXLAN (46B zero pad):
> aa 55 aa 66 00 00 aa 55 aa 55 00 00 08 00 45 00 .U.f...U.UE.
> 0010 00 60 00 00 40 00 40 11 26 81 0a 00 00 02 0a 00 .`..@.@.&...
> 0020 00 0b 9b 77 12 b5 00 4c 00 00 08 00 00 00 00 00 ...w...L
> 0030 7b 00 50 54 00 00 00 0a 50 54 00 00 00 09 12 34 {.PTPT.4
> 0040 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0050 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0060 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ..
>
> DPDK (16B zero pad):
> ff ff ff ff ff ff 5e fb 90 96 7d b7 08 06 00 01 ..^...}.
> 0010 08 00 06 04 00 01 5e fb 90 96 7d b7 0a 01 01 01 ..^...}.
> 0020 00 00 00 00 00 00 0a 01 01 02 00 00 00 00 00 00
> 0030 00 00 00 00 00 00 00 00 00 00 00 00
>
> Signed-off-by: Rohit Kumar
Hi, Rohit. Thanks for the patch!
Though I don't think OVS should add padding to packets. RFC 1042 is talking
about transmission to ethernet networks. In your example above OVS sends 104
byte long packet to the ethernet link, so no padding is needed. If the
'SWITCH' in your setup sends incorectly padded packet after decapsulation,
that sounds like an issue of that switch. Is this patch a workaround for
a hardware switch issue?
Also, we even had a request in the past to strip all the existing padding before
encapsulation, because older Linux kernel versions had issues processing padded
inner packets after decapsulation.
Adding the padding will also introduce a difference in behavior between Linux
kenrel datapath and the userspace datapath, since Linux kernel doesn't pad
packet
before encapsulation. Having a difference between datapath implementations is
not good.
For the part where we add padding before sending to a DPDK interface, this
also doesn't seem right. Normally it's a job of a driver to properly pad
packets before transmitting them. OVS doesn't know what the requirements
for the particular network it is connected to or even if it is an ethernet
or some other type of the interface. So, it can't make a decision on padding,
only driver can do that.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] Fixes: 1b3557f53dbc ("vswitchd, ofproto-dpif: Propagate the CT limit from database.")
On 5/18/24 17:36, Chandler Wu wrote:
> From 747d3c040d29e5abf2ddc45bd8fd4e904cfb53b3 Mon Sep 17 00:00:00 2001
> From: chandlerwu mailto:chandler0...@gmail.com>>
> Date: Mon, 6 May 2024 11:58:21 +0800
> Fixes: 1b3557f53dbc ("vswitchd, ofproto-dpif: Propagate the
> CT limit from database.")
For some reason this tag became a Subject line, which is a little strange.
The Fixes tag should be placed at the end of the commit message right above
the Signed-off-by tag.
The subject line should look something like this:
Subject: [PATCH v2] bridge: Fix ct timeout policy when creating a new zone.
Consider using 'git send-email' command for sending patches:
https://www.git-scm.com/docs/git-send-email/
It will handle proper formatting for you as long as you have a well-formatted
commit message.
The following documentation page also contains a few notes about patch
formatting and different tags:
https://docs.openvswitch.org/en/latest/internals/contributing/submitting-patches/
>
> If we 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 find the new copied tp== tp_cfg, so ``ofproto_ct_set_zone_timeout_policy`
> will not be called.
As I mentioned in reply to v1, I'd still prefer if we just removed the
get_timeout_policy_from_ovsrec() call from ct_zone_alloc().
Best regards, Ilya Maximets.
>
> Signed-off-by: chandlerwu
> ---
> vswitchd/bridge.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index 95a65fcdc..2c8362a35 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -766,6 +766,9 @@ 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(>ct_zones, _zone->node, hash_int(zone_id, 0));
> + ofproto_ct_set_zone_timeout_policy(dp->type, ct_zone->zone_id,
> + _zone->tp);
> + goto post_update;
> }
>
> struct simap new_tp = SIMAP_INITIALIZER(_tp);
> @@ -780,6 +783,7 @@ ct_zones_reconfigure(struct datapath *dp, struct
> ovsrec_datapath *dp_cfg)
> }
> }
>
> + post_update:;
> int64_t desired_limit = zone_cfg->limit ? *zone_cfg->limit : -1;
> if (ct_zone->limit != desired_limit) {
> ofproto_ct_zone_limit_update(dp->type, zone_id, zone_cfg->limit);
> --
> 2.39.2
>
___
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.
On 5/18/24 17:51, Chandler Wu wrote: > Hi Ilya, > > I would prefer to keep ct_zone_alloc staying as it is now semantically, > for its existence dates back to like 5 years ago This would be a good argument if this function was part of public API, but it is just a simple static function with just one user, so there is no point in trying to preserve the behavior. Also, I think, the fact that the faction not only allocates, but also fills in the structure is a primary cause of the issue you're fixing. The function is called 'alloc', but does something unexpected (populating the hash map) as well. > To void unnecessary checks, I add a jump. I'd still prefer just removing the line form the function instead. There is no need to add more complexity where it can be reduced. > > Btw, this is my first commit to the opensource community, and for a > 'v2 commit' I don't know if it's ok to reply to this thread directly. > > So I just sent it to `d...@openvswitch.org` in another mail (cc to you). That's the right way to post new versions of the patch. All good. :) For some reason the Fixes tag became a Subject of the patch, that will need to be adjusted though. I'll reply to v2 separately. And please keep the mailing list in CC while replying to emails (i.e. use Reply-All), so the thread is visible in the mailing list archives. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 5/5] netdev-linux: Fix Clang's static analyzer uninitialized values warnings.
On 5/16/24 21:37, Mike Pattrick wrote:
> Clang's static analyzer will complain about an uninitialized value
> because in some error conditions we wouldn't set all values that are
> used later.
Some more details would be nice here.
>
> Now we initialize more values that are needed later even in error
> conditions.
>
> Signed-off-by: Mike Pattrick
> ---
Same for the subject line and a Fixes tag.
Also, there seems to be two separate issues fixed here, they should
be separate patches with distinct names and Fixes tags as they may
need to go to different branches (I didn't check).
> lib/netdev-linux.c | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index 25349c605..66dae3e1a 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -2439,7 +2439,9 @@ netdev_linux_read_definitions(struct netdev_linux
> *netdev,
> int error = 0;
>
> error = netdev_linux_read_stringset_info(netdev, );
> -if (error || !len) {
> +if (!len) {
> +return -EOPNOTSUPP;
> +} else if (error) {
> return error;
> }
> strings = xzalloc(sizeof *strings + len * ETH_GSTRING_LEN);
> @@ -2724,6 +2726,7 @@ netdev_linux_get_speed_locked(struct netdev_linux
> *netdev,
>uint32_t *current, uint32_t *max)
> {
> if (netdev_linux_netnsid_is_remote(netdev)) {
> +*current = 0;
> return EOPNOTSUPP;
> }
>
> @@ -2733,6 +2736,8 @@ netdev_linux_get_speed_locked(struct netdev_linux
> *netdev,
> ? 0 : netdev->current_speed;
> *max = MIN(UINT32_MAX,
> netdev_features_to_bps(netdev->supported, 0) /
> 100ULL);
> +} else {
> +*current = 0;
> }
> return netdev->get_features_error;
> }
We should be clearing both the current and a max for consistency.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 4/5] socket: Fix Clang's static analyzer 'garbage value' warnings.
On 5/16/24 21:36, Mike Pattrick wrote:
> Clang's static analyzer will complain about an uninitialized value
> because we weren't setting a value for dns_failure in all code paths.
>
> Now we initialize this on declaration.
>
> Signed-off-by: Mike Pattrick
> ---
Same comments for the subject line and a Fixes tag.
> lib/socket-util.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/socket-util.c b/lib/socket-util.c
> index 2d89fce85..1d21ce01c 100644
> --- a/lib/socket-util.c
> +++ b/lib/socket-util.c
> @@ -711,7 +711,7 @@ inet_open_passive(int style, const char *target, int
> default_port,
> struct sockaddr_storage ss;
> int fd = 0, error;
> unsigned int yes = 1;
> -bool dns_failure;
> +bool dns_failure = false;
>
> if (!inet_parse_passive(target, default_port, , true, _failure)) {
> if (dns_failure) {
I'm not sure this is a right solution. inet_parse_passive() should set
the 'dns_failure' whenever it fails, i.e. it should set 'dns_failure'
in every condition where it sets ok = false.
inet_parse_passive() is also not a static function, so we should fix it
instead, so other potential users do not have this issue.
While at it, inet_parse_active() has the same problem. It should set
the 'dns_failure' whenever it fails.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 3/5] dpctl: Fix Clang's static analyzer 'garbage value' warnings.
On 5/16/24 21:36, Mike Pattrick wrote:
> Clang's static analyzer will complain about an uninitialized value
> because we weren't setting a value for ufid_generated in all code paths.
>
> Now we initialize this on declaration.
>
> Signed-off-by: Mike Pattrick
> ---
Same comments for the subject line.
Also needs a Fixes tag.
> lib/dpctl.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/lib/dpctl.c b/lib/dpctl.c
> index 3c555a559..9c287d060 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -1366,12 +1366,11 @@ dpctl_del_flow_dpif(struct dpif *dpif, const char
> *key_s,
> struct ofpbuf mask; /* To be ignored. */
>
> ovs_u128 ufid;
> -bool ufid_generated;
> -bool ufid_present;
> +bool ufid_generated = false;
> +bool ufid_present = false;
Maybe move these above the 'ufid' for the sake of a reverse x-mass.
And move the 'ufid' below the port names.
> struct simap port_names;
> int n, error;
>
> -ufid_present = false;
> n = odp_ufid_from_string(key_s, );
> if (n < 0) {
> dpctl_error(dpctl_p, -n, "parsing flow ufid");
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2/5] netdev-native-tnl: Fix Clang's static analyzer 'uninitialized value' warnings.
On 5/17/24 22:14, Ilya Maximets wrote:
> On 5/16/24 21:36, Mike Pattrick wrote:
>> Clang's static analyzer will complain about an uninitialized value
>> because we weren't properly checking the error code from a function that
>> would have initialized the value.
>
> Please, be more specific. :) At least, mention the variable name.
>
> And the same comment for the subject of the patch. This one is
> actually a real potential issue.
And it also needs a Fixes tag, since it's not a false-positive.
> So, something like this would
> be an appropriate name:
>
> netdev-native-tnl: Fix use of uninitialized offset on SRv6 header pop.
>
>>
>> Instead, add a check for that return code.
>>
>> Signed-off-by: Mike Pattrick
>> ---
>> lib/netdev-native-tnl.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
>> index dee9ab344..6e6b15764 100644
>> --- a/lib/netdev-native-tnl.c
>> +++ b/lib/netdev-native-tnl.c
>> @@ -1068,7 +1068,9 @@ netdev_srv6_pop_header(struct dp_packet *packet)
>> }
>>
>> pkt_metadata_init_tnl(md);
>> -netdev_tnl_ip_extract_tnl_md(packet, tnl, );
>> +if (netdev_tnl_ip_extract_tnl_md(packet, tnl, ) == NULL) {
>
> if (!...)
>
>> +goto err;
>> +}
>
> Maybe an empty line here.
>
>> dp_packet_reset_packet(packet, hlen);
>>
>> return packet;
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2/5] netdev-native-tnl: Fix Clang's static analyzer 'uninitialized value' warnings.
On 5/16/24 21:36, Mike Pattrick wrote:
> Clang's static analyzer will complain about an uninitialized value
> because we weren't properly checking the error code from a function that
> would have initialized the value.
Please, be more specific. :) At least, mention the variable name.
And the same comment for the subject of the patch. This one is
actually a real potential issue. So, something like this would
be an appropriate name:
netdev-native-tnl: Fix use of uninitialized offset on SRv6 header pop.
>
> Instead, add a check for that return code.
>
> Signed-off-by: Mike Pattrick
> ---
> lib/netdev-native-tnl.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
> index dee9ab344..6e6b15764 100644
> --- a/lib/netdev-native-tnl.c
> +++ b/lib/netdev-native-tnl.c
> @@ -1068,7 +1068,9 @@ netdev_srv6_pop_header(struct dp_packet *packet)
> }
>
> pkt_metadata_init_tnl(md);
> -netdev_tnl_ip_extract_tnl_md(packet, tnl, );
> +if (netdev_tnl_ip_extract_tnl_md(packet, tnl, ) == NULL) {
if (!...)
> +goto err;
> +}
Maybe an empty line here.
> dp_packet_reset_packet(packet, hlen);
>
> return packet;
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 1/5] netdev-offload: Fix Clang's static analyzer 'null pointer dereference' warnings.
On 5/16/24 21:36, Mike Pattrick wrote:
> Clang's static analyzer will complain about a null pointer dereference
> because dumps can be set to null and then there is a loop where it could
> have been written to.
>
> Instead, return early from the netdev_ports_flow_dump_create function if
> dumps is NULL.
>
> Signed-off-by: Mike Pattrick
> ---
> lib/netdev-offload.c | 9 -
> 1 file changed, 8 insertions(+), 1 deletion(-)
Thanks for the patch!
It is a false-positive as both loops are iterated while holding a lock.
Might be worth mentioning in the commit message.
Also, please, use more precise and concise names for the patches in the
set to reduce the chance of commits with the exact same names in the git
history. This may confuse some automation.
This one, for example, can be named:
netdev-offload: Fix null pointer dereference warning on dump creation.
There is no need to mention clang or static analysis in the subject,
this can be put in the commit message.
>
> diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c
> index 931d634e1..02b1cf203 100644
> --- a/lib/netdev-offload.c
> +++ b/lib/netdev-offload.c
> @@ -638,7 +638,14 @@ netdev_ports_flow_dump_create(const char *dpif_type, int
> *ports, bool terse)
> }
> }
>
> -dumps = count ? xzalloc(sizeof *dumps * count) : NULL;
> +if (count == 0) {
I'd prefer !count for zero checks.
> +*ports = 0;
> +ovs_rwlock_unlock(_to_netdev_rwlock);
> +
> +return NULL;
> +}
> +
> +dumps = xzalloc(sizeof *dumps * count);
I'd suggest we initialize the 'dumps' to NULL at the top and then
just if (!count) { goto unlock; } to avoid code duplication.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 2/2] atlocal: Replace deprecated pkg_resources.
'pkg_resources' module is deprecated and no longer available in newer
versions of python, so pytest tests are skipped:
DeprecationWarning: pkg_resources is deprecated as an API.
See https://setuptools.pypa.io/en/latest/pkg_resources.html
Unfortunately, there is no direct replacement for it and functionality
is scattered between different packages.
Using a new standard library importlib.metadata to find installed
packages and their versions. Using packaging.requirements to parse
lines from the requirements file and compare versions. This covers
all we need.
The 'packaging' is a project used by pip and a dependency for many
other libraries, so should be available for any supported verison of
python. 'importlib' was introduced in python 3.8. Since we support
older versions of python and 'packaging' is not part of the standard
library, checking that import is possible and falling back to
'pkg_resources' if needed. We may remove the fallback when we stop
supporting python below 3.8.
Even though 'packaging' is a common dependency, added to the test
requirements so it will not be missed in CI.
Signed-off-by: Ilya Maximets
---
python/test_requirements.txt | 1 +
tests/atlocal.in | 28 ++--
2 files changed, 23 insertions(+), 6 deletions(-)
diff --git a/python/test_requirements.txt b/python/test_requirements.txt
index 5043c71e2..a1424506b 100644
--- a/python/test_requirements.txt
+++ b/python/test_requirements.txt
@@ -1,4 +1,5 @@
netaddr
+packaging
pyftpdlib
pyparsing
pytest
diff --git a/tests/atlocal.in b/tests/atlocal.in
index 466fd4ed4..8565a0bae 100644
--- a/tests/atlocal.in
+++ b/tests/atlocal.in
@@ -229,15 +229,31 @@ export UBSAN_OPTIONS
REQUIREMENT_PATH=$abs_top_srcdir/python/test_requirements.txt $PYTHON3 -c '
import os
import pathlib
-import pkg_resources
import sys
+PACKAGING = True
+try:
+from packaging import requirements
+from importlib import metadata
+except ModuleNotFoundError:
+PACKAGING = False
+import pkg_resources
+
with pathlib.Path(os.path.join(os.getenv("REQUIREMENT_PATH"))).open() as reqs:
-for req in pkg_resources.parse_requirements(reqs):
-try:
-pkg_resources.require(str(req))
-except pkg_resources.DistributionNotFound:
-sys.exit(2)
+if PACKAGING:
+for req in reqs.readlines():
+try:
+r = requirements.Requirement(req.strip())
+if metadata.version(r.name) not in r.specifier:
+raise metadata.PackageNotFoundError
+except metadata.PackageNotFoundError:
+sys.exit(2)
+else:
+for req in pkg_resources.parse_requirements(reqs):
+try:
+pkg_resources.require(str(req))
+except pkg_resources.DistributionNotFound:
+sys.exit(2)
'
case $? in
0) HAVE_PYTEST=yes ;;
--
2.45.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 1/2] atlocal: Fix setting HAVE_PYTEST on unexpected errors.
If the python script throws an unexpected exception, the HAVE_PYTEST
variable remains undefined. If at the same time dependencies are not
actually present, pytest tests will fail instead of being skipped.
Define the variable to 'no' on unexpected failures to skip the tests
when dependencies cannot be verified.
The issue can be reproduced on systems with python 3.12+ in case the
deprecated 'pkg_resources' module is not available.
Fixes: 445dceb88461 ("python: Introduce unit tests.")
Signed-off-by: Ilya Maximets
---
tests/atlocal.in | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tests/atlocal.in b/tests/atlocal.in
index f321bae55..466fd4ed4 100644
--- a/tests/atlocal.in
+++ b/tests/atlocal.in
@@ -242,5 +242,6 @@ with
pathlib.Path(os.path.join(os.getenv("REQUIREMENT_PATH"))).open() as reqs:
case $? in
0) HAVE_PYTEST=yes ;;
2) HAVE_PYTEST=no ;;
-*) echo "$0: unexpected error probing Python unit test requirements" >&2 ;;
+*) HAVE_PYTEST=no
+ echo "$0: unexpected error probing Python unit test requirements" >&2 ;;
esac
--
2.45.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 0/2] atlocal: Fixes for dependency detection.
'pkg_resources' module is deprecated, so it needs a replacement. Ilya Maximets (2): atlocal: Fix setting HAVE_PYTEST on unexpected errors. atlocal: Replace deprecated pkg_resources. python/test_requirements.txt | 1 + tests/atlocal.in | 31 --- 2 files changed, 25 insertions(+), 7 deletions(-) -- 2.45.0 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] srv6: Fix misaligned writes to segment list.
Segments list in SRv6 header is 16-bit aligned as most of other fields
in packet headers. A little counter-intuitively, compilers are allowed
to make alignment assumptions based on the pointer type passed to
memcpy(), so they can use copy instructions that require 32-bit alignment
in case of struct in6_addr pointer. Reported by UBsan in Clang 18:
lib/netdev-native-tnl.c:985:16: runtime error: store to misaligned
address 0x7fd9e97351ce for type 'struct in6_addr *', which
requires 4 byte alignment
0x7fd9e97351ce: note: pointer points here
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
^
0 0xc1de38 in netdev_srv6_build_header lib/netdev-native-tnl.c:985:9
1 0x6e794b in tnl_port_build_header ofproto/tunnel.c:751:11
2 0x6c9c0a in native_tunnel_output ofproto/ofproto-dpif-xlate.c:3887:11
3 0x6c9c0a in compose_output_action__ ofproto/ofproto-dpif-xlate.c:4502:13
4 0x6b6646 in compose_output_action ofproto/ofproto-dpif-xlate.c:4564:5
5 0x6b6646 in xlate_output_action ofproto/ofproto-dpif-xlate.c:5517:13
6 0x68cfee in do_xlate_actions ofproto/ofproto-dpif-xlate.c:7288:13
7 0x67fed0 in xlate_actions ofproto/ofproto-dpif-xlate.c:8314:13
8 0x6468bd in ofproto_trace__ ofproto/ofproto-dpif-trace.c:782:30
9 0x64484a in ofproto_trace ofproto/ofproto-dpif-trace.c:851:5
10 0x647469 in ofproto_unixctl_trace ofproto/ofproto-dpif-trace.c:490:9
11 0xc33771 in process_command lib/unixctl.c:310:13
12 0xc33771 in run_connection lib/unixctl.c:344:17
13 0xc33771 in unixctl_server_run lib/unixctl.c:395:21
14 0x53e6ef in main vswitchd/ovs-vswitchd.c:131:9
15 0x7f61c7 in __libc_start_call_main (/lib64/libc.so.6+0x2a1c7)
16 0x7f628a in __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x2a28a)
17 0x42ca24 in _start (vswitchd/ovs-vswitchd+0x42ca24)
SUMMARY: UndefinedBehaviorSanitizer:
undefined-behavior lib/netdev-native-tnl.c:985:16
Having misaligned pointers is also generally not allowed in C, let
alone accessing memory through them.
Fix that by using an appropriate ovs_16aligned_in6_addr pointer instead.
Fixes: 7381fd440a88 ("odp: Add SRv6 tunnel actions.")
Fixes: 03fc1ad78521 ("userspace: Add SRv6 tunnel support.")
Signed-off-by: Ilya Maximets
---
lib/netdev-native-tnl.c | 5 ++---
lib/odp-util.c | 4 ++--
2 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
index dee9ab344..b21176037 100644
--- a/lib/netdev-native-tnl.c
+++ b/lib/netdev-native-tnl.c
@@ -932,9 +932,9 @@ netdev_srv6_build_header(const struct netdev *netdev,
const struct netdev_tnl_build_header_params *params)
{
const struct netdev_tunnel_config *tnl_cfg;
+union ovs_16aligned_in6_addr *s;
const struct in6_addr *segs;
struct srv6_base_hdr *srh;
-struct in6_addr *s;
ovs_be16 dl_type;
int nr_segs;
int i;
@@ -978,8 +978,7 @@ netdev_srv6_build_header(const struct netdev *netdev,
return EOPNOTSUPP;
}
-s = ALIGNED_CAST(struct in6_addr *,
- (char *) srh + sizeof *srh);
+s = (union ovs_16aligned_in6_addr *) (srh + 1);
for (i = 0; i < nr_segs; i++) {
/* Segment list is written to the header in reverse order. */
memcpy(s, [nr_segs - i - 1], sizeof *s);
diff --git a/lib/odp-util.c b/lib/odp-util.c
index 21f34d955..724e6f2bc 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -1820,8 +1820,8 @@ ovs_parse_tnl_push(const char *s, struct
ovs_action_push_tnl *data)
} else if (ovs_scan_len(s, , "srv6(segments_left=%"SCNu8,
_left)) {
struct srv6_base_hdr *srh = (struct srv6_base_hdr *) (ip6 + 1);
+union ovs_16aligned_in6_addr *segs;
char seg_s[IPV6_SCAN_LEN + 1];
-struct in6_addr *segs;
struct in6_addr seg;
uint8_t n_segs = 0;
@@ -1844,7 +1844,7 @@ ovs_parse_tnl_push(const char *s, struct
ovs_action_push_tnl *data)
return -EINVAL;
}
-segs = ALIGNED_CAST(struct in6_addr *, srh + 1);
+segs = (union ovs_16aligned_in6_addr *) (srh + 1);
segs += segments_left;
while (ovs_scan_len(s, , IPV6_SCAN_FMT, seg_s)
--
2.45.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] compiler: Fix errors in Clang 17 ubsan checks.
On 5/17/24 14:59, Eelco Chaudron wrote: > > > On 16 May 2024, at 15:57, Mike Pattrick wrote: > >> This patch attempts to fix a large number of ubsan error messages that >> take the following form: >> >> lib/netlink-notifier.c:237:13: runtime error: call to function >> route_table_change through pointer to incorrect function type >> 'void (*)(const void *, void *)' >> >> In Clang 17 the undefined behaviour sanatizer check for function >> pointers was enabled by default, whereas it was previously disabled >> while compiling C code. These warnings are a false positive in the case >> of OVS, as our macros already check to make sure the function parameter >> is the correct size. >> >> So that check is disabled in the single function that is causing all of >> the errors. >> >> Signed-off-by: Mike Pattrick > > Thanks for fixing the naming. > > Acked-by: Eelco Chaudron Thanks, Mike, Jakob and Eelco! I fixed the comment spacing and moved the attribute to the same line with a return type as we normally do. With that, applied to all the branches down to 2.17. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4] lib: Assert for incorrect packet.
On 5/15/24 14:15, Amit Prakash Shukla wrote: > Packet that are not encapsulated but metadata of the packet contains > a offload flag set, will call dp_packet_inner_l4 to get TCP, UDP, SCTP > header pointers. dp_packet_inner_l4 for such packets would return NULL > as the inner offsets by-default are configured as UINT16_MAX. On > derefrencing such pointers, segfault is observed. > > Add assert check for packets with incorrect header or incorrect > offload flag set. > > Signed-off-by: Amit Prakash Shukla > --- > v2: > - Added Fixes tag and updated commit message. > > v3: > - Resolved review comment - added assert. > - Updated patch subject and commit message. > > v4: > - Fixed checkpatch warning. Thanks! I added a note to the commit message that the crash was not related to OVS logic and applied the change. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] table: Fix freeing global variable.
On 5/15/24 17:52, Simon Horman wrote:
> On Wed, May 15, 2024 at 02:14:13PM +0800, Yunjian Wang via dev 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)
>>
>> Fixes: cb139fa8b3a1 ("table: New function table_format() for formatting a
>> table as a string.")
>> Signed-off-by: Pengfei Sun
>
> Acked-by: Simon Horman
>
Thanks, Pengfei, Eelco and Simon!
Applied and backpotred down to 2.17.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] vlog: Destroy async_append first then close log_fd.
On 5/15/24 18:01, Simon Horman wrote: > On Wed, May 15, 2024 at 11:28:21AM +0800, hepeng via dev wrote: >> From: Peng He >> >> async_append stores log_fd, it should be destructed before log_fd >> is closed. >> >> Signed-off-by: Peng He > > Acked-by: Simon Horman > Applied to main and backported down to 2.17. Thanks! Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [v2] odp-execute: Fix AVX checksum calculation.
On 5/15/24 12:12, Eelco Chaudron wrote: > > > On 14 May 2024, at 15:48, Emma Finn wrote: > >> The AVX implementation for calcualting checksums was not >> handling carry-over addition correctly in some cases. >> This patch adds an additional shuffle to add 16-bit padding >> to the final part of the calculation to handle such cases. >> This commit also adds a unit test to fuzz test the actions >> autovalidator. >> >> Signed-off-by: Emma Finn >> Reported-by: Eelco Chaudron > > Hi Emma, > > Thanks for also fixing the IPv6 case, however, the test you added does > not seem to catch the issue. See notes below. > > Cheers, > > Eelco > >> --- >> lib/odp-execute-avx512.c | 5 + >> tests/dpif-netdev.at | 26 ++ >> 2 files changed, 31 insertions(+) >> >> diff --git a/lib/odp-execute-avx512.c b/lib/odp-execute-avx512.c >> index 50c48bfd4..a74a85dc1 100644 >> --- a/lib/odp-execute-avx512.c >> +++ b/lib/odp-execute-avx512.c >> @@ -366,6 +366,8 @@ avx512_get_delta(__m256i old_header, __m256i new_header) >>0xF, 0xF, 0xF, 0xF); >> v_delta = _mm256_permutexvar_epi32(v_swap32a, v_delta); >> >> +v_delta = _mm256_hadd_epi32(v_delta, v_zeros); >> +v_delta = _mm256_shuffle_epi8(v_delta, v_swap16a); >> v_delta = _mm256_hadd_epi32(v_delta, v_zeros); >> v_delta = _mm256_hadd_epi16(v_delta, v_zeros); >> >> @@ -575,6 +577,9 @@ avx512_ipv6_sum_header(__m512i ip6_header) >>0xF, 0xF, 0xF, 0xF); >> >> v_delta = _mm256_permutexvar_epi32(v_swap32a, v_delta); >> + >> +v_delta = _mm256_hadd_epi32(v_delta, v_zeros); >> +v_delta = _mm256_shuffle_epi8(v_delta, v_swap16a); >> v_delta = _mm256_hadd_epi32(v_delta, v_zeros); >> v_delta = _mm256_hadd_epi16(v_delta, v_zeros); >> >> diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at >> index 790b5a43a..4db6a99e1 100644 >> --- a/tests/dpif-netdev.at >> +++ b/tests/dpif-netdev.at >> @@ -1091,3 +1091,29 @@ OVS_VSWITCHD_STOP(["dnl >> /Error: unknown miniflow extract implementation superstudy./d >> /Error: invalid study_pkt_cnt value: -pmd./d"]) >> AT_CLEANUP >> + >> +AT_SETUP([datapath - Actions Autovalidator Fuzzy]) > > This is not a Fuzzy test, but a normal Actions Autovalidator. FWIW, even if it was, I don't think we should add any more fuzzy tests in a general testsuite. And we should find a way to get rid of the existing ones. Having non-reproducible tests is not good. > > However, the main problem with this test is that it does not find the problem. > Even without the C code changes, it’s passing the test. > > Maybe it will be better to add a specific test to capture checksum wrapping > for > IPv4 and 6. In addition, you should also make sure the received packet is ok. > You can use options:pcap=p1.pcap for this, see other test cases. I'd suggest to model the test after 'userspace offload - ip csum offload' test case we have in tests/dpif-netdev.at. It does very similar checks. > >> +AT_SKIP_IF([! $PYTHON3 -c "import scapy"], [], []) >> +AT_SKIP_IF([! $PYTHON3 $srcdir/genpkts.py 2000 > packets]) >> + >> +OVS_VSWITCHD_START(add-port br0 p0 -- set Interface p0 type=dummy \ >> + -- add-port br0 p1 -- set Interface p1 type=dummy) >> + >> +AT_CHECK([ovs-appctl odp-execute/action-impl-set autovalidator], [0], [dnl >> +Action implementation set to autovalidator. >> +]) >> + >> +AT_DATA([flows.txt], [dnl >> + in_port=p0,ip,actions=mod_nw_src=10.1.1.1,p1 >> + in_port=p0,ipv6,actions=set_field:fc00::100->ipv6_src,p1 >> +]) >> + >> +AT_CHECK([ovs-ofctl del-flows br0]) >> +AT_CHECK([ovs-ofctl -Oopenflow13 add-flows br0 flows.txt]) >> + >> +cat packets | while read line; do >> + AT_CHECK([ovs-appctl netdev-dummy/receive p0 $line], [0], [ignore]) >> +done >> + >> +OVS_TRAFFIC_VSWITCHD_STOP >> +AT_CLEANUP >> -- >> 2.25.1 > > ___ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] fix ct tp policy when create zone.
On 5/14/24 09:50, Chandler Wu wrote:
> 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.
Ah, that makes sense. Please, add this information to the commit message
before sending v2.
Please, also add the following tag:
Fixes: 1b3557f53dbc ("vswitchd, ofproto-dpif: Propagate the CT limit from
database.")
For the code itself:
Maybe we should remove the get_timeout_policy_from_ovsrec() call from
ct_zone_alloc() instead of adding ofproto_ct_set_zone_timeout_policy()?
If we do not initialize it, the update_timeout_policy() later should
sync it correctly. What do you think?
Best regards, Ilya Maximets.
>
> Best regards.
>
> On Tue, May 14, 2024 at 5:11 AM Ilya Maximets <mailto:i.maxim...@ovn.org>> wrote:
>
> On 5/6/24 06:05, Chandler Wu wrote:
> > From 5b4d479a9083272e56c51ef9521e6ef665dd534d Mon Sep 17 00:00:00 2001
> > From: chandlerwu <mailto:chandler0...@gmail.com>>
> > Date: Mon, 6 May 2024 11:58:21 +0800
> > Subject: [PATCH] Subject:[PATCH] fix ct tp policy when create zone.
> >
> > Signed-off-by: chandlerwu <mailto:chandler0...@gmail.com>>
> > ---
> > 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(>ct_zones, _zone->node,
> hash_int(zone_id, 0));
> > + ofproto_ct_set_zone_timeout_policy(dp->type,
> ct_zone->zone_id,
> > + _zone->tp);
> > }
> >
> > struct simap new_tp = SIMAP_INITIALIZER(_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 v3] route-table: Add support for v4 via v6 route.
On 5/14/24 00:42, Ilya Maximets wrote: > On 5/14/24 00:11, William Tu wrote: >> Add route-table support for ipv4 dst via ipv6. One use case is BGP >> unnumbered, a mechanism that establishes peering sessions without the >> need to explicitly configure IPv4 addresses on the interfaces involved >> in the peering. Without using IPv4 address assignments, it uses >> link-local IPv6 addresses of the directly connected neighbors for >> peering purposes. For example, BGP might install the following route: >> $ ip route get 100.87.18.3 >> 100.87.18.3 via inet6 fe80::920a:84ff:fe9e:9570 \ >> dev br-phy src 100.87.18.6 >> >> Note that the v6 addr fe80::920a:84ff:fe9e:9570 is not being used in >> the packet header, but only used for lookup the out dev br-phy. >> Currently OVS can only support either all-ipv4 or all-ipv6, the patch >> adds support for such use case. >> >> Reported-at: >> https://mail.openvswitch.org/pipermail/ovs-discuss/2024-January/052908.html >> Acked-by: Simon Horman >> Signed-off-by: William Tu >> --- >> v3: add vxlan test, remove rfc >> v2: fix CI error >> --- >> lib/ovs-router.c | 34 ++-- >> lib/route-table.c| 21 ++ >> tests/ovs-router.at | 33 +++ >> tests/system-route.at| 24 >> tests/tunnel-push-pop.at | 48 >> 5 files changed, 142 insertions(+), 18 deletions(-) I didn't look at the code changes too closely, but in case you wan't to re-spin the patch, I added a few comments for the test cases below. Best regards, Ilya Maximets. >> diff --git a/tests/ovs-router.at b/tests/ovs-router.at >> index b3314b3dff0d..f34dd3c99f5d 100644 >> --- a/tests/ovs-router.at >> +++ b/tests/ovs-router.at >> @@ -109,6 +109,39 @@ User: 2001:db8:beef::14/128 MARK 14 dev br0 GW >> 2001:db8:cafe::1 SRC 2001:db8:caf >> OVS_VSWITCHD_STOP >> AT_CLEANUP >> >> +AT_SETUP([appctl - route/add with ipv4 via ipv6]) We should probably also add a few cases for the lookup and del, not just the add. >> +AT_KEYWORDS([ovs_router]) >> +OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=dummy]) >> +AT_CHECK([ovs-appctl netdev-dummy/ip4addr br0 192.168.9.2/24], [0], [OK >> +]) >> +AT_CHECK([ovs-appctl netdev-dummy/ip4addr br0 192.168.9.3/24], [0], [OK >> +]) >> + >> +# defualt pick the first IP, 192.168.9.2, as src >> +AT_CHECK([ovs-appctl ovs/route/add 192.168.9.10/32 br0 >> fe80::920a:84ff:beef:9510], [0], [OK >> +]) >> +AT_CHECK([ovs-appctl ovs/route/add 192.168.9.11/32 br0 >> fe80::920a:84ff:beef:9511 src=192.168.9.2], [0], [OK >> +]) >> +AT_CHECK([ovs-appctl ovs/route/add 192.168.9.12/32 br0 >> fe80::920a:84ff:beef:9512 src=192.168.9.3], [0], [OK >> +]) >> +AT_CHECK([ovs-appctl ovs/route/add 192.168.9.13/32 br0 >> fe80::920a:84ff:beef:9513 pkt_mark=13 src=192.168.9.3], [0], [OK >> +]) Might be better to wrap some of the longer lines, e.g. after the bridge name. >> + >> +AT_CHECK([ovs-appctl ovs/route/show | grep User | grep beef | sort], [0], >> [dnl >> +User: 192.168.9.10/32 dev br0 GW fe80::920a:84ff:beef:9510 SRC 192.168.9.2 >> +User: 192.168.9.11/32 dev br0 GW fe80::920a:84ff:beef:9511 SRC 192.168.9.2 >> +User: 192.168.9.12/32 dev br0 GW fe80::920a:84ff:beef:9512 SRC 192.168.9.3 >> +User: 192.168.9.13/32 MARK 13 dev br0 GW fe80::920a:84ff:beef:9513 SRC >> 192.168.9.3 >> +]) >> + >> +# DST and SRC must be in the same subnet domain >> +AT_CHECK([ovs-appctl ovs/route/add 192.168.10.12/32 br0 >> fe80::920a:84ff:beef:9512 src=192.168.9.3], [2], [], [dnl >> +Error while inserting route. >> +ovs-appctl: ovs-vswitchd: server returned an error >> +]) >> +OVS_VSWITCHD_STOP >> +AT_CLEANUP >> + >> AT_SETUP([appctl - route/lookup]) >> AT_KEYWORDS([ovs_router]) >> OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=dummy]) >> diff --git a/tests/system-route.at b/tests/system-route.at >> index c0ecad6cfb49..fd698321b401 100644 >> --- a/tests/system-route.at >> +++ b/tests/system-route.at >> @@ -65,6 +65,30 @@ Cached: fc00:db8:beef::13/128 dev br0 GW fc00:db8:cafe::1 >> SRC fc00:db8:cafe::2]) >> OVS_TRAFFIC_VSWITCHD_STOP >> AT_CLEANUP >> >> +AT_SETUP([ovs-route - add system route ipv4 via ipv6]) >> +AT_KEYWORDS([route]) >> +OVS_TRAFFIC_VSWITCHD_START() >> +AT_CHECK([ip link set br0 up]) >> + >> +AT_CHECK([ip addr add 192.168.9.2/24 dev br0], [0], [stdout]) >&g
Re: [ovs-dev] [PATCH v3] utilities: Correct deletion reason in flow_reval_monitor.py.
On 5/14/24 15:15, 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
>
> ---
> 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.
> ---
I didn't test, but the code looks fine to me. Thanks!
Acked-by: Ilya Maximets
BTW, is there a reason why we can't just report a static string
from the USDT probe instead of an integer? If we did that we
would not need to have this mapping in the script at all.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v29 0/8] Add offload support for sFlow
On 3/26/24 03:30, Chris Mi wrote: > This patch set adds offload support for sFlow. > > Psample is a genetlink channel for packet sampling. TC action act_sample > uses psample to send sampled packets to userspace. > > When offloading sample action to TC, userspace creates a unique ID to > map sFlow action and tunnel info and passes this ID to kernel instead > of the sFlow info. psample will send this ID and sampled packet to > userspace. Using the ID, userspace can recover the sFlow info and send > sampled packet to the right sFlow monitoring host. Hi, Chris, others. I've been looking through the set and a psample in general for the past few days. There are few fairly minor issues in the set like UBsan crash because of incorrect OVS_RETURNS_NONNULL annotation and a few style and formatting issues, which are not hard to fix. However, I encountered an issue with a way tunnel metadata is recovered that I'm not sure we can actually fix. The algorithm of work in this patch set is described in the cover letter above. The key point is then packet-1 goes though MISS upcall we install a new datapath flow into TC and we remember the tunnel metadata of the packet-1 associated with a sample ID. When the packet-2 hits the datapath flow (tc actions), it gets sent to psample and ovs-vswitchd reads this packet from psample netlink socket and presents it as an ACTION upcall. Doing that it finds tunnel metadata using the sample ID and uses it as a tunnel metadata for packet-2. The key of the problem is that this is metadata from packet-1 and it can be different from metadata of packet-2. An example of this will be having two separate TCP streams going through the same VxLAN tunnel. For load balancing reasons most UDP tunnels choose source port of the outer UDP header based on RSS or 5-tuple hash of the inner packet. This means that two TCP streams going through the same tunnel will have different UDP source port in the outer header. When a packet from a first stream hits a MISS upcall, datapath flow will be installed and the sample ID will be associated with the metadata of the first stream. Chances are this datapath flow will not have exact match on the source port of the tunnel metadata. That means that a packet from the second stream will successfully match on the installed datapath flow and will go to psample with the ID of the first stream. OVS will read it from the psample socket and send to sFlow collector with metadata it found by the ID, i.e. with a tunnel metadata that contains tunnel source port of the first TCP stream, which is incorrect. The test below reproduces the issue. It's not a real test, it always fails on purpose and not very pretty, but what it does is that it creates a tunnel, then starts netcat server on one side and sends two files to it from the other side. These two transfers are separate TCP connections, so they use different source ports, that is causing the tunnel to choose different UDP source ports for them. After the test failure we can see in the sflow.log that all the values for 'tunnel4_in_src_port' are the same for both streams and some exchanges prior to that. However, if offload is disabled, the values will be different as they should. So, unless we can ensure that packets from different streams do not match on the same datapath flow, this schema doesn't work. The only way to ensure that packets from different streams within the same tunnel do not match on the same datapath flow is to always have an exact match on the whole tunnel metadata. But I don't think that is a good idea, because this way we'll have a separate datapath flow per TCP stream, which will be very bad for performance. And it will be bad for hardware offload since hardware NICs normally have a more limited amount of available resources. This leads us to conclusion that only non-tunneling traffic can be offloaded this way. For this to work we'll have to add an explicit match on tunnel metadata being empty (can that be expressed in TC?) But at this point a question arises if this even worth implementing, taking into account that it requires a lot of infrastructure changes throughout all the layers of OVS code just for sampling of non-tunnel packets, i.e. a very narrow use case. Also, my understanding was that there is a plan to offload other types of userspace() action in the future, such as controller() action using the same psample infrastructure. But that will not be possible for the same reason. In addition to tunnel metadata we will also have to provide connection tracking information, which is even harder, because conntrack state is more dynamic and is only actually known to the datapath. psample can't supply this information to the userspace. Thoughts? Best regards, Ilya Maximets. P.S. The test that reproduces the issue: --- diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at index ee4817e8e..51abc2782 100644 --- a/te
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] [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?
>>>>>>>>>
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
>>>>>>> e
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
>>
Re: [ovs-dev] [RFC 00/11] Add psample support to NXAST_SAMPLE action.
ly 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.
Collector is always an external process...
>>>
>>> "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 "dp" for
>>> short would yield kind of redundant odp attributes:
>>> SAMPLE_ATTR_{DP_GROUP,DP_USER_COOKIE} but could still work...
>>>
>>> Any other suggestion or preference?
>>
>> Yes naming is hard, I just thought of ‘localhost’, ‘localEndpoint’, just
>> adding another option…
>>
>> Does anyone else have a cool name?!?
>
>
> Ilya, do you have any suggestion or preference? Mine are "raw" and "system".
TBH, "local" sounds best to me, because other sampling methods send
packets somewhere remote, while this one only emits them locally on
the same system.
"system" maybe a second best, but it is so ambiguous so it doesn't
really mean anything. "raw" also means nothing to me and it is not
even raw, it's wrapped into a netlink message with some cookie / IDs
attached to it.
For the kernel uAPI we may avoid naming the sampling method by naming
the action instead. See the other thread.
We can't do that for the database interface, in-code functions and
structures though, so we need a name for those. I'd vote of "local",
"local_sample", e.g.:
create FLow_Sample_Collector_Set id=1 local-sample-group=10
Best 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.
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
Re: [ovs-dev] [RFC 01/11] odp-util: Add support to new psample uAPI.
/* 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.
Naming is still hard though. I like the word "emit" because it doesn't have a
concept of directionality, i.e. we're not sending it to somewhere, we're just
emitting it to the wild. That kind of matches with our vague definition of a
"local" or "system" sampling.
Another option might be 'OUTPUT_SAMPLE/SAMPLE_OUTPUT', i.e. output(sample,
cookie=...).
The main concern is that we already have truncation implemented, so we can be
more
flexible in a way we generate actions instead of adding new attributes.
Thoughts?
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] [PATCH v3] route-table: Add support for v4 via v6 route.
On 5/14/24 00:11, William Tu wrote:
> Add route-table support for ipv4 dst via ipv6. One use case is BGP
> unnumbered, a mechanism that establishes peering sessions without the
> need to explicitly configure IPv4 addresses on the interfaces involved
> in the peering. Without using IPv4 address assignments, it uses
> link-local IPv6 addresses of the directly connected neighbors for
> peering purposes. For example, BGP might install the following route:
> $ ip route get 100.87.18.3
> 100.87.18.3 via inet6 fe80::920a:84ff:fe9e:9570 \
> dev br-phy src 100.87.18.6
>
> Note that the v6 addr fe80::920a:84ff:fe9e:9570 is not being used in
> the packet header, but only used for lookup the out dev br-phy.
> Currently OVS can only support either all-ipv4 or all-ipv6, the patch
> adds support for such use case.
>
> Reported-at:
> https://mail.openvswitch.org/pipermail/ovs-discuss/2024-January/052908.html
> Acked-by: Simon Horman
> Signed-off-by: William Tu
> ---
> v3: add vxlan test, remove rfc
> v2: fix CI error
> ---
> lib/ovs-router.c | 34 ++--
> lib/route-table.c| 21 ++
> tests/ovs-router.at | 33 +++
> tests/system-route.at| 24
> tests/tunnel-push-pop.at | 48
> 5 files changed, 142 insertions(+), 18 deletions(-)
>
> diff --git a/lib/ovs-router.c b/lib/ovs-router.c
> index 3d84c9a30a8f..3ee927e6f7de 100644
> --- a/lib/ovs-router.c
> +++ b/lib/ovs-router.c
> @@ -415,7 +415,6 @@ ovs_router_add(struct unixctl_conn *conn, int argc,
> unsigned int plen;
> ovs_be32 src = 0;
> ovs_be32 gw = 0;
> -bool is_ipv6;
> ovs_be32 ip;
> int err;
> int i;
> @@ -423,9 +422,8 @@ ovs_router_add(struct unixctl_conn *conn, int argc,
> if (scan_ipv4_route(argv[1], , )) {
> in6_addr_set_mapped_ipv4(, ip);
> plen += 96;
> -is_ipv6 = false;
> } else if (scan_ipv6_route(argv[1], , )) {
> -is_ipv6 = true;
> +;
> } else {
> unixctl_command_reply_error(conn,
> "Invalid 'ip/plen' parameter");
> @@ -438,21 +436,21 @@ ovs_router_add(struct unixctl_conn *conn, int argc,
> continue;
> }
>
> -if (is_ipv6) {
> -if (ovs_scan(argv[i], "src="IPV6_SCAN_FMT, src6_s) &&
> -ipv6_parse(src6_s, )) {
> -continue;
> -}
> -if (ipv6_parse(argv[i], )) {
> -continue;
> -}
> -} else {
> -if (ovs_scan(argv[i], "src="IP_SCAN_FMT, IP_SCAN_ARGS())) {
> -continue;
> -}
> -if (ip_parse(argv[i], )) {
> -continue;
> -}
> +if (ovs_scan(argv[i], "src="IPV6_SCAN_FMT, src6_s) &&
> +ipv6_parse(src6_s, )) {
> +continue;
> +}
> +
> +if (ipv6_parse(argv[i], )) {
> +continue;
> +}
> +
> +if (ovs_scan(argv[i], "src="IP_SCAN_FMT, IP_SCAN_ARGS())) {
> +continue;
> +}
> +
> +if (ip_parse(argv[i], )) {
> +continue;
> }
>
> unixctl_command_reply_error(conn,
> diff --git a/lib/route-table.c b/lib/route-table.c
> index f1fe32714e8d..58412711888f 100644
> --- a/lib/route-table.c
> +++ b/lib/route-table.c
> @@ -232,6 +232,7 @@ route_table_parse(struct ofpbuf *buf, struct
> route_table_msg *change)
> [RTA_OIF] = { .type = NL_A_U32, .optional = true },
> [RTA_GATEWAY] = { .type = NL_A_U32, .optional = true },
> [RTA_MARK] = { .type = NL_A_U32, .optional = true },
> +[RTA_VIA] = { .type = NL_A_UNSPEC, .optional = true },
> [RTA_PREFSRC] = { .type = NL_A_U32, .optional = true },
> [RTA_TABLE] = { .type = NL_A_U32, .optional = true },
> };
> @@ -241,6 +242,7 @@ route_table_parse(struct ofpbuf *buf, struct
> route_table_msg *change)
> [RTA_OIF] = { .type = NL_A_U32, .optional = true },
> [RTA_MARK] = { .type = NL_A_U32, .optional = true },
> [RTA_GATEWAY] = { .type = NL_A_IPV6, .optional = true },
> +[RTA_VIA] = { .type = NL_A_UNSPEC, .optional = true },
> [RTA_PREFSRC] = { .type = NL_A_IPV6, .optional = true },
> [RTA_TABLE] = { .type = NL_A_U32, .optional = true },
> };
> @@ -333,6 +335,25 @@ route_table_parse(struct ofpbuf *buf, struct
> route_table_msg *change)
> nl_attr_get_in6_addr(attrs[RTA_PREFSRC]);
> }
> }
> +if (attrs[RTA_VIA]) {
> +const struct rtvia *via;
> +ovs_be32 gw;
> +
> +via = nl_attr_get(attrs[RTA_VIA]);
> +switch (via->rtvia_family) {
> +case AF_INET:
> +gw = *(ALIGNED_CAST(ovs_be32 *, via->rtvia_addr));
> +in6_addr_set_mapped_ipv4(>rd.rta_gw, gw);
> +break;
Re: [ovs-dev] [PATCH] fix ct tp policy when create zone.
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(>ct_zones, _zone->node, hash_int(zone_id, 0));
> +ofproto_ct_set_zone_timeout_policy(dp->type, ct_zone->zone_id,
> + _zone->tp);
> }
>
> struct simap new_tp = SIMAP_INITIALIZER(_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 v2] utilities: Correct deletion reason in flow_reval_monitor.py.
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.
> +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 +572,10 @@ def print_expiration(event):
> """Prints a UFID
Re: [ovs-dev] [RFC 06/11] utilities: Add ovs-psample.
gt;>>> in the name, as it's rather general and not OVS specific. Maybe just >>>> psample/psample_mon, as we also have nlmon. >>>> >>> >>> Well, the way the cookie is decoded into observation_domain_id and >>> observation_point_id is ovs-specific. >>> >>> In fact, for the next iteration of the series I will remove the filtering >>> API since its getting removed from the kernel series as well and add some >>> kind of BPF code that does the filtering. Also I was considering looking >>> into the OVSDB to ensure that we filter on groups configured in it or else >>> we could wrongly interpet a sampled packet that comes from some other >>> subsystem. >> >> I would prefer to have this as an ova-sample application, which can be >> extended with other sample methods as we see fit. So when we added userspace >> support we can add it here. If we find someone crazy enough to do a simple >> IPFIX and/or sFlow collector it can be added too. >> >> So my request would be to remove the (p) from ovs-psample, and have a switch >> to select --psample (the only supported (MANDATORY) option for now). >> > > Oh, ok. Now I undestand, thank you. We want to leave room for the userspace > implementation. Will do. FYI, we do have sflow and netflow basic collectors in tests/test-sflow.c and tests/test-netflow.c. And I'm not convinced we should maintain an actual collector for any of these. It's not OVS'es job as a project. Like we're not shipping nlmon, we probably shouldn't ship psample or any other collector. But we can and should have basic implementations for testing purposes in tests/test-psample.c for example. Best 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.
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
Re: [ovs-dev] [RFC 04/11] vswitchd: Add psample to schema and configure it.
On 4/24/24 21:53, Adrian Moreno wrote:
> Add a psample_group field to the Flow Sample Collector Set table and use
> it to configure the psample ofproto layer.
>
> Signed-off-by: Adrian Moreno
> ---
> vswitchd/bridge.c | 54 ++
> vswitchd/vswitch.ovsschema | 7 -
> vswitchd/vswitch.xml | 32 +++---
> 3 files changed, 83 insertions(+), 10 deletions(-)
>
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index 95a65fcdc..474eb1650 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -288,6 +288,7 @@ static void bridge_configure_mac_table(struct bridge *);
> static void bridge_configure_mcast_snooping(struct bridge *);
> static void bridge_configure_sflow(struct bridge *, int
> *sflow_bridge_number);
> static void bridge_configure_ipfix(struct bridge *);
> +static void bridge_configure_psample(struct bridge *);
> static void bridge_configure_spanning_tree(struct bridge *);
> static void bridge_configure_tables(struct bridge *);
> static void bridge_configure_dp_desc(struct bridge *);
> @@ -989,6 +990,7 @@ bridge_reconfigure(const struct ovsrec_open_vswitch
> *ovs_cfg)
> bridge_configure_netflow(br);
> bridge_configure_sflow(br, _bridge_number);
> bridge_configure_ipfix(br);
> +bridge_configure_psample(br);
> bridge_configure_spanning_tree(br);
> bridge_configure_tables(br);
> bridge_configure_dp_desc(br);
> @@ -1537,10 +1539,11 @@ ovsrec_ipfix_is_valid(const struct ovsrec_ipfix
> *ipfix)
> return ipfix && ipfix->n_targets > 0;
> }
>
> -/* Returns whether a Flow_Sample_Collector_Set row is valid. */
> +/* Returns whether a Flow_Sample_Collector_Set row constains valid IPFIX
typo
> + * configuration. */
> static bool
> -ovsrec_fscs_is_valid(const struct ovsrec_flow_sample_collector_set *fscs,
> - const struct bridge *br)
> +ovsrec_fscs_is_valid_ipfix(const struct ovsrec_flow_sample_collector_set
> *fscs,
> + const struct bridge *br)
> {
> return ovsrec_ipfix_is_valid(fscs->ipfix) && fscs->bridge == br->cfg;
> }
> @@ -1558,7 +1561,7 @@ bridge_configure_ipfix(struct bridge *br)
> const char *virtual_obs_id;
>
> OVSREC_FLOW_SAMPLE_COLLECTOR_SET_FOR_EACH(fe_cfg, idl) {
> -if (ovsrec_fscs_is_valid(fe_cfg, br)) {
> +if (ovsrec_fscs_is_valid_ipfix(fe_cfg, br)) {
> n_fe_opts++;
> }
> }
> @@ -1621,7 +1624,7 @@ bridge_configure_ipfix(struct bridge *br)
> fe_opts = xcalloc(n_fe_opts, sizeof *fe_opts);
> opts = fe_opts;
> OVSREC_FLOW_SAMPLE_COLLECTOR_SET_FOR_EACH(fe_cfg, idl) {
> -if (ovsrec_fscs_is_valid(fe_cfg, br)) {
> +if (ovsrec_fscs_is_valid_ipfix(fe_cfg, br)) {
> opts->collector_set_id = fe_cfg->id;
> sset_init(>targets);
> sset_add_array(>targets, fe_cfg->ipfix->targets,
> @@ -1667,6 +1670,47 @@ bridge_configure_ipfix(struct bridge *br)
> }
> }
>
> +/* Set psample configuration on 'br'. */
> +static void
> +bridge_configure_psample(struct bridge *br)
> +{
> +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> +const struct ovsrec_flow_sample_collector_set *fscs;
> +struct ofproto_psample_options *ps_options;
> +struct ovs_list options_list;
> +int ret;
> +
> +ovs_list_init(_list);
> +
> +OVSREC_FLOW_SAMPLE_COLLECTOR_SET_FOR_EACH(fscs, idl) {
> +if (!fscs->psample_group || fscs->n_psample_group != 1
> +|| fscs->bridge != br->cfg)
> +continue;
> +
> +ps_options = xzalloc(sizeof *ps_options);
> +ps_options->collector_set_id = fscs->id;
> +ps_options->group_id = *fscs->psample_group;
> +
> +ovs_list_insert(_list, _options->list_node);
> +}
> +
> +ret = ofproto_set_psample(br->ofproto, _list);
> +
> +if (ret == ENOTSUP) {
> +if (!ovs_list_is_empty(_list)) {
> +VLOG_WARN_RL(, "bridge %s: ignoring psample configuration: "
> + "not supported by this datapath", br->name);
> +}
> +} else if (ret) {
> +VLOG_ERR_RL(, "bridge %s: error configuring psample: %s",
> + br->name, ovs_strerror(ret));
> +}
> +
> +LIST_FOR_EACH_POP(ps_options, list_node, _list) {
> +free(ps_options);
> +}
> +}
> +
> static void
> port_configure_stp(const struct ofproto *ofproto, struct port *port,
> struct ofproto_port_stp_settings *port_s,
> diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema
> index e2d5e2e85..a7ad9bcaa 100644
> --- a/vswitchd/vswitch.ovsschema
> +++ b/vswitchd/vswitch.ovsschema
> @@ -1,6 +1,6 @@
> {"name": "Open_vSwitch",
> "version": "8.5.0",
Version should be increased.
> - "cksum": "4040946650 27557",
> + "cksum": "1366215760 27764",
> "tables": {
> "Open_vSwitch": {
>
Re: [ovs-dev] [RFC 03/11] ofproto: Add ofproto-dpif-psample.
On 5/10/24 12:06, Eelco Chaudron wrote:
> On 24 Apr 2024, at 21:53, Adrian Moreno wrote:
>
>> Add a new resource in ofproto-dpif and the corresponding API in
>> ofproto_provider.h to represent and change psample configuration.
>
> See comments below.
>
> //Eelco
>
>> Signed-off-by: Adrian Moreno
>> ---
>> ofproto/automake.mk| 2 +
>> ofproto/ofproto-dpif-psample.c | 167 +
>> ofproto/ofproto-dpif-psample.h | 31 ++
>> ofproto/ofproto-dpif.c | 33 +++
>> ofproto/ofproto-dpif.h | 1 +
>> ofproto/ofproto-provider.h | 9 ++
>> ofproto/ofproto.c | 10 ++
>> ofproto/ofproto.h | 8 ++
>> 8 files changed, 261 insertions(+)
>> create mode 100644 ofproto/ofproto-dpif-psample.c
>> create mode 100644 ofproto/ofproto-dpif-psample.h
>> +OVS_EXCLUDED(mutex)
>> +{
>> +struct ofproto_psample_options *options;
>> +struct psample_exporter_map_node *node;
>> +bool changed = false;
>> +
>> +ovs_mutex_lock();
>> +
>> +/* psample exporters do not hold any runtime memory so we do not need to
>> + * be extra careful at detecting which exporter changed and which did
>> + * not. As soon as we detect any change we can just recreate them all.
>> */
>
> Double space for new lines. Also not sure what multi line comment style we
> would
> like to enforce for new files. Coding style prefers /* and /* on new lines,
> but
> other options are allowed. Ilya?
Coding style allows for /* and */ on the same line as the comment start/end,
and I prefer it that way. Putting them on separate lines sometimes beneficial
for readability when the code is busy and tightly packed, so enforcing one
way or another is probably not a great thing.
Best 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.
*/
>>>> - u32 probability;/* Same value as
>>>> -* 'OVS_SAMPLE_ATTR_PROBABILITY'.
>>>> -*/
>>>> + u16 flags; /* Flags that modify the behavior of the
>>>> + * action. See SAMPLE_ARG_FLAG_*
>>>> + */
>>>> + u32 probability; /* Same value as
>>>> + * 'OVS_SAMPLE_ATTR_PROBABILITY'.
>>>> + */
>>>> + u32 group_id; /* Same value as
>>>> + * 'OVS_SAMPLE_ATTR_PSAMPLE_GROUP'.
>>>> + */
>>>> + u8 cookie_len; /* Length of psample cookie */
>>>> + char cookie[OVS_PSAMPLE_COOKIE_MAX_SIZE]; /* psample cookie data. */
>>>
>>> Would it make sense for the cookie also to be u8?
>>>
>>
>> Yes.
>>
>>>> };
>>>> #endif
>>>>
>>>> diff --git a/lib/odp-execute.c b/lib/odp-execute.c
>>>> index 081e4d432..d8ee93429 100644
>>>> --- a/lib/odp-execute.c
>>>> +++ b/lib/odp-execute.c
>>>> @@ -716,6 +716,9 @@ odp_execute_sample(void *dp, struct dp_packet *packet,
>>>> bool steal,
>>>> subactions = a;
>>>> break;
>>>>
>>>> +/* Ignored in userspace datapath. */
>>>> +case OVS_SAMPLE_ATTR_PSAMPLE_GROUP:
>>>> +case OVS_SAMPLE_ATTR_PSAMPLE_COOKIE:
Action that has these attributes should be marked as action that requires
datapath assistance. And we should never hit them here.
We should have a test case covering a packet sent from a controller or
execution with a debug_slow flag.
BTW, this code can be executed for kernel datapath as well if the packet
goes to userspace after upcall or comes from a controller, so the comment
is not correct.
>>>> case OVS_SAMPLE_ATTR_UNSPEC:
>>>> case __OVS_SAMPLE_ATTR_MAX:
>>>> default:
>>>> diff --git a/lib/odp-util.c b/lib/odp-util.c
>>>> index 21f34d955..611b5229e 100644
>>>> --- a/lib/odp-util.c
>>>> +++ b/lib/odp-util.c
>>>> @@ -227,12 +227,16 @@ format_odp_sample_action(struct ds *ds, const struct
>>>> nlattr *attr,
>>>> {
>>>> static const struct nl_policy ovs_sample_policy[] = {
>>>> [OVS_SAMPLE_ATTR_PROBABILITY] = { .type = NL_A_U32 },
>>>> -[OVS_SAMPLE_ATTR_ACTIONS] = { .type = NL_A_NESTED }
>>>> +[OVS_SAMPLE_ATTR_ACTIONS] = { .type = NL_A_NESTED,
>>>> + .optional = true },
>>>> +[OVS_SAMPLE_ATTR_PSAMPLE_GROUP] = { .type = NL_A_U32,
>>>> +.optional = true },
>>>> +[OVS_SAMPLE_ATTR_PSAMPLE_COOKIE] = { .type = NL_A_UNSPEC,
>>>> + .optional = true }
>>>> };
>>>> struct nlattr *a[ARRAY_SIZE(ovs_sample_policy)];
>>>> +const struct nlattr *nla;
>>>> double percentage;
>>>> -const struct nlattr *nla_acts;
>>>> -int len;
>>>>
>>>> ds_put_cstr(ds, "sample");
>>>>
>>>> @@ -244,13 +248,33 @@ format_odp_sample_action(struct ds *ds, const struct
>>>> nlattr *attr,
>>>> percentage = (100.0 *
>>>> nl_attr_get_u32(a[OVS_SAMPLE_ATTR_PROBABILITY])) /
>>>> UINT32_MAX;
>>>>
>>>> -ds_put_format(ds, "(sample=%.1f%%,", percentage);
>>>> +ds_put_format(ds, "(sample=%.1f%%", percentage);
>>>>
>>>> -ds_put_cstr(ds, "actions(");
>>>> -nla_acts = nl_attr_get(a[OVS_SAMPLE_ATTR_ACTIONS]);
>>>> -len = nl_attr_get_size(a[OVS_SAMPLE_ATTR_ACTIONS]);
>>>> -format_odp_actions(ds, nla_acts, len, portno_names);
>>>> -ds_put_format(ds, "))");
>>>> +nla = a[OVS_SAMPLE_ATTR_PSAMPLE_GROUP];
>>>> +if (nla) {
>>>> +ds_put_format(ds, ",group_id=%d", nl_attr_get_u32(nla));
>>>> +
>>>> +nla = a[OVS_SAMPLE_ATTR_PSAMPLE_COOKIE];
>>>> +
>>>> +if (nla) {
>>>> +size_t i;
>>>> +const uint8_t *c = nl_attr_get(nla);
>>>> +ds_put_cstr(ds, ",cookie=");
>>>> +for (i = 0; i < nl_attr_get_size(nla); i++) {
>>>> +ds_put_format(ds, "%02x", c[i]);
We print userdata in some other actions as dot-separated bytes.
maybe do the same here? Otherwise we should at least add 0x to
the front to signify that it is a hex data.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2] ovn-controller: Initialize bitmap to zero.
On 5/10/24 13:44, Ales Musil wrote:
>
>
> On Fri, May 10, 2024 at 1:34 PM Ilya Maximets <mailto:i.maxim...@ovn.org>> wrote:
>
> On 5/10/24 12:57, Ales Musil wrote:
> > The bitmap used in the update_ct_zones was uninitialized, and it could
> > contain any value besides 0. Use the bitmap_allocate() function instead,
> > to allocate the bitmap in heap rather than stack, the allocate makes
> sure
> > that the memory is properly zeroed.
> > This was caught by valgrind:
> >
> > Conditional jump or move depends on uninitialised value(s)
> > at 0x44074B: update_ct_zones (ovn-controller.c:812)
> > by 0x440DC9: en_ct_zones_run (ovn-controller.c:2579)
> > by 0x468BB7: engine_recompute (inc-proc-eng.c:415)
> > by 0x46954C: engine_compute (inc-proc-eng.c:454)
> > by 0x46954C: engine_run_node (inc-proc-eng.c:503)
> > by 0x46954C: engine_run (inc-proc-eng.c:528)
> > by 0x40AE9D: main (ovn-controller.c:5776)
> > Uninitialised value was created by a stack allocation
> > at 0x440313: update_ct_zones (ovn-controller.c:747)
> >
> > Fixes: f9cab11d5fab ("Allow explicit setting of the SNAT zone on a
> gateway router.")
> > Signed-off-by: Ales Musil mailto:amu...@redhat.com>>
> > ---
> > v2: Use bitmap_allocate() instead of array on stack.
> > ---
> > controller/ovn-controller.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index 453dc62fd..8ee2da2fd 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -732,7 +732,7 @@ update_ct_zones(const struct sset *local_lports,
> > const char *user;
> > struct sset all_users = SSET_INITIALIZER(_users);
> > struct simap req_snat_zones = SIMAP_INITIALIZER(_snat_zones);
> > - unsigned long unreq_snat_zones_map[BITMAP_N_LONGS(MAX_CT_ZONES)];
> > + unsigned long *unreq_snat_zones_map =
> bitmap_allocate(MAX_CT_ZONES);
> > struct simap unreq_snat_zones =
> SIMAP_INITIALIZER(_snat_zones);
> >
> > const char *local_lport;
> > @@ -843,6 +843,7 @@ update_ct_zones(const struct sset *local_lports,
> > simap_destroy(_snat_zones);
> > simap_destroy(_snat_zones);
> > sset_destroy(_users);
> > + free(unreq_snat_zones_map);
> > }
> >
> > static void
>
> Thanks, Ales. This change LGTM.
>
> Though I'm a bit surprised asan didn't catch this. Is this code not
> covered by tests?
>
>
> It should be covered indirectly by a lot of tests, but there are some that
> target
> this code specifically e.g. "resolve CT zone conflicts from ovsdb".
I'd expect asan to catch use of uninitialized stack memory. Weird.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC 02/11] ofproto_dpif: Check for psample support.
On 5/10/24 13:03, Adrian Moreno wrote:
>
>
> On 5/10/24 12:06 PM, Eelco Chaudron wrote:
>> On 24 Apr 2024, at 21:53, Adrian Moreno wrote:
>>
>>> Only kernel datapath supports psample so check that the datapath is not
>>> userspace and that it accepts the new attributes.
>>>
>>> Signed-off-by: Adrian Moreno
>>> ---
>>> ofproto/ofproto-dpif.c | 59 ++
>>> ofproto/ofproto-dpif.h | 6 -
>>> 2 files changed, 64 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>>> index 32d037be6..3cee2795a 100644
>>> --- a/ofproto/ofproto-dpif.c
>>> +++ b/ofproto/ofproto-dpif.c
>>> @@ -25,6 +25,7 @@
>>> #include "coverage.h"
>>> #include "cfm.h"
>>> #include "ct-dpif.h"
>>> +#include "dpif-netdev.h"
>>> #include "fail-open.h"
>>> #include "guarded-list.h"
>>> #include "hmapx.h"
>>> @@ -873,6 +874,12 @@ ovs_lb_output_action_supported(struct ofproto_dpif
>>> *ofproto)
>>> return ofproto->backer->rt_support.lb_output_action;
>>> }
>>>
>>> +bool
>>> +ovs_psample_supported(struct ofproto_dpif *ofproto)
>>
>> As mentioned in the previous patch, I do not like the psample terminology.
>> It refers to an implementation-specific name, we should come up with an
>> agnostic name, like ’system/local’, but there are probably better names.
>>
>>> +{
>>> +return ofproto->backer->rt_support.psample;
>>> +}
>>> +
>>> /* Tests whether 'backer''s datapath supports recirculation. Only newer
>>>* datapaths support OVS_KEY_ATTR_RECIRC_ID in keys. We need to disable
>>> some
>>>* features on older datapaths that don't support this feature.
>>> @@ -1440,6 +1447,14 @@ dpif_supports_ct_zero_snat(struct dpif_backer
>>> *backer)
>>> return supported;
>>> }
>>>
>>> +static bool check_psample(struct dpif_backer *backer);
>>> +
>>> +static bool
>>> +dpif_supports_psample(struct dpif_backer *backer)
>>> +{
>>> +return !dpif_is_netdev(backer->dpif) && check_psample(backer);
>>> +}
>>
>> This looks odd, we should not do any datapath specific checks here. We
>> should just call check_psample() on the netdev datapath, and it should fail.
>> If we ever add support for a similar feature we need to remove this code
>> anyway. Guess it also omits the log message for userspace.
>>
>
> I agree. Unfortunately, I didn't see any action verification code in the
> netdev
> datapath that (as the kernel does) would make the action installation fail.
> I'll
> double check but I think it's just not capable of detecting unsupported
> attributes on flow installation.
It's expected that userspace datapath is a reference implementation that
supports any actions, because it executes most of the actions via odp-execute
module that supposed to know all the actions.
This one however doesn't make sense for userspace datapath, so we have to
check beforehand. Adding extra parsing of actions may have significant
performance impact for upcalls.
However, ofproto-dpif module should not know any datapath details and must
not include dpif-netdev.h, the logic should be moved to dpif.c. For example,
see what we did for the explicit drop action in
dpif_may_support_explicit_drop_action().
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2] ovn-controller: Initialize bitmap to zero.
On 5/10/24 12:57, Ales Musil wrote:
> The bitmap used in the update_ct_zones was uninitialized, and it could
> contain any value besides 0. Use the bitmap_allocate() function instead,
> to allocate the bitmap in heap rather than stack, the allocate makes sure
> that the memory is properly zeroed.
> This was caught by valgrind:
>
> Conditional jump or move depends on uninitialised value(s)
> at 0x44074B: update_ct_zones (ovn-controller.c:812)
> by 0x440DC9: en_ct_zones_run (ovn-controller.c:2579)
> by 0x468BB7: engine_recompute (inc-proc-eng.c:415)
> by 0x46954C: engine_compute (inc-proc-eng.c:454)
> by 0x46954C: engine_run_node (inc-proc-eng.c:503)
> by 0x46954C: engine_run (inc-proc-eng.c:528)
> by 0x40AE9D: main (ovn-controller.c:5776)
> Uninitialised value was created by a stack allocation
> at 0x440313: update_ct_zones (ovn-controller.c:747)
>
> Fixes: f9cab11d5fab ("Allow explicit setting of the SNAT zone on a gateway
> router.")
> Signed-off-by: Ales Musil
> ---
> v2: Use bitmap_allocate() instead of array on stack.
> ---
> controller/ovn-controller.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 453dc62fd..8ee2da2fd 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -732,7 +732,7 @@ update_ct_zones(const struct sset *local_lports,
> const char *user;
> struct sset all_users = SSET_INITIALIZER(_users);
> struct simap req_snat_zones = SIMAP_INITIALIZER(_snat_zones);
> -unsigned long unreq_snat_zones_map[BITMAP_N_LONGS(MAX_CT_ZONES)];
> +unsigned long *unreq_snat_zones_map = bitmap_allocate(MAX_CT_ZONES);
> struct simap unreq_snat_zones = SIMAP_INITIALIZER(_snat_zones);
>
> const char *local_lport;
> @@ -843,6 +843,7 @@ update_ct_zones(const struct sset *local_lports,
> simap_destroy(_snat_zones);
> simap_destroy(_snat_zones);
> sset_destroy(_users);
> +free(unreq_snat_zones_map);
> }
>
> static void
Thanks, Ales. This change LGTM.
Though I'm a bit surprised asan didn't catch this. Is this code not
covered by tests?
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] ovn-controller: Initialize bitmap to zero.
On 5/10/24 10:44, Ales Musil wrote:
> The bitmap used in the update_ct_zones was uninitialized and it could
> contain any value besides 0, make sure we initialize it to 0 instead.
> This was caught by valgrind:
>
> Conditional jump or move depends on uninitialised value(s)
> at 0x44074B: update_ct_zones (ovn-controller.c:812)
> by 0x440DC9: en_ct_zones_run (ovn-controller.c:2579)
> by 0x468BB7: engine_recompute (inc-proc-eng.c:415)
> by 0x46954C: engine_compute (inc-proc-eng.c:454)
> by 0x46954C: engine_run_node (inc-proc-eng.c:503)
> by 0x46954C: engine_run (inc-proc-eng.c:528)
> by 0x40AE9D: main (ovn-controller.c:5776)
> Uninitialised value was created by a stack allocation
> at 0x440313: update_ct_zones (ovn-controller.c:747)
>
> Fixes: f9cab11d5fab ("Allow explicit setting of the SNAT zone on a gateway
> router.")
> Signed-off-by: Ales Musil
> ---
> controller/ovn-controller.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 453dc62fd..2388a1c15 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -732,7 +732,7 @@ update_ct_zones(const struct sset *local_lports,
> const char *user;
> struct sset all_users = SSET_INITIALIZER(_users);
> struct simap req_snat_zones = SIMAP_INITIALIZER(_snat_zones);
> -unsigned long unreq_snat_zones_map[BITMAP_N_LONGS(MAX_CT_ZONES)];
> +unsigned long unreq_snat_zones_map[BITMAP_N_LONGS(MAX_CT_ZONES)] = {0};
> struct simap unreq_snat_zones = SIMAP_INITIALIZER(_snat_zones);
>
> const char *local_lport;
Hi, Ales. Thanks for the fix!
The issue is caused by not using a proper bitmap API. Can we just use
the bitmap_allocate() here instead? With the amount of dynamic memory
allocations this function does with all the hash maps adding one more
allocation will not make any difference, but may protect from potential
issues of not using the API / providing a bad example. Allocating 8KB
on stack is not a particularly good thing anyway.
What do you think?
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH net] net: openvswitch: fix overwriting ct original tuple for ICMPv6
OVS_PACKET_CMD_EXECUTE has 3 main attributes:
- OVS_PACKET_ATTR_KEY - Packet metadata in a netlink format.
- OVS_PACKET_ATTR_PACKET - Binary packet content.
- OVS_PACKET_ATTR_ACTIONS - Actions to execute on the packet.
OVS_PACKET_ATTR_KEY is parsed first to populate sw_flow_key structure
with the metadata like conntrack state, input port, recirculation id,
etc. Then the packet itself gets parsed to populate the rest of the
keys from the packet headers.
Whenever the packet parsing code starts parsing the ICMPv6 header, it
first zeroes out fields in the key corresponding to Neighbor Discovery
information even if it is not an ND packet.
It is an 'ipv6.nd' field. However, the 'ipv6' is a union that shares
the space between 'nd' and 'ct_orig' that holds the original tuple
conntrack metadata parsed from the OVS_PACKET_ATTR_KEY.
ND packets should not normally have conntrack state, so it's fine to
share the space, but normal ICMPv6 Echo packets or maybe other types of
ICMPv6 can have the state attached and it should not be overwritten.
The issue results in all but the last 4 bytes of the destination
address being wiped from the original conntrack tuple leading to
incorrect packet matching and potentially executing wrong actions
in case this packet recirculates within the datapath or goes back
to userspace.
ND fields should not be accessed in non-ND packets, so not clearing
them should be fine. Executing memset() only for actual ND packets to
avoid the issue.
Initializing the whole thing before parsing is needed because ND packet
may not contain all the options.
The issue only affects the OVS_PACKET_CMD_EXECUTE path and doesn't
affect packets entering OVS datapath from network interfaces, because
in this case CT metadata is populated from skb after the packet is
already parsed.
Fixes: 9dd7f8907c37 ("openvswitch: Add original direction conntrack tuple to
sw_flow_key.")
Reported-by: Antonin Bas
Closes: https://github.com/openvswitch/ovs-issues/issues/327
Signed-off-by: Ilya Maximets
---
Note: I'm working on a selftest for this issue, but it requires some
ground work first to add support for OVS_PACKET_CMD_EXECUTE into
opnevswitch selftests as well as parsing of ct tuples. So it is going
to be a separate patch set.
net/openvswitch/flow.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
index 33b21a0c0548..8a848ce72e29 100644
--- a/net/openvswitch/flow.c
+++ b/net/openvswitch/flow.c
@@ -561,7 +561,6 @@ static int parse_icmpv6(struct sk_buff *skb, struct
sw_flow_key *key,
*/
key->tp.src = htons(icmp->icmp6_type);
key->tp.dst = htons(icmp->icmp6_code);
- memset(>ipv6.nd, 0, sizeof(key->ipv6.nd));
if (icmp->icmp6_code == 0 &&
(icmp->icmp6_type == NDISC_NEIGHBOUR_SOLICITATION ||
@@ -570,6 +569,8 @@ static int parse_icmpv6(struct sk_buff *skb, struct
sw_flow_key *key,
struct nd_msg *nd;
int offset;
+ memset(>ipv6.nd, 0, sizeof(key->ipv6.nd));
+
/* In order to process neighbor discovery options, we need the
* entire packet.
*/
--
2.44.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2/2] dpif-netdev: Drop invalid parsed packets
On 5/5/24 08:42, Roi Dayan via dev wrote: > From: Eli Britstein > > In case of a malformed packet, its parsing fails. Instead of continuing > and possible form a wrong flow, drop the packet. Hi, Eli and Roi. Thanks for the patch! But I don't think we can do that. There are few reasons why the packets should not be dropped in the datapath: 1. OVS is a switch, the only reasons why it should drop packets are: - User configuration - Inability to make a forwarding decision Both are not the case here. For example, if the packet has some issue in the IPv4 header, we should still forward it in case we just acting as an L2 learning switch. L2 learning switch doesn't need any information from IPv4 header to function. 2. Datapath should not make forwarding decisions including a decision to drop a packet. Datapath implementation should just execute actions that OpenFlow layers tell it to execute. OpenFlow layers should decide what to do. Also, inability to parse certain parts of the packet is not a parsing failure. The resulted flow structure should be valid regardless of the packet content. Fields that were not extracted remain zero and OpenFlow layers should correctly handle that and execute appropriate actions, i.e. properly match on all-zero values if they were used to make a forwarding decision. If the wrong flow can be installed in this situation - it's a bug somewhere in the flow translation logic that should be fixed. Hope this makes sense. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ofproto-dpif-trace: Fix access to an out-of-scope stack memory.
On 5/3/24 11:29, Eelco Chaudron wrote:
>
>
> On 3 May 2024, at 1:36, Ilya Maximets wrote:
>
>> While tracing NAT actions, pointer to the action may be stored in the
>> recirculation node for future reference. However, while translating
>> actions for the group bucket in xlate_group_bucket, the action list is
>> allocated temporarily on stack. So, in case the group translation
>> leads to NAT, the stack pointer can be stored in the recirculation node
>> and accessed later by the tracing mechanism when this stack memory is
>> long gone:
>>
>> ==396230==ERROR: AddressSanitizer: stack-use-after-return on address
>> 0x191844 at pc 0x64222a bp 0xa5da10 sp 0xa5da08
>> READ of size 1 at 0x191844 thread T0
>> 0 0x642229 in ofproto_trace_recirc_node ofproto/ofproto-dpif-trace.c:704:49
>> 1 0x642229 in ofproto_trace ofproto/ofproto-dpif-trace.c:867:9
>> 2 0x6434c1 in ofproto_unixctl_trace ofproto/ofproto-dpif-trace.c:489:9
>> 3 0xc1e491 in process_command lib/unixctl.c:310:13
>> 4 0xc1e491 in run_connection lib/unixctl.c:344:17
>> 5 0xc1e491 in unixctl_server_run lib/unixctl.c:395:21
>> 6 0x53eedf in main ovs/vswitchd/ovs-vswitchd.c:131:9
>> 7 0x2be087 in __libc_start_call_main
>> 8 0x2be14a in __libc_start_main@GLIBC_2.2.5
>> 9 0x42dee4 in _start (vswitchd/ovs-vswitchd+0x42dee4)
>>
>> Address 0x191844 is located in stack of thread T0 at offset 68 in frame
>> 0 0x6d391f in xlate_group_bucket ofproto/ofproto-dpif-xlate.c:4751
>>
>> This frame has 3 object(s):
>> [32, 1056) 'action_list_stub' (line 4760) <== Memory access at
>> offset 68 is inside
>> this variable
>> [1184, 1248) 'action_list' (line 4761)
>> [1280, 1344) 'action_set' (line 4762)
>>
>> SUMMARY: AddressSanitizer: stack-use-after-return
>>ofproto/ofproto-dpif-trace.c:704:49 in ofproto_trace_recirc_node
>>
>> Fix that by copying the action.
>>
>> Fixes: d072d2de011b ("ofproto-dpif-trace: Improve NAT tracing.")
>> Reported-by: Ales Musil
>> Signed-off-by: Ilya Maximets
>> ---
>
> Thanks for the patch, and adding a test case.
>
> Acked-by: Eelco Chaudron
>
Thanks, Adrian and Eelco!
Applied and backported down to 2.17.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3] hash, jhash: Fix unaligned access to the hash remainder.
On 5/3/24 13:44, Simon Horman wrote:
> On Fri, May 03, 2024 at 07:44:13AM +0200, Ales Musil wrote:
>> Partially revert db5a101931c5, this was to avoid warning, however we
>> shouldn't use pointer to "uint32_t" when the data are potentially
>> unaligned [0]. Use pointer to "uint8_t" right from the start, this
>> requires us to use ALIGNED_CAST for the get_unaligned_u32, which is
>> fine in that case, because the function uses
>> " __attribute__((__packed__))" struct to access the underlying "uint32_t".
>>
>> lib/hash.c:46:22: runtime error: load of misaligned address
>> 0x50700065 for type 'const uint32_t *' (aka 'const unsigned int *'),
>> which requires 4 byte alignment
>> 0x50700065: note: pointer points here
>> 73 62 2e 73 6f 63 6b 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> ^
>> 00 00 00 00 00 00 00 00 00
>> 0 0x6191cb in hash_bytes ovs/lib/hash.c:46:9
>> 1 0x69d064 in hash_string ovs/lib/hash.h:404:12
>> 2 0x69d064 in hash_name ovs/lib/shash.c:29:12
>> 3 0x69d064 in shash_find ovs/lib/shash.c:237:49
>> 4 0x69dada in shash_find_data ovs/lib/shash.c:251:31
>> 5 0x507987 in add_remote ovs/ovsdb/ovsdb-server.c:1382:15
>> 6 0x507987 in parse_options ovs/ovsdb/ovsdb-server.c:2659:13
>> 7 0x507987 in main ovs/ovsdb/ovsdb-server.c:751:5
>>
>> SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior lib/hash.c:46:22
>>
>> [0] https://github.com/llvm/llvm-project/issues/90848
>> Fixes: db5a101931c5 ("clang: Fix the alignment warning.")
>> Signed-off-by: Ales Musil
>> ---
>> v3: Do partial revert of db5a101931c5 instead of simple cast.
>
> Acked-by: Simon Horman
>
Thanks, Ales, Eelco and Simon!
Applied and backported down to 2.17.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] sparse: Add additional define for sparse on GCC >= 14.
On 5/3/24 13:42, Simon Horman wrote: > On Thu, May 02, 2024 at 01:13:39PM +0200, Ales Musil wrote: >> GCC 14 renamed one of the AVX512 defines to have only single >> underscore instead of two [0]. Add the single underscore define to >> keep compatibility with multiple GCC versions. >> >> [0] >> https://github.com/gcc-mirror/gcc/commit/aea8e4105553cd16799f2134d15420ccf182d732 >> Tested-by: Dumitru Ceara >> Signed-off-by: Ales Musil > > Acked-by: Simon Horman > Thanks, Ales, Dumitru and Simon! Applied to all branches down to 2.17 since it is needed for a successful build. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3] docs: Document manual cluster recovery procedure.
On 5/3/24 00:42, Ihar Hrachyshka wrote: > On Thu, May 2, 2024 at 5:52 PM Ilya Maximets <mailto:i.maxim...@ovn.org>> wrote: > > On 4/26/24 18:54, Ihar Hrachyshka wrote: > > Remove the notion of cluster/leave --force since it was never > > implemented. Instead of these instructions, document how a broken > > cluster can be re-initialized with the old database contents. > > > > Signed-off-by: Ihar Hrachyshka <mailto:ihrac...@redhat.com>> > > > > --- > > > > v1: initial version. > > v2: remove --force mentioned in ovsdb-server(1). > > v3: multiple language and markup changes suggested by Ilya. > > Thanks, Ihar! This version looks good to me in general. > I have a couple of minor nits below. If you agree, I can > fold those in while applying the change. > > > Feel free to. And thanks for your patience. Thanks, Ihar and Simon! I made the discussed changes and applied the patch. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [Patch] ovsdb-client: Add missing arg to help for 'dump'.
On 5/3/24 14:16, Simon Horman wrote: > On Wed, May 01, 2024 at 01:50:36PM +0100, Simon Horman wrote: >> On Wed, May 01, 2024 at 01:10:43PM +0200, Martin Kalcok wrote: >>> Help text for 'ovsdb-client dump' does not mention that it's capable >>> of dumping specific table's contents if user supplies table's name >>> as a third positional argument. >>> >>> Signed-off-by: Martin Kalcok >> >> Acked-by: Simon Horman > > Hi Martin, > > I am unsure what has happened, perhaps there is some silly error on my > side, but I am unable to see this patch in OVS patchwork [1]. > > If it is not there, would it be possible to resubmit? > > [1] https://patchwork.ozlabs.org/project/openvswitch/list/ For some reason it had 'Archived' flag set. I un-archived it: https://patchwork.ozlabs.org/project/openvswitch/patch/2024050043.357669-1-martin.kal...@canonical.com/ Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] utilities: Correct deletion reason in flow_reval_monitor.py.
On 5/2/24 12:11, 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.
>
> Fixes: 86b9e653ef22 ("revalidator: Add a USDT probe during flow deletion with
> purge reason.")
> Signed-off-by: Eelco Chaudron
> ---
> utilities/usdt-scripts/flow_reval_monitor.py | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/utilities/usdt-scripts/flow_reval_monitor.py
> b/utilities/usdt-scripts/flow_reval_monitor.py
> index 534ba8fa2..117f5bc27 100755
> --- a/utilities/usdt-scripts/flow_reval_monitor.py
> +++ b/utilities/usdt-scripts/flow_reval_monitor.py
> @@ -262,8 +262,8 @@ FdrReasonStrings = [
> "Kill all flows condition detected",
> "Mask too wide - need narrower match",
> "No matching ofproto rules",
> -"Too expensive to revalidate",
> "Purged with user action",
> +"Too expensive to revalidate",
> "Flow state inconsistent after updates",
> "Flow translation error",
> ]
Hi, Eelco. Thanks for the fix!
Did you consider changing this array to a dictionary? This may help
avoiding such issues in the future.
A few other general notes:
We may consider adding a comment to the C definition of the enum that
python version should be kept in sync.
Comments in the .c file and the descriptions here are fairly different
as well. That may be a little confusing.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] lib: Fix segfault for tunnel packet.
On 5/3/24 12:21, Amit Prakash Shukla wrote:
> Add NULL check to UDP, TCP and SCTP checksum functions. This patch
> also adds changes to populate inner_l3_ofs and inner_l4_ofs for the
> tunneled packets received from ports other than vport which are
> required by the protocol specific checksum function to parse the
> headers.
>
> Thread 22 "pmd-c07/id:15" received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 0x6e70dc00 (LWP 1061)]
> 0x13f61750 in packet_udp_complete_csum at lib/packets.c:2061
> 2061if (!udp->udp_csum) {
>
> 0x13f61750 in packet_udp_complete_csum at lib/packets.c:2061
> 0x13e5126c in dp_packet_ol_send_prepare at lib/dp-packet.c:638
> 0x13eb7d4c in netdev_push_header at lib/netdev.c:1035
> 0x13e69830 in push_tnl_action at lib/dpif-netdev.c:9067
> 0x13e69dac in dp_execute_cb at lib/dpif-netdev.c:9226
> 0x13ec72c4 in odp_execute_actions at lib/odp-execute.c:1008
> 0x13e6a7bc in dp_netdev_execute_actions at lib/dpif-netdev.c:9524
> 0x13e673d0 in packet_batch_per_flow_execute at lib/dpif-netdev.c:8271
> 0x13e69188 in dp_netdev_input__ at lib/dpif-netdev.c:8899
> 0x13e691f8 in dp_netdev_input at lib/dpif-netdev.c:8908
> 0x13e600e4 in dp_netdev_process_rxq_port at lib/dpif-netdev.c:5660
> 0x13e649a8 in pmd_thread_main at lib/dpif-netdev.c:7295
> 0x13f44b2c in ovsthread_wrapper at lib/ovs-thread.c:423
>
Hi, Amit. Thanks for the patch!
Nit: Please, send new versions as separate emails, do not send them
as a reply.
Could you, please, explain how exactly we end up in this situation?
The stack trace is fine, but it doesn't tell the whole story, e.g.
what other actions being executed on this packet?
We will likely need a test case for this scenario. We can help
writing one, if you can describe steps to reproduce the issue.
> CC: Mike Pattrick
> Fixes: 85bcbbed839a ("userspace: Enable tunnel tests with TSO.")
>
> Signed-off-by: Amit Prakash Shukla
> ---
>
> v2:
> - Added Fixes tag and updated commit message.
>
> lib/netdev.c | 7 +++
> lib/packets.c | 10 +-
> 2 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/lib/netdev.c b/lib/netdev.c
> index f2d921ed6..19bd87ef7 100644
> --- a/lib/netdev.c
> +++ b/lib/netdev.c
> @@ -1032,6 +1032,13 @@ netdev_push_header(const struct netdev *netdev,
> netdev_get_name(netdev));
> continue;
> }
> +if (packet->l3_ofs != UINT16_MAX) {
> +packet->inner_l3_ofs = packet->l3_ofs + data->header_len;
> +}
> +if (packet->l4_ofs != UINT16_MAX) {
> +packet->inner_l4_ofs = packet->l4_ofs + data->header_len;
> +}
> +
This is confusing. We should not end up here, unless it is a double
encapsulation scenario. And in this case the offsets should already
be initialized by the same code in netdev_tnl_push_udp_header().
> dp_packet_ol_send_prepare(packet, 0);
> }
> netdev->netdev_class->push_header(netdev, packet, data);
> diff --git a/lib/packets.c b/lib/packets.c
> index 5803d26f4..988c0e41f 100644
> --- a/lib/packets.c
> +++ b/lib/packets.c
> @@ -2011,6 +2011,10 @@ packet_tcp_complete_csum(struct dp_packet *p, bool
> inner)
> tcp_sz = dp_packet_l4_size(p);
> }
>
> +if (!tcp || !ip_hdr) {
> +return;
> +}
> +
> if (!inner && dp_packet_hwol_is_outer_ipv6(p)) {
> is_v4 = false;
> } else if (!inner && dp_packet_hwol_is_outer_ipv4(p)) {
> @@ -2058,7 +2062,7 @@ packet_udp_complete_csum(struct dp_packet *p, bool
> inner)
> }
>
> /* Skip csum calculation if the udp_csum is zero. */
> -if (!udp->udp_csum) {
> +if (!udp || !ip_hdr || !udp->udp_csum) {
> return;
> }
>
> @@ -2109,6 +2113,10 @@ packet_sctp_complete_csum(struct dp_packet *p, bool
> inner)
> tp_len = dp_packet_l4_size(p);
> }
>
> +if (!sh) {
> +return;
> +}
> +
> put_16aligned_be32(>sctp_csum, 0);
> csum = crc32c((void *) sh, tp_len);
> put_16aligned_be32(>sctp_csum, csum);
Please, don't add these checks. If these functions are called for
incorrect packet it's a bug in a code that calls them. We may add
assertions instead, e.g. ovs_assert(sh);
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] socket: Don't fail when AF_UNIX connect() returns EAGAIN.
uidset
> diff --git a/tests/automake.mk b/tests/automake.mk
> index 04f48f2d8..fc74cfa7d 100644
> --- a/tests/automake.mk
> +++ b/tests/automake.mk
> @@ -493,7 +493,8 @@ tests_ovstest_SOURCES = \
>
> if !WIN32
> tests_ovstest_SOURCES += \
> - tests/test-unix-socket.c
> + tests/test-unix-socket.c \
> + tests/test-unix-socket-listen-backlog.c
> endif
>
> if LINUX
> diff --git a/tests/library.at b/tests/library.at
> index d962e1b3f..fff92bc93 100644
> --- a/tests/library.at
> +++ b/tests/library.at
> @@ -219,6 +219,11 @@ AT_CHECK([mkdir $longname || exit 77])
> AT_CHECK([cd $longname && $PYTHON3 $abs_srcdir/test-unix-socket.py
> ../$longname/socket socket])
> AT_CLEANUP
>
> +AT_SETUP([unix socket, listen backlog])
> +AT_SKIP_IF([test "$IS_WIN32" = "yes"])
> +AT_CHECK([ovstest test-unix-socket-listen-backlog x x])
Maybe s/x/sock/ ?
> +AT_CLEANUP
> +
> AT_SETUP([ovs_assert])
> if test "$IS_WIN32" = "yes"; then
>exit_status=9
> diff --git a/tests/test-unix-socket-listen-backlog.c
> b/tests/test-unix-socket-listen-backlog.c
> new file mode 100644
> index 0..d1a73ba39
> --- /dev/null
> +++ b/tests/test-unix-socket-listen-backlog.c
> @@ -0,0 +1,73 @@
> +/*
> + * Copyright (c) 2010, 2012, 2014 Nicira, Inc.
> + * Copyright (c) 2024 Red Hat, Inc.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include
> +#undef NDEBUG
> +#include "socket-util.h"
> +#include
> +#include
> +#include
> +#include "ovstest.h"
> +#include "util.h"
> +
> +static void
> +test_unix_socket_listen_backlog_main(int argc, char *argv[])
> +{
> +const char *servername;
> +const char *clientname;
> +int serversock;
> +int clientsocks[LISTEN_BACKLOG + 1];
> +
> +set_program_name(argv[0]);
> +
> +if (argc != 3) {
> +ovs_fatal(0, "usage: %s SERVERSOCKET CLIENTSOCKET", argv[0]);
> +}
> +servername = argv[1];
> +clientname = argv[2];
> +
> +signal(SIGALRM, SIG_DFL);
> +alarm(5);
> +
> +/* Create a listening socket under name 'serversocket'. */
> +serversock = make_unix_socket(SOCK_STREAM, false, servername, NULL);
> +if (serversock < 0) {
> +ovs_fatal(-serversock, "%s: bind failed", servername);
> +}
> +if (listen(serversock, 1)) {
Should it be LISTEN_BACKLOG instead of 1 ?
Otherwise the clientsocks array size is kind of arbitrary, i.e.
it doesn't need to be LISTEN_BACKLOG + 1.
> +ovs_fatal(errno, "%s: listen failed", servername);
> +}
> +
> +/* Connect to 'clientname' (which should be the same file, perhaps under
> a
> + * different name). Connect enough times to overflow listen backlog. The
> + * last attempt should succeed, even though listen backlog is full and
> + * connect() returns EAGAIN (on Linux) or EINPROGRESS (on POSIX). */
Double spaces.
> +for (int i = 0; i < sizeof(clientsocks) / sizeof(clientsocks[0]); i++) {
ARRAY_SIZE macro from util.h.
> +clientsocks[i] = make_unix_socket(SOCK_STREAM, true, NULL,
> clientname);
> +if (clientsocks[i] < 0) {
> +ovs_fatal(-clientsocks[i], "%s: connect failed", clientname);
> +}
> +}
> +
> +for (int i = 0; i < sizeof(clientsocks) / sizeof(clientsocks[0]); i++) {
ARRAY_SIZE
> +close(clientsocks[i]);
Maybe instead of closing we should accept all them and then wait
for connection completion with check_connection_completion() on
all of them before closing?
> +}
> +close(serversock);
> +}
> +
> +OVSTEST_REGISTER("test-unix-socket-listen-backlog",
> + test_unix_socket_listen_backlog_main);
We may need a similar test for python.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] ofproto-dpif-trace: Fix access to an out-of-scope stack memory.
While tracing NAT actions, pointer to the action may be stored in the
recirculation node for future reference. However, while translating
actions for the group bucket in xlate_group_bucket, the action list is
allocated temporarily on stack. So, in case the group translation
leads to NAT, the stack pointer can be stored in the recirculation node
and accessed later by the tracing mechanism when this stack memory is
long gone:
==396230==ERROR: AddressSanitizer: stack-use-after-return on address
0x191844 at pc 0x64222a bp 0xa5da10 sp 0xa5da08
READ of size 1 at 0x191844 thread T0
0 0x642229 in ofproto_trace_recirc_node ofproto/ofproto-dpif-trace.c:704:49
1 0x642229 in ofproto_trace ofproto/ofproto-dpif-trace.c:867:9
2 0x6434c1 in ofproto_unixctl_trace ofproto/ofproto-dpif-trace.c:489:9
3 0xc1e491 in process_command lib/unixctl.c:310:13
4 0xc1e491 in run_connection lib/unixctl.c:344:17
5 0xc1e491 in unixctl_server_run lib/unixctl.c:395:21
6 0x53eedf in main ovs/vswitchd/ovs-vswitchd.c:131:9
7 0x2be087 in __libc_start_call_main
8 0x2be14a in __libc_start_main@GLIBC_2.2.5
9 0x42dee4 in _start (vswitchd/ovs-vswitchd+0x42dee4)
Address 0x191844 is located in stack of thread T0 at offset 68 in frame
0 0x6d391f in xlate_group_bucket ofproto/ofproto-dpif-xlate.c:4751
This frame has 3 object(s):
[32, 1056) 'action_list_stub' (line 4760) <== Memory access at
offset 68 is inside
this variable
[1184, 1248) 'action_list' (line 4761)
[1280, 1344) 'action_set' (line 4762)
SUMMARY: AddressSanitizer: stack-use-after-return
ofproto/ofproto-dpif-trace.c:704:49 in ofproto_trace_recirc_node
Fix that by copying the action.
Fixes: d072d2de011b ("ofproto-dpif-trace: Improve NAT tracing.")
Reported-by: Ales Musil
Signed-off-by: Ilya Maximets
---
ofproto/ofproto-dpif-trace.c | 3 ++-
ofproto/ofproto-dpif-trace.h | 2 +-
tests/ofproto-dpif.at| 22 ++
3 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c
index 87506aa78..e43d9f88c 100644
--- a/ofproto/ofproto-dpif-trace.c
+++ b/ofproto/ofproto-dpif-trace.c
@@ -102,7 +102,7 @@ oftrace_add_recirc_node(struct ovs_list *recirc_queue,
node->flow = *flow;
node->flow.recirc_id = recirc_id;
node->flow.ct_zone = zone;
-node->nat_act = ofn;
+node->nat_act = ofn ? xmemdup(ofn, sizeof *ofn) : NULL;
node->packet = packet ? dp_packet_clone(packet) : NULL;
return true;
@@ -113,6 +113,7 @@ oftrace_recirc_node_destroy(struct oftrace_recirc_node
*node)
{
if (node) {
recirc_free_id(node->recirc_id);
+free(node->nat_act);
dp_packet_delete(node->packet);
free(node);
}
diff --git a/ofproto/ofproto-dpif-trace.h b/ofproto/ofproto-dpif-trace.h
index f579a5ca4..f023b10cd 100644
--- a/ofproto/ofproto-dpif-trace.h
+++ b/ofproto/ofproto-dpif-trace.h
@@ -73,7 +73,7 @@ struct oftrace_recirc_node {
uint32_t recirc_id;
struct flow flow;
struct dp_packet *packet;
-const struct ofpact_nat *nat_act;
+struct ofpact_nat *nat_act;
};
/* A node within a next_ct_states list. */
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 3eaccb13a..0b23fd6c5 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -947,6 +947,28 @@ AT_CHECK([tail -1 stdout], [0],
OVS_VSWITCHD_STOP
AT_CLEANUP
+AT_SETUP([ofproto-dpif - group with ct and dnat recirculation in action list])
+OVS_VSWITCHD_START
+add_of_ports br0 1 10
+AT_CHECK([ovs-ofctl -O OpenFlow12 add-group br0 \
+'group_id=1234,type=all,bucket=ct(nat(dst=10.10.10.7:80),commit,table=2)'])
+AT_DATA([flows.txt], [dnl
+table=0 ip,ct_state=-trk actions=group:1234
+table=2 ip,ct_state=+trk actions=output:10
+])
+AT_CHECK([ovs-ofctl -O OpenFlow12 add-flows br0 flows.txt])
+AT_CHECK([ovs-appctl ofproto/trace br0 '
+ in_port=1,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:07,dl_type=0x0800,
+
nw_src=192.168.0.1,nw_dst=192.168.0.2,nw_proto=1,nw_tos=0,nw_ttl=128,nw_frag=no,
+ icmp_type=8,icmp_code=0
+'], [0], [stdout])
+AT_CHECK([grep 'Datapath actions' stdout], [0], [dnl
+Datapath actions: ct(commit,nat(dst=10.10.10.7:80)),recirc(0x1)
+Datapath actions: 10
+])
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
AT_SETUP([ofproto-dpif - group actions have no effect afterwards])
OVS_VSWITCHD_START
add_of_ports br0 1 10
--
2.44.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3] docs: Document manual cluster recovery procedure.
On 4/26/24 18:54, Ihar Hrachyshka wrote: > Remove the notion of cluster/leave --force since it was never > implemented. Instead of these instructions, document how a broken > cluster can be re-initialized with the old database contents. > > Signed-off-by: Ihar Hrachyshka > > --- > > v1: initial version. > v2: remove --force mentioned in ovsdb-server(1). > v3: multiple language and markup changes suggested by Ilya. Thanks, Ihar! This version looks good to me in general. I have a couple of minor nits below. If you agree, I can fold those in while applying the change. Let me know what you think. Best regards, Ilya Maximets. > > --- > Documentation/ref/ovsdb.7.rst | 44 --- > ovsdb/ovsdb-server.1.in | 3 +-- > 2 files changed, 37 insertions(+), 10 deletions(-) > > diff --git a/Documentation/ref/ovsdb.7.rst b/Documentation/ref/ovsdb.7.rst > index 46ed13e61..5766e64b9 100644 > --- a/Documentation/ref/ovsdb.7.rst > +++ b/Documentation/ref/ovsdb.7.rst > @@ -315,16 +315,11 @@ The above methods for adding and removing servers only > work for healthy > clusters, that is, for clusters with no more failures than their maximum > tolerance. For example, in a 3-server cluster, the failure of 2 servers > prevents servers joining or leaving the cluster (as well as database access). > + > To prevent data loss or inconsistency, the preferred solution to this problem > is to bring up enough of the failed servers to make the cluster healthy > again, > -then if necessary remove any remaining failed servers and add new ones. If > -this cannot be done, though, use ``ovs-appctl`` to invoke ``cluster/leave > ---force`` on a running server. This command forces the server to which it is > -directed to leave its cluster and form a new single-node cluster that > contains > -only itself. The data in the new cluster may be inconsistent with the former > -cluster: transactions not yet replicated to the server will be lost, and > -transactions not yet applied to the cluster may be committed. Afterward, any > -servers in its former cluster will regard the server to have failed. > +then if necessary remove any remaining failed servers and add new ones. If > this Nit: 2 spaces between sentences. > +is not an option, see the next section for `Manual cluster recovery`_. > > Once a server leaves a cluster, it may never rejoin it. Instead, create a > new > server and join it to the cluster. > @@ -362,6 +357,39 @@ Clustered OVSDB does not support the OVSDB "ephemeral > columns" feature. > ones when they work with schemas for clustered databases. Future versions of > OVSDB might add support for this feature. > > +Manual cluster recovery > +~~~ > + > +.. important:: Nit: An empty line here would be nice to be consistent at least within this document. > + The procedure below will result in ``cid`` and ``sid`` change. A *new* Nit: 2 spaces between sentences. > + cluster will be initialized. > + > +To recover a clustered database after a failure: > + > +1. Stop *all* old cluster ``ovsdb-server`` instances before proceeding. > + > +2. Pick one of the old members which will serve as a bootstrap member of the > + to-be-recovered cluster. > + > +3. Convert its database file to the standalone format using ``ovsdb-tool > + cluster-to-standalone``. > + > +4. Backup the standalone database file. > + > +5. Create a new single-node cluster with ``ovsdb-tool create-cluster`` > + using the previously saved standalone database file, then start > + ``ovsdb-server``. > + > +Once the single-node cluster is up and running and serves the restored data, > +new members should be created and join the new cluster, as usual > (``ovsdb-tool > +join-cluster``). I'm having hard time reading 'new members should be created and join' as my brain wants to relate 'should be' to both 'created' and 'join' and 'should be join' is not a correct construct. How about: "new members should be created and added to the cluster, as usual, with ``ovsdb-tool join-cluster``." ? Also, should it be a step 6 ? > + > +.. note:: > + > + The data in the new cluster may be inconsistent with the former cluster: > + transactions not yet replicated to the server chosen in step 2 will be > lost, > + and transactions not yet applied to the cluster may be committed. > + > Upgrading from version 2.14 and earlier to 2.15 and later > ~ > > diff --git a/ovsdb/ovsdb-server.1.in b/ovsdb/ovsdb-server.1.in > index 9fabf2d67..23b8e6e9c 100644 > --- a/ovsdb/ovsdb-server.1.in > +++ b/ovsdb/ovsdb-server.1.in > @@ -461,8 +4
Re: [ovs-dev] [PATCH v9 5/6] vswitchd: Add JSON output for 'list-commands' command.
On 4/12/24 09:26, jm...@redhat.com wrote:
> From: Jakob Meng
>
> The 'list-commands' command now supports machine-readable JSON output
> in addition to the plain-text output for humans.
>
> Reported-at: https://bugzilla.redhat.com/1824861
> Signed-off-by: Jakob Meng
> ---
> NEWS | 1 +
> lib/unixctl.c | 46 ++-
> tests/ovs-vswitchd.at | 11 +++
> 3 files changed, 44 insertions(+), 14 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 957351acb..af83623b3 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -13,6 +13,7 @@ Post-v3.3.0
> * Added new option [--pretty] to print JSON output in a readable
> fashion.
> E.g. members of objects and elements of arrays are printed one per
> line,
> with indentation.
> + * 'list-commands' now supports output in JSON format.
> - Python:
> * Added support for choosing the output format, e.g. 'json' or 'text'.
> * Added new option [--pretty] to print JSON output in a readable
> fashion.
> diff --git a/lib/unixctl.c b/lib/unixctl.c
> index 67cf26418..ca54884d0 100644
> --- a/lib/unixctl.c
> +++ b/lib/unixctl.c
> @@ -68,24 +68,42 @@ static void
> unixctl_list_commands(struct unixctl_conn *conn, int argc OVS_UNUSED,
>const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
> {
> -struct ds ds = DS_EMPTY_INITIALIZER;
> -const struct shash_node **nodes = shash_sort();
> -size_t i;
> +if (unixctl_command_get_output_format(conn) == OVS_OUTPUT_FMT_JSON) {
> +struct json *json_commands = json_array_create_empty();
> +const struct shash_node *node;
>
> -ds_put_cstr(, "The available commands are:\n");
> +SHASH_FOR_EACH (node, ) {
> +const struct unixctl_command *command = node->data;
>
> -for (i = 0; i < shash_count(); i++) {
> -const struct shash_node *node = nodes[i];
> -const struct unixctl_command *command = node->data;
> -
> -if (command->usage) {
> -ds_put_format(, " %-23s %s\n", node->name, command->usage);
> +if (command->usage) {
> +struct json *json_command = json_object_create();
An empty line.
> +json_object_put_string(json_command, "name", node->name);
> +json_object_put_string(json_command, "usage",
> command->usage);
> +json_array_add(json_commands, json_command);
Can it be just a { "name": "usage", "name1": "usage1", ... } map
instead of array of objects?
> +}
> }
> -}
> -free(nodes);
> +unixctl_command_reply_json(conn, json_commands);
> +} else {
> +struct ds ds = DS_EMPTY_INITIALIZER;
> +const struct shash_node **nodes = shash_sort();
> +size_t i;
>
> -unixctl_command_reply(conn, ds_cstr());
> -ds_destroy();
> +ds_put_cstr(, "The available commands are:\n");
> +
> +for (i = 0; i < shash_count(); ++i) {
> +const struct shash_node *node = nodes[i];
> +const struct unixctl_command *command = node->data;
> +
> +if (command->usage) {
> +ds_put_format(, " %-23s %s\n", node->name,
> + command->usage);
> +}
> +}
> +free(nodes);
> +
> +unixctl_command_reply(conn, ds_cstr());
> +ds_destroy();
> +}
> }
>
> static void
> diff --git a/tests/ovs-vswitchd.at b/tests/ovs-vswitchd.at
> index 5683896ef..dcda71d04 100644
> --- a/tests/ovs-vswitchd.at
> +++ b/tests/ovs-vswitchd.at
> @@ -281,3 +281,14 @@ AT_CHECK_UNQUOTED([ovs-appctl --format json --pretty
> version], [0], [dnl
>"reply-format": "plain"}])
>
> AT_CLEANUP
> +
> +AT_SETUP([ovs-vswitchd list-commands])
> +OVS_VSWITCHD_START
> +
> +AT_CHECK([ovs-appctl list-commands], [0], [ignore])
> +AT_CHECK([ovs-appctl --format json list-commands], [0], [stdout])
> +AT_CHECK([wc -l stdout], [0], [0 stdout
> +])
This line looks strange. What does it do?
> +AT_CHECK([grep -E '^\[[\{"name":.*\}\]]$' stdout], [0], [ignore])
> +
> +AT_CLEANUP
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v9 4/6] python: Add option '--pretty' for pretty-printing JSON output.
On 4/12/24 09:26, jm...@redhat.com wrote: > From: Jakob Meng > > With the '--pretty' option, appctl.py will now print JSON output in a > more readable fashion, i.e. with additional line breaks, spaces and > sorted dictionary keys. The pretty-printed output from appctl.py is not > strictly the same as with ovs-appctl because of both use different > pretty-printing implementations. > > Signed-off-by: Jakob Meng > --- > NEWS | 3 +++ > python/ovs/unixctl/client.py | 4 ++-- > tests/appctl.py | 16 ++-- > tests/unixctl-py.at | 5 + > 4 files changed, 24 insertions(+), 4 deletions(-) > > diff --git a/NEWS b/NEWS > index 52cadbe0d..957351acb 100644 > --- a/NEWS > +++ b/NEWS > @@ -15,6 +15,9 @@ Post-v3.3.0 > with indentation. > - Python: > * Added support for choosing the output format, e.g. 'json' or 'text'. > + * Added new option [--pretty] to print JSON output in a readable > fashion. > + E.g. members of objects and elements of arrays are printed one per > line, > + with indentation. The option was added to the test-only code. This shouldn't be in the news. If we move the formatting responsibility out of the UnixctlClient to the user, this patch may not be needed at all. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v9 3/6] appctl: Add option '--pretty' for pretty-printing JSON output.
On 4/12/24 09:26, jm...@redhat.com wrote:
> From: Jakob Meng
>
> With the '--pretty' option, ovs-appctl will now print JSON output in a
> more readable fashion, i.e. with additional line breaks, spaces and
> sorted dictionary keys.
>
> Signed-off-by: Jakob Meng
> ---
> NEWS | 3 +++
> lib/unixctl.c | 6 +++---
> lib/unixctl.h | 2 +-
> tests/ovs-vswitchd.at | 5 +
> utilities/ovs-appctl.c | 32
> 5 files changed, 40 insertions(+), 8 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index f18238159..52cadbe0d 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -10,6 +10,9 @@ Post-v3.3.0
> - ovs-appctl:
> * Added new option [-f|--format] to choose the output format, e.g.
> 'json'
> or 'text' (by default).
> + * Added new option [--pretty] to print JSON output in a readable
> fashion.
> + E.g. members of objects and elements of arrays are printed one per
> line,
> + with indentation.
> - Python:
> * Added support for choosing the output format, e.g. 'json' or 'text'.
>
> diff --git a/lib/unixctl.c b/lib/unixctl.c
> index 18638d86a..67cf26418 100644
> --- a/lib/unixctl.c
> +++ b/lib/unixctl.c
> @@ -572,8 +572,8 @@ unixctl_client_create(const char *path, struct jsonrpc
> **client)
> * '*err' if not NULL. */
> int
> unixctl_client_transact(struct jsonrpc *client, const char *command, int
> argc,
> -char *argv[], enum ovs_output_fmt fmt, char **result,
> -char **err)
> +char *argv[], enum ovs_output_fmt fmt,
> +unsigned int fmt_flags, char **result, char **err)
As I mentioned in the review for patch #1, it may be better to handle
for matting in the ovs-appctl instead and keep unixctl module unaware
of it.
> {
> struct jsonrpc_msg *request, *reply;
> struct json **json_args, *params, *output_src;
> @@ -604,7 +604,7 @@ unixctl_client_transact(struct jsonrpc *client, const
> char *command, int argc,
> if (fmt == OVS_OUTPUT_FMT_TEXT && output_src->type == JSON_STRING) {
> *output_dst = xstrdup(json_string(output_src));
> } else if (fmt == OVS_OUTPUT_FMT_JSON) {
> -*output_dst = json_to_string(output_src, 0);
> +*output_dst = json_to_string(output_src, fmt_flags);
> } else {
> VLOG_WARN("%s: unexpected %s type in JSON rpc reply: %s",
>jsonrpc_get_name(client),
> diff --git a/lib/unixctl.h b/lib/unixctl.h
> index b30bcf092..5a08a1bd1 100644
> --- a/lib/unixctl.h
> +++ b/lib/unixctl.h
> @@ -39,7 +39,7 @@ int unixctl_client_create(const char *path, struct jsonrpc
> **client);
> int unixctl_client_transact(struct jsonrpc *client,
> const char *command,
> int argc, char *argv[],
> -enum ovs_output_fmt fmt,
> +enum ovs_output_fmt fmt, unsigned int fmt_flags,
> char **result, char **error);
>
> /* Command registration. */
> diff --git a/tests/ovs-vswitchd.at b/tests/ovs-vswitchd.at
> index 32ca2c6e7..5683896ef 100644
> --- a/tests/ovs-vswitchd.at
> +++ b/tests/ovs-vswitchd.at
> @@ -275,4 +275,9 @@ ovs_version=$(ovs-appctl version)
> AT_CHECK_UNQUOTED([ovs-appctl --format json version], [0], [dnl
> {"reply-format":"plain","reply":"$ovs_version\n"}])
>
> +AT_CHECK_UNQUOTED([ovs-appctl --format json --pretty version], [0], [dnl
> +{
> + "reply": "$ovs_version\n",
> + "reply-format": "plain"}])
> +
> AT_CLEANUP
> diff --git a/utilities/ovs-appctl.c b/utilities/ovs-appctl.c
> index 4d4503597..29a0de2ee 100644
> --- a/utilities/ovs-appctl.c
> +++ b/utilities/ovs-appctl.c
> @@ -26,6 +26,7 @@
> #include "daemon.h"
> #include "dirs.h"
> #include "openvswitch/dynamic-string.h"
> +#include "openvswitch/json.h"
> #include "jsonrpc.h"
> #include "process.h"
> #include "timeval.h"
> @@ -39,6 +40,7 @@ static void usage(void);
> /* Parsed command line args. */
> struct cmdl_args {
> enum ovs_output_fmt format;
> +unsigned int format_flags;
> char *target;
> };
>
> @@ -74,8 +76,8 @@ main(int argc, char *argv[])
> if (!svec_is_empty(_argv)) {
> error = unixctl_client_transact(client, "set-options",
> opt_argv.n, opt_argv.names,
> -args->format, _result,
> -_error);
> +args->format, 0,
> +_result, _error);
>
> if (error) {
> ovs_fatal(error, "%s: transaction error", args->target);
> @@ -98,7 +100,9 @@ main(int argc, char *argv[])
> cmd_argc = argc - optind;
> cmd_argv = cmd_argc ? argv + optind : NULL;
> error = unixctl_client_transact(client, cmd, cmd_argc, cmd_argv,
> -
Re: [ovs-dev] [PATCH v9 2/6] python: Add global option for JSON output to Python tools.
On 4/12/24 09:26, jm...@redhat.com wrote: > From: Jakob Meng > > This patch introduces support for different output formats to the > Python code, as did the previous commit for ovs-xxx tools like > 'ovs-appctl --format json dpif/show'. > In particular, tests/appctl.py gains a global option '-f,--format' > which allows users to request JSON instead of plain-text for humans. > > Reported-at: https://bugzilla.redhat.com/1824861 > Signed-off-by: Jakob Meng > --- > NEWS | 2 ++ > python/ovs/unixctl/client.py | 14 --- > python/ovs/unixctl/server.py | 45 +--- > python/ovs/util.py | 8 +++ > tests/appctl.py | 19 ++- > tests/unixctl-py.at | 3 +++ > 6 files changed, 84 insertions(+), 7 deletions(-) > > diff --git a/NEWS b/NEWS > index 03ef6486b..f18238159 100644 > --- a/NEWS > +++ b/NEWS > @@ -10,6 +10,8 @@ Post-v3.3.0 > - ovs-appctl: > * Added new option [-f|--format] to choose the output format, e.g. > 'json' > or 'text' (by default). > + - Python: > + * Added support for choosing the output format, e.g. 'json' or 'text'. This is misleading. Unixctl should be mentioned. The rest of the patch has similar issues to the C implementation, will not repeat my comments here. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v9 1/6] Add global option for JSON output to ovs-appctl.
On 4/12/24 09:26, jm...@redhat.com wrote:
> From: Jakob Meng
>
> For monitoring systems such as Prometheus it would be beneficial if
> OVS would expose statistics in a machine-readable format.
>
> This patch introduces support for different output formats to ovs-xxx
> tools, in particular ovs-appctl.
What other tools you plan to add support to?
Database tools already have the machine-readable output from the database.
appctl should generelly be used instead of ovs-dpctl. What else?
I'd suggest to not try to make this generic.
> The latter gains a global option
> '-f,--format' which changes it to print a JSON document instead of
> plain-text for humans. For example, a later patch implements support
> for 'ovs-appctl --format json dpif/show'. By default, the output format
> is plain-text as before.
>
> A new 'set-options' command has been added to lib/unixctl.c which
> allows to change the output format of the commands executed afterwards
> on the same socket connection. It is supposed to be run by ovs-xxx
It's specific for appctl unility, nothing else is using unixctl.
> tools transparently for the user when a specific output format has been
> requested.
> For example, when a user calls 'ovs-appctl --format json dpif/show',
> then ovs-appctl will call 'set-options' to set the output format as
> requested by the user and afterwards it will call the actual command
> 'dpif/show'.
> This ovs-appctl behaviour has been implemented in a backward compatible
> way. One can use an updated client (ovs-appctl) with an old server
> (ovs-vswitchd) and vice versa. Of course, JSON output only works when
> both sides have been updated.
>
> Two access functions unixctl_command_{get,set}_output_format() and
> two unixctl_command_reply_{,error_}json functions have been added to
> lib/unixctl.h:
> unixctl_command_get_output_format() is supposed to be used in commands
> like 'dpif/show' to query the requested output format. When JSON output
> has been selected, both unixctl_command_reply_{,error_}json() functions
> can be used to return JSON objects to the client (ovs-appctl) instead
> of plain-text with the unixctl_command_reply{,_error}() functions.
>
> When JSON has been requested but a command has not implemented JSON
> output the plain-text output will be wrapped in a provisional JSON
> document with the following structure:
>
> {"reply-format":"plain","reply":"$PLAIN_TEXT_HERE"}
>
> Thus commands which have been executed successfully will not fail when
> they try to render the output at a later stage.
>
> In popular tools like kubectl the option for output control is usually
> called '-o|--output' instead of '-f,--format'. But ovs-appctl already
> has an short option '-o' which prints the available ovs-appctl options
> ('--option'). The now choosen name also better alignes with
> ovsdb-client where '-f,--format' controls output formatting.
>
> Reported-at: https://bugzilla.redhat.com/1824861
> Signed-off-by: Jakob Meng
> ---
> NEWS | 3 +
> lib/command-line.c | 36 ++
> lib/command-line.h | 10 +++
> lib/unixctl.c | 158 -
> lib/unixctl.h | 11 +++
> tests/ovs-vswitchd.at | 11 +++
> utilities/ovs-appctl.c | 98 +
> 7 files changed, 277 insertions(+), 50 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index b92cec532..03ef6486b 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -7,6 +7,9 @@ Post-v3.3.0
> - The primary development branch has been renamed from 'master' to 'main'.
> The OVS tree remains hosted on GitHub.
> https://github.com/openvswitch/ovs.git
> + - ovs-appctl:
'o' goes before 't' or 'u', so I'd put this entry to the top.
> + * Added new option [-f|--format] to choose the output format, e.g.
> 'json'
> + or 'text' (by default).
A man page update is missing:
Documentation/ref/ovs-appctl.8.rst
>
>
> v3.3.0 - 16 Feb 2024
> diff --git a/lib/command-line.c b/lib/command-line.c
> index 967f4f5d5..775e0430a 100644
> --- a/lib/command-line.c
> +++ b/lib/command-line.c
These changes should be moved to unixctl.[ch].
> @@ -24,6 +24,7 @@
> #include "ovs-thread.h"
> #include "util.h"
> #include "openvswitch/vlog.h"
> +#include "openvswitch/json.h"
>
> VLOG_DEFINE_THIS_MODULE(command_line);
>
> @@ -53,6 +54,41 @@ ovs_cmdl_long_options_to_short_options(const struct option
> options[])
> return xstrdup(short_options);
> }
>
> +const char *
> +ovs_output_fmt_to_string(enum ovs_output_fmt fmt)
> +{
> +switch (fmt) {
> +case OVS_OUTPUT_FMT_TEXT:
> +return "text";
> +
> +case OVS_OUTPUT_FMT_JSON:
> +return "json";
> +
> +default:
> +return NULL;
> +}
> +}
> +
> +struct json *
> +ovs_output_fmt_to_json(enum ovs_output_fmt fmt)
> +{
> +const char *string = ovs_output_fmt_to_string(fmt);
> +return string ? json_string_create(string) : NULL;
> +}
> +
> +bool
> +ovs_output_fmt_from_string(const
Re: [ovs-dev] [PATCH v2] hash, jhash: Fix unaligned access to the hash remainder.
On 5/2/24 15:28, Ales Musil wrote:
> The pointer was passed to memcpy as uin32_t *, however the hash bytes
> might be unaligned at that point. Case it to uint8_t * instead
'Case' ?
> which has only single byte alignment requirement. This seems to be
> a false positive reported by clang [0].
After thinking some more, it's not actually a false positive per se.
According to the C spec we're not actually allowed to have misaligned
pointers even if we're not reading/writing through them.
So, technically, the initial cast to uint32_t pointer is no correct.
I don't think we can fully avoid such casts without loosing type checking,
but I think we need to revert changes to hash functions made in
commit db5a101931c5 ("clang: Fix the alignment warning.").
i.e. we should go back to using uint8_t pointer and cast it on the
get_unaligned_u32() call with ALIGNED_CAST. We will still have a
misaligned pointer, but it will be immediately cast back, so should
cause less issues.
Note: all arithmetic should be done on the uint8_t pointer, not a
misaligned uin32_t one to avoid potential other UB conditions.
Best regards, Ilya Maximets.
>
> lib/hash.c:46:22: runtime error: load of misaligned address
> 0x50700065 for type 'const uint32_t *' (aka 'const unsigned int *'),
> which requires 4 byte alignment
> 0x50700065: note: pointer points here
> 73 62 2e 73 6f 63 6b 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> ^
> 00 00 00 00 00 00 00 00 00
> 0 0x6191cb in hash_bytes ovs/lib/hash.c:46:9
> 1 0x69d064 in hash_string ovs/lib/hash.h:404:12
> 2 0x69d064 in hash_name ovs/lib/shash.c:29:12
> 3 0x69d064 in shash_find ovs/lib/shash.c:237:49
> 4 0x69dada in shash_find_data ovs/lib/shash.c:251:31
> 5 0x507987 in add_remote ovs/ovsdb/ovsdb-server.c:1382:15
> 6 0x507987 in parse_options ovs/ovsdb/ovsdb-server.c:2659:13
> 7 0x507987 in main ovs/ovsdb/ovsdb-server.c:751:5
>
> SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior lib/hash.c:46:22
>
> [0] https://github.com/llvm/llvm-project/issues/90848
> Signed-off-by: Ales Musil
> ---
> lib/hash.c | 2 +-
> lib/jhash.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/lib/hash.c b/lib/hash.c
> index c722f3c3c..986fa6643 100644
> --- a/lib/hash.c
> +++ b/lib/hash.c
> @@ -43,7 +43,7 @@ hash_bytes(const void *p_, size_t n, uint32_t basis)
> if (n) {
> uint32_t tmp = 0;
>
> -memcpy(, p, n);
> +memcpy(, (const uint8_t *) p, n);
> hash = hash_add(hash, tmp);
> }
>
> diff --git a/lib/jhash.c b/lib/jhash.c
> index c59b51b61..0a0628589 100644
> --- a/lib/jhash.c
> +++ b/lib/jhash.c
> @@ -114,7 +114,7 @@ jhash_bytes(const void *p_, size_t n, uint32_t basis)
> uint32_t tmp[3];
>
> tmp[0] = tmp[1] = tmp[2] = 0;
> -memcpy(tmp, p, n);
> +memcpy(tmp, (const uint8_t *) p, n);
> a += tmp[0];
> b += tmp[1];
> c += tmp[2];
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] netdev-tc-offloads: Don't offload header modification on ip fragments.
ally we would use compose-packet for this, but it can't generate such packets today, so it's fine to just plain-code the packet. But, please, add a description on what this packet is. It's very inconvenient to copy-paste this into some packet decoder. > +done > + > +OVS_WAIT_UNTIL([test $(grep -c -E \ > + "0x: *36b1 *ee7c *0102 *36b1 *ee7c *0103 *0800 *4500" p1.out) -eq 5]) > +OVS_WAIT_UNTIL([test $(grep -c -E \ > + "0x0010: *0026 *0001 *2000 *4011 *43c2 *0b01 *0101 *0a01" p1.out) -eq 5]) > +OVS_WAIT_UNTIL([test $(grep -c -E \ > + "0x0020: *0102 *0bc4 *0884 *0026 *e864 *0102 *0304 *0506" p1.out) -eq 5]) Maybe it's better to use pcap output on tcpdump and compare with ovs-pcap? Maybe also split the hex in parts like we do in tunnel tests here: https://github.com/openvswitch/ovs/blob/bd8e9f48f180800292c10e12f26824833f18506a/tests/tunnel-push-pop.at#L834 Same for the IPv6 check below. > + > +dnl Repeat similar test with IPv6. > +for i in 1 2 3 4 5; do > + NS_CHECK_EXEC([at_ns0], [$PYTHON3 $srcdir/sendpkt.py p0 \ > +36 b1 ee 7c 01 02 36 b1 ee 7c 01 03 86 dd 60 00 00 00 00 18 2c 40 fc 00 \ > +00 00 00 00 00 00 00 00 00 00 00 00 00 01 fc 00 00 00 00 00 00 00 00 00 \ > +00 00 00 00 00 02 11 00 00 01 23 16 ab 36 0b c4 08 84 00 26 07 65 01 02 \ > +03 04 05 06 07 08 > /dev/null]) > +done > + > +OVS_WAIT_UNTIL([test $(grep -c -E \ > + "0x: *36b1 *ee7c *0102 *36b1 *ee7c *0103 *86dd *6000" p1.out) -eq 5]) > +OVS_WAIT_UNTIL([test $(grep -c -E \ > + "0x0010: * *0018 *2c40 *fc00 * * * *" p1.out) -eq 5]) > +OVS_WAIT_UNTIL([test $(grep -c -E \ > + "0x0020: * * *0100 *fc00 * * * *" p1.out) -eq 5]) > +OVS_WAIT_UNTIL([test $(grep -c -E \ > + "0x0030: * * *0002 *1100 *0001 *2316 *ab36 *0bc4" p1.out) -eq 5]) > +OVS_WAIT_UNTIL([test $(grep -c -E \ > + "0x0040: *0884 *0026 *0666 *0102 *0304 *0506 *0708" p1.out) -eq 5]) > + > +OVS_TRAFFIC_VSWITCHD_STOP > +AT_CLEANUP Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] hash, jhash: Fix unaligned access to the hash remainder.
On 5/2/24 12:22, Ales Musil wrote:
> The has was passed to memcpy as uin32_t *, however the hash bytes
'The has was passed' ? :)
> might be unaligned at that point. Case it to uint8_t * instead
> which has only single byte alignment requirement.
>
> lib/hash.c:46:22: runtime error: load of misaligned address 0x50700065
> for type 'const uint32_t *' (aka 'const unsigned int *'), which requires 4
> byte alignment
> 0x50700065: note: pointer points here
> 73 62 2e 73 6f 63 6b 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00
> ^
Please, wrap these lines.
> #0 0x6191cb in hash_bytes /workspace/ovn/ovs/lib/hash.c:46:9
> #1 0x69d064 in hash_string /workspace/ovn/ovs/lib/hash.h:404:12
> #2 0x69d064 in hash_name /workspace/ovn/ovs/lib/shash.c:29:12
> #3 0x69d064 in shash_find /workspace/ovn/ovs/lib/shash.c:237:49
> #4 0x69dada in shash_find_data /workspace/ovn/ovs/lib/shash.c:251:31
> #5 0x507987 in add_remote /workspace/ovn/ovs/ovsdb/ovsdb-server.c:1382:15
> #6 0x507987 in parse_options
> /workspace/ovn/ovs/ovsdb/ovsdb-server.c:2659:13
> #7 0x507987 in main /workspace/ovn/ovs/ovsdb/ovsdb-server.c:751:5
> #8 0x7f47e3997087 in __libc_start_call_main (/lib64/libc.so.6+0x2a087)
> (BuildId: b098f1c75a76548bb230d8f551eae07a2aeccf06)
> #9 0x7f47e399714a in __libc_start_main@GLIBC_2.2.5
> (/lib64/libc.so.6+0x2a14a) (BuildId: b098f1c75a76548bb230d8f551eae07a2aeccf06)
> #10 0x42de64 in _start (/workspace/ovn/ovs/ovsdb/ovsdb-server+0x42de64)
> (BuildId: 6c3f4e311556b29f84c9c4a5d6df5114dc08a12e)
>
Please, remove the '#' signs as github misinterprets them as PR/issue
reference. And, please, remove the unnecessary info from the trace,
e.g. BuildId, '/workspace/ovn/' part of the paths and maybe some other
parts of the base libc frames.
> SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior lib/hash.c:46:22
>
> Signed-off-by: Ales Musil
> ---
> lib/hash.c | 2 +-
> lib/jhash.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/lib/hash.c b/lib/hash.c
> index c722f3c3c..986fa6643 100644
> --- a/lib/hash.c
> +++ b/lib/hash.c
> @@ -43,7 +43,7 @@ hash_bytes(const void *p_, size_t n, uint32_t basis)
> if (n) {
> uint32_t tmp = 0;
>
> -memcpy(, p, n);
> +memcpy(, (const uint8_t *) p, n);
We may accept the change, however, this looks more like a compiler
bug to me. memcpy() accepts void pointers, so there is already an
implicit cast. I didn't look into assembly, but I'd guess clang
inlines the call and while doing that assumes the type. I'm not
sure it is allowed to do that. Also, the 'n' here is always less
than 4, so alignment should not be a problem because we can't copy
the whole thing in a single aligned instruction (maybe there are
instructions that can copy just 3 bytes without touching the 4th,
but idk).
Did you have a look at the asm by any chance?
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1] nedev-dpdk: Fix config with dpdk net_bonding offloads.
KSUM_OFFLOAD;
>>> }
>>>
>>> - if (info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_OUTER_UDP_CKSUM) {
>>> + if (info->tx_offload_capa & RTE_ETH_TX_OFFLOAD_OUTER_UDP_CKSUM) {
>>> dev->hw_ol_features |= NETDEV_TX_OUTER_UDP_CKSUM_OFFLOAD;
>>> } else {
>>> dev->hw_ol_features &= ~NETDEV_TX_OUTER_UDP_CKSUM_OFFLOAD;
>>> @@ -1404,21 +1397,21 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
>>>
>>> dev->hw_ol_features &= ~NETDEV_TX_TSO_OFFLOAD;
>>> if (userspace_tso_enabled()) {
>>> - if (info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO) {
>>> + if (info->tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO) {
>>> dev->hw_ol_features |= NETDEV_TX_TSO_OFFLOAD;
>>> } else {
>>> VLOG_WARN("%s: Tx TSO offload is not supported.",
>>> netdev_get_name(>up));
>>> }
>>>
>>> - if (info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO) {
>>> + if (info->tx_offload_capa & RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO) {
>>> dev->hw_ol_features |= NETDEV_TX_VXLAN_TNL_TSO_OFFLOAD;
>>> } else {
>>> VLOG_WARN("%s: Tx Vxlan tunnel TSO offload is not supported.",
>>> netdev_get_name(>up));
>>> }
>>>
>>> - if (info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO) {
>>> + if (info->tx_offload_capa & RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO) {
>>> dev->hw_ol_features |= NETDEV_TX_GENEVE_TNL_TSO_OFFLOAD;
>>> } else {
>>> VLOG_WARN("%s: Tx Geneve tunnel TSO offload is not supported.",
>>> @@ -1426,6 +1419,21 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
>>> }
>>> }
>>>
>>> +}
>>> +
>>> +static int
>>> +dpdk_eth_dev_init(struct netdev_dpdk *dev)
>>> + OVS_REQUIRES(dev->mutex)
>>> +{
>>> + struct rte_pktmbuf_pool_private *mbp_priv;
>>> + struct rte_eth_dev_info info;
>>> + struct rte_ether_addr eth_addr;
>>> + int diag;
>>> + int n_rxq, n_txq;
>>> +
>>> + rte_eth_dev_info_get(dev->port_id, );
>>> + dpdk_eth_offload_config(dev, );
>>> +
>>> n_rxq = MIN(info.max_rx_queues, dev->up.n_rxq);
>>> n_txq = MIN(info.max_tx_queues, dev->up.n_txq);
>>>
>>> @@ -1439,6 +1447,13 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
>>> return -diag;
>>> }
>>>
>>> + rte_eth_dev_info_get(dev->port_id, );
>>> + if (!strcmp(info.driver_name, "net_bonding")) {
>>> + dpdk_eth_offload_config(dev, );
>>> + VLOG_INFO("%s: configure offloads for a dpdk net_bonding port.",
>>> + netdev_get_name(>up));
>>> + }
>>> +
>>
>>I'm a bit confused by this. Why do we need to check offloads again
>>after the initialization? Is there a chance that bonding driver
>>will enable features we didn't ask for?
>>
>>In general, since we don't know what kind of device is in the bonding,
>>we should just disable all offloads for it as long as there is a chance
>>to have a broken driver downstream of a bond.
>>
>>Best regards, Ilya Maximets.
>
> I believe the DPDK net/bonding port has not been configured here yet, so
> all the obtained offloads are false. That's why I reset them again after
> dpdk_eth_dev_port_config. It seems like the DPDK bond has completed
> initialization at this point, allowing us to properly obtain the offload
> flags. I think this should be where the DPDK net/bonding port sets the
> offload flags, which may be the logic of DPDK net/bonding.
If the driver doesn't correctly report its offload capabilities until
it is already configured, it should like a violation of a DPDK API.
Though, knowing DPDK API, it might be undefined enough to allow this
behavior. But still, offloads are part of the rte_eth_dev_configure()
request and based on what the dirver reported. So, we should be
calling rte_eth_dev_configure() again and that doesn't sound right.
> Additionally,
> regarding your second point, shouldn't we consider DPDK's net/bonding?
> I have verified with CX5 network cards, and after reacquiring the offloads,
> the functionality works properly.
Sure, but what about i40e or ice, for example? If those drivers
are behind the bonding we'll get incorrect features reported as
we know that these drivers report incorrect features.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] sparse: Add immintrin.h header.
On 4/30/24 16:41, Ales Musil wrote: > > > On Tue, Apr 30, 2024 at 4:36 PM Ilya Maximets <mailto:i.maxim...@ovn.org>> wrote: > > Sparse doesn't understand _Float16 and some other types used by > immintrin.h from GCC 13. This breaks sparse builds with DPDK on > Fedora 38+ and Ubuntu 24.04. > > Add another sparse-specific header to workaround the problem. We do > need some of the functions and types defined in these headers, so we > can't really stab out the whole header. Carving out the main offenders > instead by defining the inclusion guards. > > This is fragile and depends on internals of immintrin and underlying > headers, but I'm not sure what the better way to solve the issue > would be. This approach should be more or less portable between > compilers, because it only defines a few specific variables. We may > have to add more as GCC headers change over time. > > This fixes the build with a following config on F38 and Ubuntu 24.04: > > ./configure --enable-sparse --with-dpdk=yes --enable-Werror > > Signed-off-by: Ilya Maximets <mailto:i.maxim...@ovn.org>> > --- > > Looks good to me, thanks! > > Acked-by: Ales Musil mailto:amu...@redhat.com>> Thanks! Applied to all branches down to 2.17. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] tc: Fix -Wgnu-variable-sized-type-not-at-end warning with Clang 18.
On 4/30/24 12:41, Ales Musil wrote: > > > On Fri, Apr 26, 2024 at 7:44 PM Ilya Maximets <mailto:i.maxim...@ovn.org>> wrote: > > Clang 18.1.3-2.fc41 throws a warning: > > lib/tc.c:3060:25: error: field 'sel' with variable sized type > 'struct tc_pedit_sel' not at the end of a struct or class is a > GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end] > > 3060 | struct tc_pedit sel; > | ^ > > Refactor the structure into a proper union to avoid the build failure. > > Interestingly, clang 18.1.3-2.fc41 on Fedora throws a warning, but > relatively the same version 18.1.3 (1) on Ubuntu 23.04 does not. > > Signed-off-by: Ilya Maximets <mailto:i.maxim...@ovn.org>> > --- > > Looks good to me, thanks. > > Acked-by: Ales Musil mailto:amu...@redhat.com>> Thanks, Ales! Applied to all branches down to 2.17. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] tests: Fix build failure with Clang 18 due to -Wformat-truncation.
On 4/30/24 13:12, Eelco Chaudron wrote: > > > On 30 Apr 2024, at 11:53, Ilya Maximets wrote: > >> On 4/26/24 18:35, Ilya Maximets wrote: >>> Cirrus CI is broken on FreeBSD 13.3 due to clang version update. >>> It now complains about snprintf truncation the same way GCC does: >>> >>> tests/test-util.c:1129:16: error: 'snprintf' will always be truncated; >>> specified size is 5, but format string expands to at least 6 >>> [-Werror,-Wformat-truncation] >>> >>> 1129 | ovs_assert(snprintf(s, 5, "abcde") == 5); >>>|^ >>> >>> Clang 17 on FreeBSD 14.0 works fine, but new Clang 18.1.4 on 13.3 >>> fails to build. >>> >>> Fix that by disabling Clang diagnostic the same way as we do for GCC. >>> >>> Unfortunately, the pragma's are compiler-specific, so cannot be >>> combined, AFAIK. >>> >>> Signed-off-by: Ilya Maximets >>> --- >> >> Hi, Eelco and Simon. May I ask you to take a look at this patch? >> >> It's blocking Cirrus CI and I don't think anything else should be >> merged until CI is fixed. >> >> Best regards, Ilya Maximets. > > Thanks Ilya, I agree! This looks good to me. > > Acked-by: Eelco Chaudron Thanks, Eelco and Ales! Applied to all branches down to 2.17. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v3] ci: Keep the container version pinned.
On 4/30/24 16:44, Ales Musil wrote: > The Ubuntu 24.04 brought some issues that are not really straight > forward to fix. Keep the Ubuntu version on 22.04 for now to keep > the CI working. > > At the same time Fedora updated Clang to version 18, which is > throwing compilation error that need to be fixed in OvS first. > > Signed-off-by: Ales Musil > --- > utilities/containers/fedora/Dockerfile | 2 +- > utilities/containers/ubuntu/Dockerfile | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/utilities/containers/fedora/Dockerfile > b/utilities/containers/fedora/Dockerfile > index bf3c293fc..9b8386aae 100755 > --- a/utilities/containers/fedora/Dockerfile > +++ b/utilities/containers/fedora/Dockerfile > @@ -1,4 +1,4 @@ > -FROM quay.io/fedora/fedora:latest > +FROM quay.io/fedora/fedora:39 > > ARG CONTAINERS_PATH > > diff --git a/utilities/containers/ubuntu/Dockerfile > b/utilities/containers/ubuntu/Dockerfile > index 1371b3f70..ac1e6a5bf 100755 > --- a/utilities/containers/ubuntu/Dockerfile > +++ b/utilities/containers/ubuntu/Dockerfile > @@ -1,4 +1,4 @@ > -FROM registry.hub.docker.com/library/ubuntu:latest > +FROM registry.hub.docker.com/library/ubuntu:22.04 > > ARG CONTAINERS_PATH > I think, we should also pin the version in build-linux-rpm job in .github/workflows/test.yml. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] sparse: Add immintrin.h header.
Sparse doesn't understand _Float16 and some other types used by immintrin.h from GCC 13. This breaks sparse builds with DPDK on Fedora 38+ and Ubuntu 24.04. Add another sparse-specific header to workaround the problem. We do need some of the functions and types defined in these headers, so we can't really stab out the whole header. Carving out the main offenders instead by defining the inclusion guards. This is fragile and depends on internals of immintrin and underlying headers, but I'm not sure what the better way to solve the issue would be. This approach should be more or less portable between compilers, because it only defines a few specific variables. We may have to add more as GCC headers change over time. This fixes the build with a following config on F38 and Ubuntu 24.04: ./configure --enable-sparse --with-dpdk=yes --enable-Werror Signed-off-by: Ilya Maximets --- include/sparse/automake.mk | 1 + include/sparse/immintrin.h | 30 ++ 2 files changed, 31 insertions(+) create mode 100644 include/sparse/immintrin.h diff --git a/include/sparse/automake.mk b/include/sparse/automake.mk index c1229870b..45e6202c5 100644 --- a/include/sparse/automake.mk +++ b/include/sparse/automake.mk @@ -1,5 +1,6 @@ noinst_HEADERS += \ include/sparse/rte_byteorder.h \ +include/sparse/immintrin.h \ include/sparse/xmmintrin.h \ include/sparse/arpa/inet.h \ include/sparse/bits/floatn.h \ diff --git a/include/sparse/immintrin.h b/include/sparse/immintrin.h new file mode 100644 index 0..dd742be9f --- /dev/null +++ b/include/sparse/immintrin.h @@ -0,0 +1,30 @@ +/* Copyright (c) 2024 Red Hat, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at: + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#ifndef __CHECKER__ +#error "Use this header only with sparse. It is not a correct implementation." +#endif + +/* Sparse doesn't know some types used by AVX512 and some other headers. + * Mark those headers as already included to avoid failures. This is fragile, + * so may need adjustments with compiler changes. */ +#define _AVX512BF16INTRIN_H_INCLUDED +#define _AVX512BF16VLINTRIN_H_INCLUDED +#define _AVXNECONVERTINTRIN_H_INCLUDED +#define _KEYLOCKERINTRIN_H_INCLUDED +#define __AVX512FP16INTRIN_H_INCLUDED +#define __AVX512FP16VLINTRIN_H_INCLUDED + +#include_next -- 2.44.0 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] tests: Fix build failure with Clang 18 due to -Wformat-truncation.
On 4/26/24 18:35, Ilya Maximets wrote:
> Cirrus CI is broken on FreeBSD 13.3 due to clang version update.
> It now complains about snprintf truncation the same way GCC does:
>
> tests/test-util.c:1129:16: error: 'snprintf' will always be truncated;
> specified size is 5, but format string expands to at least 6
> [-Werror,-Wformat-truncation]
>
> 1129 | ovs_assert(snprintf(s, 5, "abcde") == 5);
>|^
>
> Clang 17 on FreeBSD 14.0 works fine, but new Clang 18.1.4 on 13.3
> fails to build.
>
> Fix that by disabling Clang diagnostic the same way as we do for GCC.
>
> Unfortunately, the pragma's are compiler-specific, so cannot be
> combined, AFAIK.
>
> Signed-off-by: Ilya Maximets
> ---
Hi, Eelco and Simon. May I ask you to take a look at this patch?
It's blocking Cirrus CI and I don't think anything else should be
merged until CI is fixed.
Best regards, Ilya Maximets.
> tests/test-util.c | 13 ++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/tests/test-util.c b/tests/test-util.c
> index 7d899fbbf..5d88d38f2 100644
> --- a/tests/test-util.c
> +++ b/tests/test-util.c
> @@ -1116,12 +1116,16 @@ test_snprintf(struct ovs_cmdl_context *ctx OVS_UNUSED)
> {
> char s[16];
>
> +/* GCC 7+ and Clang 18+ warn about the following calls that truncate
> + * a string using snprintf(). We're testing that truncation works
> + * properly, so temporarily disable the warning. */
> #if __GNUC__ >= 7
> -/* GCC 7+ warns about the following calls that truncate a string using
> - * snprintf(). We're testing that truncation works properly, so
> - * temporarily disable the warning. */
> #pragma GCC diagnostic push
> #pragma GCC diagnostic ignored "-Wformat-truncation"
> +#endif
> +#if __clang_major__ >= 18
> +#pragma clang diagnostic push
> +#pragma clang diagnostic ignored "-Wformat-truncation"
> #endif
> ovs_assert(snprintf(s, 4, "abcde") == 5);
> ovs_assert(!strcmp(s, "abc"));
> @@ -1130,6 +1134,9 @@ test_snprintf(struct ovs_cmdl_context *ctx OVS_UNUSED)
> ovs_assert(!strcmp(s, "abcd"));
> #if __GNUC__ >= 7
> #pragma GCC diagnostic pop
> +#endif
> +#if __clang_major__ >= 18
> +#pragma clang diagnostic pop
> #endif
>
> ovs_assert(snprintf(s, 6, "abcde") == 5);
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] GitHub: Add Coverity scan as a daily GitHub action.
On 4/16/24 09:44, Eelco Chaudron wrote: > This patch adds a daily Coverity run for the OVS main branch > to the GitHub actions. The result of the runs can be found here: > > https://scan.coverity.com/projects/openvswitch > > Before applying, we need to add the following two actions secrets > to the GitHub openvswitch project: > > - COVERITY_SCAN_TOKEN; The secret token from the project page > - COVERITY_SCAN_EMAIL; The maintainer's email alias > > Signed-off-by: Eelco Chaudron > --- > .github/workflows/coverity.yml | 131 + > Makefile.am| 1 + > README.rst | 2 + > 3 files changed, 134 insertions(+) > create mode 100644 .github/workflows/coverity.yml Thanks, Eelco for bringing this up. Though, continuing the offline discussion, I'm not sure having coverity scans as a GitHub action has any benefits for OVS community. A few issues: 1. Reports are not available to people outside of the OVS Coverity project. Adding everyone there doesn't seem reasonable. Also a potential security concern. 2. Workflow cannot be used in forks due to organization secrets. (IIUC, current implementation will just fail once a day annoying people who didn't disable that workflow.) 3. Necessity to have secrets in the organization. We currently don't have any, and from a security standpoint not having any secrets is always better. 4. A decent amount of code duplication for this workflow. (Reusable workflows maybe?) In its current form, I think, the same functionality can be provided by the 0-day robot that could submit sources once a day for the scan. And it could also send some emails with build details if necessary (again, there may be some security concerns in case coverity discovers a genuine security issue). With the total amount of code in the project, IIUC, we could submit up to 21 builds a week with no more than 3 per day (Coverity claims that it checked just under 300K lines in OVS). So, technically, we could create a bit more complex logic for 0-day bot to run a check per patch set (probably, not per patch though). In most cases we would fit within the limit with the current patch rate. Maybe reserving one run a week for the main branch. Such logic would be harder to implement with GHA. All in all, using GHA just for scheduling of a job that wider community can't take advantage of seems a little unjustified. Thoughts? Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] tc: Fix -Wgnu-variable-sized-type-not-at-end warning with Clang 18.
On 4/26/24 19:44, Ilya Maximets wrote: > Clang 18.1.3-2.fc41 throws a warning: > > lib/tc.c:3060:25: error: field 'sel' with variable sized type > 'struct tc_pedit_sel' not at the end of a struct or class is a > GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end] > >3060 | struct tc_pedit sel; > | ^ > > Refactor the structure into a proper union to avoid the build failure. > > Interestingly, clang 18.1.3-2.fc41 on Fedora throws a warning, but > relatively the same version 18.1.3 (1) on Ubuntu 23.04 does not. > > Signed-off-by: Ilya Maximets > --- Recheck-request: github-robot (Robot was down and didn't check the patch. Same for a few other patches on a list.) ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] tests: Fix build failure with Clang 18 due to -Wformat-truncation.
On 4/26/24 18:35, Ilya Maximets wrote: > Cirrus CI is broken on FreeBSD 13.3 due to clang version update. > It now complains about snprintf truncation the same way GCC does: > > tests/test-util.c:1129:16: error: 'snprintf' will always be truncated; > specified size is 5, but format string expands to at least 6 > [-Werror,-Wformat-truncation] > > 1129 | ovs_assert(snprintf(s, 5, "abcde") == 5); >|^ > > Clang 17 on FreeBSD 14.0 works fine, but new Clang 18.1.4 on 13.3 > fails to build. > > Fix that by disabling Clang diagnostic the same way as we do for GCC. > > Unfortunately, the pragma's are compiler-specific, so cannot be > combined, AFAIK. > > Signed-off-by: Ilya Maximets > --- Recheck-request: github-robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] tc: Fix -Wgnu-variable-sized-type-not-at-end warning with Clang 18.
Clang 18.1.3-2.fc41 throws a warning:
lib/tc.c:3060:25: error: field 'sel' with variable sized type
'struct tc_pedit_sel' not at the end of a struct or class is a
GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
3060 | struct tc_pedit sel;
| ^
Refactor the structure into a proper union to avoid the build failure.
Interestingly, clang 18.1.3-2.fc41 on Fedora throws a warning, but
relatively the same version 18.1.3 (1) on Ubuntu 23.04 does not.
Signed-off-by: Ilya Maximets
---
lib/tc.c | 22 +++---
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/lib/tc.c b/lib/tc.c
index e9bcae4e4..e55ba3b1b 100644
--- a/lib/tc.c
+++ b/lib/tc.c
@@ -3056,17 +3056,17 @@ nl_msg_put_flower_rewrite_pedits(struct ofpbuf *request,
struct tc_action *action,
uint32_t action_pc)
{
-struct {
+union {
struct tc_pedit sel;
-struct tc_pedit_key keys[MAX_PEDIT_OFFSETS];
-struct tc_pedit_key_ex keys_ex[MAX_PEDIT_OFFSETS];
-} sel = {
-.sel = {
-.nkeys = 0
-}
-};
+uint8_t buffer[sizeof(struct tc_pedit)
+ + MAX_PEDIT_OFFSETS * sizeof(struct tc_pedit_key)];
+} sel;
+struct tc_pedit_key_ex keys_ex[MAX_PEDIT_OFFSETS];
int i, j, err;
+memset(, 0, sizeof sel);
+memset(keys_ex, 0, sizeof keys_ex);
+
for (i = 0; i < ARRAY_SIZE(flower_pedit_map); i++) {
struct flower_key_to_pedit *m = _pedit_map[i];
struct tc_pedit_key *pedit_key = NULL;
@@ -3100,8 +3100,8 @@ nl_msg_put_flower_rewrite_pedits(struct ofpbuf *request,
return EOPNOTSUPP;
}
-pedit_key = [sel.sel.nkeys];
-pedit_key_ex = _ex[sel.sel.nkeys];
+pedit_key = [sel.sel.nkeys];
+pedit_key_ex = _ex[sel.sel.nkeys];
pedit_key_ex->cmd = TCA_PEDIT_KEY_EX_CMD_SET;
pedit_key_ex->htype = m->htype;
pedit_key->off = cur_offset;
@@ -3121,7 +3121,7 @@ nl_msg_put_flower_rewrite_pedits(struct ofpbuf *request,
}
}
}
-nl_msg_put_act_pedit(request, , sel.keys_ex,
+nl_msg_put_act_pedit(request, , keys_ex,
flower->csum_update_flags ? TC_ACT_PIPE : action_pc);
return 0;
--
2.44.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] tests: Fix build failure with Clang 18 due to -Wformat-truncation.
On 4/26/24 18:35, Ilya Maximets wrote:
> Cirrus CI is broken on FreeBSD 13.3 due to clang version update.
> It now complains about snprintf truncation the same way GCC does:
>
> tests/test-util.c:1129:16: error: 'snprintf' will always be truncated;
> specified size is 5, but format string expands to at least 6
> [-Werror,-Wformat-truncation]
>
> 1129 | ovs_assert(snprintf(s, 5, "abcde") == 5);
>|^
>
> Clang 17 on FreeBSD 14.0 works fine, but new Clang 18.1.4 on 13.3
> fails to build.
>
> Fix that by disabling Clang diagnostic the same way as we do for GCC.
>
> Unfortunately, the pragma's are compiler-specific, so cannot be
> combined, AFAIK.
>
> Signed-off-by: Ilya Maximets
> ---
The issue affects both Fedora Rawhide and Ubuntu 24.03 as well.
Interestingly, Fedora Rawhide throws one more warning in a different
area, even though the clang version in it is 18.1.3 and it is the
same as in Ubuntu.
Will address that other warning separately.
> tests/test-util.c | 13 ++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/tests/test-util.c b/tests/test-util.c
> index 7d899fbbf..5d88d38f2 100644
> --- a/tests/test-util.c
> +++ b/tests/test-util.c
> @@ -1116,12 +1116,16 @@ test_snprintf(struct ovs_cmdl_context *ctx OVS_UNUSED)
> {
> char s[16];
>
> +/* GCC 7+ and Clang 18+ warn about the following calls that truncate
> + * a string using snprintf(). We're testing that truncation works
> + * properly, so temporarily disable the warning. */
> #if __GNUC__ >= 7
> -/* GCC 7+ warns about the following calls that truncate a string using
> - * snprintf(). We're testing that truncation works properly, so
> - * temporarily disable the warning. */
> #pragma GCC diagnostic push
> #pragma GCC diagnostic ignored "-Wformat-truncation"
> +#endif
> +#if __clang_major__ >= 18
> +#pragma clang diagnostic push
> +#pragma clang diagnostic ignored "-Wformat-truncation"
> #endif
> ovs_assert(snprintf(s, 4, "abcde") == 5);
> ovs_assert(!strcmp(s, "abc"));
> @@ -1130,6 +1134,9 @@ test_snprintf(struct ovs_cmdl_context *ctx OVS_UNUSED)
> ovs_assert(!strcmp(s, "abcd"));
> #if __GNUC__ >= 7
> #pragma GCC diagnostic pop
> +#endif
> +#if __clang_major__ >= 18
> +#pragma clang diagnostic pop
> #endif
>
> ovs_assert(snprintf(s, 6, "abcde") == 5);
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] tests: Fix build failure with Clang 18 due to -Wformat-truncation.
Cirrus CI is broken on FreeBSD 13.3 due to clang version update.
It now complains about snprintf truncation the same way GCC does:
tests/test-util.c:1129:16: error: 'snprintf' will always be truncated;
specified size is 5, but format string expands to at least 6
[-Werror,-Wformat-truncation]
1129 | ovs_assert(snprintf(s, 5, "abcde") == 5);
|^
Clang 17 on FreeBSD 14.0 works fine, but new Clang 18.1.4 on 13.3
fails to build.
Fix that by disabling Clang diagnostic the same way as we do for GCC.
Unfortunately, the pragma's are compiler-specific, so cannot be
combined, AFAIK.
Signed-off-by: Ilya Maximets
---
tests/test-util.c | 13 ++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/tests/test-util.c b/tests/test-util.c
index 7d899fbbf..5d88d38f2 100644
--- a/tests/test-util.c
+++ b/tests/test-util.c
@@ -1116,12 +1116,16 @@ test_snprintf(struct ovs_cmdl_context *ctx OVS_UNUSED)
{
char s[16];
+/* GCC 7+ and Clang 18+ warn about the following calls that truncate
+ * a string using snprintf(). We're testing that truncation works
+ * properly, so temporarily disable the warning. */
#if __GNUC__ >= 7
-/* GCC 7+ warns about the following calls that truncate a string using
- * snprintf(). We're testing that truncation works properly, so
- * temporarily disable the warning. */
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wformat-truncation"
+#endif
+#if __clang_major__ >= 18
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wformat-truncation"
#endif
ovs_assert(snprintf(s, 4, "abcde") == 5);
ovs_assert(!strcmp(s, "abc"));
@@ -1130,6 +1134,9 @@ test_snprintf(struct ovs_cmdl_context *ctx OVS_UNUSED)
ovs_assert(!strcmp(s, "abcd"));
#if __GNUC__ >= 7
#pragma GCC diagnostic pop
+#endif
+#if __clang_major__ >= 18
+#pragma clang diagnostic pop
#endif
ovs_assert(snprintf(s, 6, "abcde") == 5);
--
2.44.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] ci: Fix OPTS not being passed to OSX builds.
On 4/25/24 18:35, Numan Siddique wrote: > On Thu, Apr 25, 2024 at 12:24 PM Ales Musil wrote: >> >> On Thu, Apr 25, 2024 at 4:44 PM wrote: >> >>> From: Numan Siddique >>> >>> OSX job is failing with the below error even though the job >>> disables SSL. >>> >>> ld: library 'ssl' not found >>> >>> Passing OPTS to the OSX build fixes this issue. >>> >>> This issue is already addressed in ovs [1]. >>> >>> [1] - >>> https://github.com/openvswitch/ovs/commit/2f34475a9708617eaa484044a5b485980b734b38 >>> >>> Signed-off-by: Numan Siddique >>> --- >>> .ci/osx-build.sh | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/.ci/osx-build.sh b/.ci/osx-build.sh >>> index 4b78b66dd1..3fcc801e7f 100755 >>> --- a/.ci/osx-build.sh >>> +++ b/.ci/osx-build.sh >>> @@ -19,7 +19,7 @@ function configure_ovn() >>> ./boot.sh && ./configure $* >>> } >>> >>> -configure_ovn $EXTRA_OPTS $* >>> +configure_ovn $EXTRA_OPTS $OPTS $* >>> >>> if [ "$CC" = "clang" ]; then >>> set make CFLAGS="$CFLAGS -Wno-error=unused-command-line-argument" >>> -- >>> 2.44.0 >>> >>> ___ >>> dev mailing list >>> d...@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> >>> >> >> Looks good to me, thanks. >> >> Acked-by: Ales Musil > > Thanks. I applied to main. Should be applied to all supported branches, I suppose. Otherwise, CI will be broken on those. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] rhel/systemd: Set ovsdb-server timeout to 5 minutes
On 4/11/24 15:43, Chris Riches wrote: > On 11/04/2024 14:24, Ilya Maximets wrote: >> On 4/11/24 10:59, Chris Riches wrote: >>> From what we know so far, the DB was full of stale connection-tracking >>> information such as the following: >>> >>> [...] >>> >>> Once the host was recovered by putting in the timeout increase, >>> ovsdb-server successfully started and GCed the database down from 2.4 >>> *GB* to 29 *KB*. Had this happened before the host restart, we would >>> have never seen this problem. But since it seems possible to end up >>> booting with such a large DB, we figured a timeout increase was a >>> sensible measure to take. >> Uff. Sounds like ovn-controller went off the rails. >> >> Normally, ovsdb-server compacts the database once in 10-20 minutes, >> if the database doubles the size since the previous check. If all >> the transactions are that small, it would mean ovn-controller made >> about 10K transactions per second in the 10-20 minutes before the >> restart. That's huge. >> >> I wonder if this can be addressed with a better compaction strategy. >> Something like forcing compaction if "the database is more than 10 MB >> and increased 10x" regardless of the time. > > I'm not sure exactly what the test was doing when this was observed, so > I don't know whether that transaction volume is within the realm of > possibility or if we're looking at a failure to perform compaction on > time. It would be nice to have an enhanced safety-net for DB size, as we > were only a few hundred MB away from hitting filesystem space issues as > well. The compaction check is on the path in the main event loop, so it should not be possible to avoid it, especially for a standalone database. Database will stop executing transactions until compaction is done. The transaction rate is very high, bu it might be possible, I guess, with very small transaction as we have here. I need to experiment with it and maybe I'll post some patches to force compaction earlier under extreme conditions like these. > >> Normally, ovsdb-server compacts the database once in 10-20 minutes, if >> the database doubles the size since the previous check. > > I presume you mean if it doubled in size since the previous > *compaction*? If we only compact when it doubles since the last *check*, > then it would be easy for it to slightly-less-than-double every 10-20 > minutes and never trigger the compaction while still growing exponentially. Yes, I meant compaction, not the check, sorry. So, this scenario is covered and should not be possible. > > I'm happy to discuss compaction approaches (though my expertise is very > much in host service management and not OVS itself), but do you think > there's merit in having this extended timeout as a backstop too? Yep, I applied the change for now. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] rhel/systemd: Set ovsdb-server timeout to 5 minutes
On 4/23/24 13:10, Ilya Maximets wrote: > On 4/23/24 12:35, Simon Horman wrote: >> On Thu, Apr 18, 2024 at 03:35:06PM +0100, Chris Riches wrote: >>> On 15/04/2024 14:39, Jon Kohler wrote: >>>>> On Apr 11, 2024, at 9:43 AM, Chris Riches >>>>> wrote: >>>>> >>>>> On 11/04/2024 14:24, Ilya Maximets wrote: >>>>>> On 4/11/24 10:59, Chris Riches wrote: >>>>>>> From what we know so far, the DB was full of stale connection-tracking >>>>>>> information such as the following: >>>>>>> >>>>>>> [...] >>>>>>> >>>>>>> Once the host was recovered by putting in the timeout increase, >>>>>>> ovsdb-server successfully started and GCed the database down from 2.4 >>>>>>> *GB* to 29 *KB*. Had this happened before the host restart, we would >>>>>>> have never seen this problem. But since it seems possible to end up >>>>>>> booting with such a large DB, we figured a timeout increase was a >>>>>>> sensible measure to take. >>>>>> Uff. Sounds like ovn-controller went off the rails. >>>>>> >>>>>> Normally, ovsdb-server compacts the database once in 10-20 minutes, >>>>>> if the database doubles the size since the previous check. If all >>>>>> the transactions are that small, it would mean ovn-controller made >>>>>> about 10K transactions per second in the 10-20 minutes before the >>>>>> restart. That's huge. >>>>>> >>>>>> I wonder if this can be addressed with a better compaction strategy. >>>>>> Something like forcing compaction if "the database is more than 10 MB >>>>>> and increased 10x" regardless of the time. >>>>> I'm not sure exactly what the test was doing when this was observed, so I >>>>> don't know whether that transaction volume is within the realm of >>>>> possibility or if we're looking at a failure to perform compaction on >>>>> time. It would be nice to have an enhanced safety-net for DB size, as we >>>>> were only a few hundred MB away from hitting filesystem space issues as >>>>> well. >>>>> >>>>>> Normally, ovsdb-server compacts the database once in 10-20 minutes, if >>>>>> the database doubles the size since the previous check. >>>>> I presume you mean if it doubled in size since the previous *compaction*? >>>>> If we only compact when it doubles since the last *check*, then it would >>>>> be easy for it to slightly-less-than-double every 10-20 minutes and never >>>>> trigger the compaction while still growing exponentially. >>>>> >>>>> I'm happy to discuss compaction approaches (though my expertise is very >>>>> much in host service management and not OVS itself), but do you think >>>>> there's merit in having this extended timeout as a backstop too? >>>> FWIW, I think we should do both extending the time out and tuning up the >>>> compaction, as having a situation where a service can get in an endless >>>> loop if for whatever reason it takes too long is problematic. Addressing >>>> the root cause (compaction, too many calls, some other bug(s) etc) is >>>> good, but extending the timeout seems like an easy backstop. >>> >>> I agree with Jon's assessment - regardless of any action taken on compaction >>> or preventing growth in the first place, we should consider the proposed >>> timeout increase as a backstop against getting stuck in an infinite loop. >>> >>> Ilya (or another maintainer) - can I get an opinion on this? >> >> Yes, I agree that the timeout increase is a good idea. >> >> Acked-by: Simon Horman >> > > Sorry for delay, been off for a week. I agree that timeout increase > makes sense since we know the mechanism for the occurrence of the issue. > > I plan to catch up on the rest of the thread and apply the fix later today. Applied to main and backported down to 2.17. Thanks! Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 0/2] ovsdb: raft: Fixes for probe interval updates.
On 4/12/24 01:45, Ilya Maximets wrote: > A couple of fixes related to corner cases with probe intervals > on RAFT connections between servers. > > Ilya Maximets (2): > ovsdb: raft: Fix inability to join a cluster with a large database. > ovsdb: raft: Fix probe intervals after install snapshot request. > > ovsdb/raft.c | 23 ++- > 1 file changed, 18 insertions(+), 5 deletions(-) > Thanks, Mike, for review! Applied and backported down to 2.17. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] rhel/systemd: Set ovsdb-server timeout to 5 minutes
On 4/23/24 12:35, Simon Horman wrote: > On Thu, Apr 18, 2024 at 03:35:06PM +0100, Chris Riches wrote: >> On 15/04/2024 14:39, Jon Kohler wrote: >>>> On Apr 11, 2024, at 9:43 AM, Chris Riches wrote: >>>> >>>> On 11/04/2024 14:24, Ilya Maximets wrote: >>>>> On 4/11/24 10:59, Chris Riches wrote: >>>>>> From what we know so far, the DB was full of stale connection-tracking >>>>>> information such as the following: >>>>>> >>>>>> [...] >>>>>> >>>>>> Once the host was recovered by putting in the timeout increase, >>>>>> ovsdb-server successfully started and GCed the database down from 2.4 >>>>>> *GB* to 29 *KB*. Had this happened before the host restart, we would >>>>>> have never seen this problem. But since it seems possible to end up >>>>>> booting with such a large DB, we figured a timeout increase was a >>>>>> sensible measure to take. >>>>> Uff. Sounds like ovn-controller went off the rails. >>>>> >>>>> Normally, ovsdb-server compacts the database once in 10-20 minutes, >>>>> if the database doubles the size since the previous check. If all >>>>> the transactions are that small, it would mean ovn-controller made >>>>> about 10K transactions per second in the 10-20 minutes before the >>>>> restart. That's huge. >>>>> >>>>> I wonder if this can be addressed with a better compaction strategy. >>>>> Something like forcing compaction if "the database is more than 10 MB >>>>> and increased 10x" regardless of the time. >>>> I'm not sure exactly what the test was doing when this was observed, so I >>>> don't know whether that transaction volume is within the realm of >>>> possibility or if we're looking at a failure to perform compaction on >>>> time. It would be nice to have an enhanced safety-net for DB size, as we >>>> were only a few hundred MB away from hitting filesystem space issues as >>>> well. >>>> >>>>> Normally, ovsdb-server compacts the database once in 10-20 minutes, if >>>>> the database doubles the size since the previous check. >>>> I presume you mean if it doubled in size since the previous *compaction*? >>>> If we only compact when it doubles since the last *check*, then it would >>>> be easy for it to slightly-less-than-double every 10-20 minutes and never >>>> trigger the compaction while still growing exponentially. >>>> >>>> I'm happy to discuss compaction approaches (though my expertise is very >>>> much in host service management and not OVS itself), but do you think >>>> there's merit in having this extended timeout as a backstop too? >>> FWIW, I think we should do both extending the time out and tuning up the >>> compaction, as having a situation where a service can get in an endless >>> loop if for whatever reason it takes too long is problematic. Addressing >>> the root cause (compaction, too many calls, some other bug(s) etc) is >>> good, but extending the timeout seems like an easy backstop. >> >> I agree with Jon's assessment - regardless of any action taken on compaction >> or preventing growth in the first place, we should consider the proposed >> timeout increase as a backstop against getting stuck in an infinite loop. >> >> Ilya (or another maintainer) - can I get an opinion on this? > > Yes, I agree that the timeout increase is a good idea. > > Acked-by: Simon Horman > Sorry for delay, been off for a week. I agree that timeout increase makes sense since we know the mechanism for the occurrence of the issue. I plan to catch up on the rest of the thread and apply the fix later today. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v9 6/6] ofproto: Add JSON output for 'dpif/show' command.
On 4/12/24 09:26, jm...@redhat.com wrote:
> From: Jakob Meng
>
> The 'dpif/show' command now supports machine-readable JSON output in
> addition to the plain-text output for humans. An example would be:
>
> ovs-appctl --format json dpif/show
>
> Reported-at: https://bugzilla.redhat.com/1824861
> Signed-off-by: Jakob Meng
> ---
> NEWS | 1 +
> ofproto/ofproto-dpif.c | 124 +
> tests/pmd.at | 28 ++
> 3 files changed, 142 insertions(+), 11 deletions(-)
>
Hi, Jakob. Thanks for v9!
The general approach in the set seems reasonable, however I didn't
read the code carefully enough. I hope to do that once I'm back
from PTO in one week.
I didn't read the code in this patch either, but I have a couple
of general comments for the formatting below.
> diff --git a/NEWS b/NEWS
> index af83623b3..97b30233c 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -14,6 +14,7 @@ Post-v3.3.0
> E.g. members of objects and elements of arrays are printed one per
> line,
> with indentation.
> * 'list-commands' now supports output in JSON format.
> + * 'dpif/show' now supports output in JSON format.
> - Python:
> * Added support for choosing the output format, e.g. 'json' or 'text'.
> * Added new option [--pretty] to print JSON output in a readable
> fashion.
These NEWS entries look strange. "Output format for python" sounds
strange. Is it for every python library we have? The section should
be more specific on what it means.
> diff --git a/tests/pmd.at b/tests/pmd.at
> index 35a44b4df..cff80da15 100644
> --- a/tests/pmd.at
> +++ b/tests/pmd.at
> @@ -112,6 +112,34 @@ dummy@ovs-dummy: hit:0 missed:0
> p0 1/1: (dummy-pmd: n_rxq=1, n_txq=1, numa_id=0)
> ])
>
> +AT_CHECK([ovs-appctl --format json --pretty dpif/show], [0], [dnl
> +[[
> + {
> +"name": "dummy@ovs-dummy",
> +"ofprotos": [
> + {
> +"name": "br0",
> +"ports": [
> + {
> +"netdev_config": {
> + },
> +"netdev_name": "br0",
> +"netdev_type": "dummy-internal",
> +"odp_port": "100",
> +"ofp_port": "65534"},
> + {
> +"netdev_config": {
> + "n_rxq": "1",
> + "n_txq": "1",
> + "numa_id": "0"},
> +"netdev_name": "p0",
> +"netdev_type": "dummy-pmd",
> +"odp_port": "1",
> +"ofp_port": "1"}]}],
> +"stats": {
> + "n_hit": "0",
> + "n_missed": "0"}}]]])
I'd suggest a different format for this command, e.g.:
{
"dummy@ovs-dummy": {
"bridges": {
"br0": {
"br0": {
"config": {},
"type": "dummy-internal",
"port_no": "100",
"ofport": "65534"},
"p0": {
"config": {
"n_rxq": "1",
"n_txq": "1",
"numa_id": "0"},
"type": "dummy-pmd",
"port_no": "1",
"ofport": "1"}}},
"stats": {
"n_hit": "0",
"n_missed": "0"}}
}
Using dictionaries with names as keys saves us from some of the strange
naming. "ofprotos" is a strange word and is not understandable by users.
It's an internal word. The user-facing entity is a bridge, not ofproto.
We don't need to have "netdev_" prefixes, it's already clear that it is
a port configuration if it is a part of port values. All datapaths,
bridges and ports have unique names, so they should be keys in JSON objects,
no need to use arrays. "ofp_port" means "OpenFlow port port", no need to
repeat. In the database we have "ofport" name, so we can use it here as
well. "dpif" commands operate with "port_no" as a datapath port number,
so that should be used instead of "odp_port".
Does that make sense?
Also, I noticed that non-pretty output is not sorted, this may cause
random CI failures, because the order depends on a hash function for
smap. I'd say the output should always be sorted, or we'll have to
somehow sort the values in tests.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [ovs-build] |fail| pw1922880 [ovs-dev, v9, 1/6] Add global option for JSON output to ovs-appctl.
On 4/12/24 16:19, Phelan, Michael wrote: >> -Original Message- >> From: Ilya Maximets >> Sent: Friday, April 12, 2024 12:46 PM >> To: Phelan, Michael >> Cc: i.maxim...@ovn.org; Jakob Meng ; Aaron Conole >> ; Stokes, Ian ; ovs-dev > d...@openvswitch.org> >> Subject: Re: [ovs-build] |fail| pw1922880 [ovs-dev, v9, 1/6] Add global >> option for JSON output to ovs-appctl. >> >>> Test-Label: intel-ovs-compilation >>> Test-Status: fail >>> http://patchwork.ozlabs.org/api/patches/1922880/ >>> >>> AVX-512_compilation: failed >>> DPLCS Test: success >>> DPIF Test: fail >>> MFEX Test: fail >>> Actions Test: fail >>> Errors in DPCLS test: >>> None >>> >>> Errors in DPIF test: >>> git-pw patch apply 1922880 >>> Starting new HTTPS connection (1): patchwork.ozlabs.org Starting new >>> HTTPS connection (1): patchwork.ozlabs.org Failed to apply patch: >>> Applying: Add global option for JSON output to ovs-appctl. >>> Using index info to reconstruct a base tree... >>> M NEWS >>> Falling back to patching base and 3-way merge... >>> Auto-merging NEWS >>> CONFLICT (content): Merge conflict in NEWS >>> error: Failed to merge in the changes. >> >> Hi, Michael. >> >> Could you, please, check why Intel CI fails to apply the change? >> Can it be related to the master/main rename? The NEWS file was changed >> after the rename, so I'm guessing, CI still trying to apply patches to a >> version of >> the code before the rename. >> But it's just a guess. > Hi Ilya, > Yes, sorry, the repo was being reset to an older version, I've updated it now > to keep up to date with main. Thanks! Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1] nedev-dpdk: Fix config with dpdk net_bonding offloads.
}
>
> -if (info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO) {
> +if (info->tx_offload_capa & RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO) {
> dev->hw_ol_features |= NETDEV_TX_GENEVE_TNL_TSO_OFFLOAD;
> } else {
> VLOG_WARN("%s: Tx Geneve tunnel TSO offload is not supported.",
> @@ -1426,6 +1419,21 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
> }
> }
>
> +}
> +
> +static int
> +dpdk_eth_dev_init(struct netdev_dpdk *dev)
> +OVS_REQUIRES(dev->mutex)
> +{
> +struct rte_pktmbuf_pool_private *mbp_priv;
> +struct rte_eth_dev_info info;
> +struct rte_ether_addr eth_addr;
> +int diag;
> +int n_rxq, n_txq;
> +
> +rte_eth_dev_info_get(dev->port_id, );
> +dpdk_eth_offload_config(dev, );
> +
> n_rxq = MIN(info.max_rx_queues, dev->up.n_rxq);
> n_txq = MIN(info.max_tx_queues, dev->up.n_txq);
>
> @@ -1439,6 +1447,13 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
> return -diag;
> }
>
> +rte_eth_dev_info_get(dev->port_id, );
> +if (!strcmp(info.driver_name, "net_bonding")) {
> +dpdk_eth_offload_config(dev, );
> +VLOG_INFO("%s: configure offloads for a dpdk net_bonding port.",
> + netdev_get_name(>up));
> +}
> +
I'm a bit confused by this. Why do we need to check offloads again
after the initialization? Is there a chance that bonding driver
will enable features we didn't ask for?
In general, since we don't know what kind of device is in the bonding,
we should just disable all offloads for it as long as there is a chance
to have a broken driver downstream of a bond.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] docs: Document manual cluster recovery procedure.
On 4/12/24 15:12, Ihar Hrachyshka wrote: > Remove the notion of cluster/leave --force since it was never > implemented. Instead of these instructions, document how a broken > cluster can be re-initialized with the old database contents. > > Signed-off-by: Ihar Hrachyshka Hi, Ihar. Thanks for cleaning this up! See some comments inline. Best regards, Ilya Maximets. > > --- > > v1: initial version. > v2: remove --force mentioned in ovsd-server(1). > > --- > Documentation/ref/ovsdb.7.rst | 50 +-- > ovsdb/ovsdb-server.1.in | 3 +-- > 2 files changed, 43 insertions(+), 10 deletions(-) > > diff --git a/Documentation/ref/ovsdb.7.rst b/Documentation/ref/ovsdb.7.rst > index 46ed13e61..5882643a0 100644 > --- a/Documentation/ref/ovsdb.7.rst > +++ b/Documentation/ref/ovsdb.7.rst > @@ -315,16 +315,11 @@ The above methods for adding and removing servers only > work for healthy > clusters, that is, for clusters with no more failures than their maximum > tolerance. For example, in a 3-server cluster, the failure of 2 servers > prevents servers joining or leaving the cluster (as well as database access). > + > To prevent data loss or inconsistency, the preferred solution to this problem > is to bring up enough of the failed servers to make the cluster healthy > again, > -then if necessary remove any remaining failed servers and add new ones. If > -this cannot be done, though, use ``ovs-appctl`` to invoke ``cluster/leave > ---force`` on a running server. This command forces the server to which it is > -directed to leave its cluster and form a new single-node cluster that > contains > -only itself. The data in the new cluster may be inconsistent with the former > -cluster: transactions not yet replicated to the server will be lost, and > -transactions not yet applied to the cluster may be committed. Afterward, any > -servers in its former cluster will regard the server to have failed. > +then if necessary remove any remaining failed servers and add new ones. If > this > +is not an option, see the next section for manual recovery procedure. The link to the section should be used here: ... see the next section for `Manual cluster recovery`_. > > Once a server leaves a cluster, it may never rejoin it. Instead, create a > new > server and join it to the cluster. > @@ -362,6 +357,45 @@ Clustered OVSDB does not support the OVSDB "ephemeral > columns" feature. > ones when they work with schemas for clustered databases. Future versions of > OVSDB might add support for this feature. > > +Manual cluster recovery > +~~~ > + > +If kicking and rejoining failed members to the existing cluster is not > +available in your environment, you may consider to recover the cluster Please, avoid personal pronouns like 'your/you' in documentation. Documentation should describe the process or functionality, it should not generally talk to a reader. For example, in this part, it can be: """ If kicking and rejoining failed members to the existing cluster is not available, it is possible to recover the cluster manually, as follows. """ Also, kicked server can't re-join without following this manual recovery procedure, so this sentence is probably not something we should say. > +manually, as follows. > + > +*Important*: The procedure below will result in `cid` and `sid` change. It should be in '.. important::' section instead. Also, double-quote cid and sid. > +Afterward, any servers in the former cluster will regard the recovered server > +failed. There will be no former cluster, the procedure below is asking to stop all the members... > + > +If you understand the risks and are still willing to proceed, then: Would be nice to re-phrase this to not have 'you'. > + > +1. Stop the old cluster ``ovsdb-server`` instances before proceeding. Might make sense to clarify that all servers should be stopped. > + > +2. Pick one of the old members which will serve as the bootstrap member of > the 'as a bootstrap' ? > + to-be-recovered cluster. > + > +3. Convert its database file to standalone format using ``ovsdb-tool 'to a standalone format' ? > + cluster-to-standalone``. > + > +4. Backup the standalone database file. You will use the file in the next > step. The second sentence can be removed, I think. > + > +5. Re-initialize the new cluster with the bootstrap member (``ovsdb-tool > + create-cluster``) using the previously saved database file. Maybe "Create a new single-node cluster with ``ovsdb-tool create-cluster`` using using the previously saved standalone database file, then strart ``ovsdb-server``" ? > + > +6. Start the bootstrapped cluster w
Re: [ovs-dev] [ovs-build] |fail| pw1922880 [ovs-dev, v9, 1/6] Add global option for JSON output to ovs-appctl.
> Test-Label: intel-ovs-compilation > Test-Status: fail > http://patchwork.ozlabs.org/api/patches/1922880/ > > AVX-512_compilation: failed > DPLCS Test: success > DPIF Test: fail > MFEX Test: fail > Actions Test: fail > Errors in DPCLS test: > None > > Errors in DPIF test: > git-pw patch apply 1922880 > Starting new HTTPS connection (1): patchwork.ozlabs.org > Starting new HTTPS connection (1): patchwork.ozlabs.org > Failed to apply patch: > Applying: Add global option for JSON output to ovs-appctl. > Using index info to reconstruct a base tree... > M NEWS > Falling back to patching base and 3-way merge... > Auto-merging NEWS > CONFLICT (content): Merge conflict in NEWS > error: Failed to merge in the changes. Hi, Michael. Could you, please, check why Intel CI fails to apply the change? Can it be related to the master/main rename? The NEWS file was changed after the rename, so I'm guessing, CI still trying to apply patches to a version of the code before the rename. But it's just a guess. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
