Hi Kevin, Kevin Traynor, Jan 17, 2023 at 19:38: > I saw in patchwork that there was a CI fail but I didn't examine it. > Perhaps you could check and confirm about it.
Yes I saw it and I didn't understand what went wrong: 2023-01-13T15:44:29.422Z|00001|dpif_netdev(revalidator75)|ERR|internal error parsing flow key skb_priority(0),skb_mark(0),ct_state(0),ct_zone(0),ct_mark(0),ct_label(0),recirc_id(0),dp_hash(0),in_port(2),packet_type(ns=0,id=0),eth(src=5a:51:ee:7c:af:0d,dst=33:33:ff:7c:af:0d),eth_type(0x88a8),vlan(vid=4094,pcp=0),encap(eth_type(0x8100),vlan(vid=100,pcp=0),encap(eth_type(0x86dd),ipv6(src=::,dst=ff02::1:ff7c:af0d,label=0,proto=58,tclass=0,hlimit=255,frag=no),icmpv6(type=135,code=0),nd(target=fe80::5851:eeff:fe7c:af0d,sll=00:00:00:00:00:00,tll=00:00:00:00:00:00))) 2023-01-13T15:44:29.422Z|00002|dpif(revalidator75)|WARN|netdev at ovs-netdev: failed to put[modify] (Invalid argument) ufid:8ad5a583-080e-4333-9645-095cc328f6c3 skb_priority(0/0),skb_mark(0/0),ct_state(0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),recirc_id(0),dp_hash(0/0),in_port(2),packet_type(ns=0,id=0),eth(src=5a:51:ee:7c:af:0d,dst=33:33:ff:7c:af:0d),eth_type(0x88a8),vlan(vid=4094,pcp=0/0x0),encap(eth_type(0x8100),vlan(vid=100/0x0,pcp=0/0x0),encap(eth_type(0x86dd),ipv6(src=::/::,dst=ff02::1:ff7c:af0d/::,label=0/0,proto=58/0,tclass=0/0,hlimit=255/0,frag=no),icmpv6(type=135/0,code=0/0),nd(target=fe80::5851:eeff:fe7c:af0d/::,sll=00:00:00:00:00:00/00:00:00:00:00:00,tll=00:00:00:00:00:00/00:00:00:00:00:00))), actions:drop 151. system-traffic.at:7110: 151. 802.1ad - vlan_limit (system-traffic.at:7110): FAILED (system-traffic.at:7135) https://mail.openvswitch.org/pipermail/ovs-build/2023-January/027852.html It does not seem related to this patch but I could be wrong. > > It cannot be enabled when other_config:hw-offload=true as it may > > conflict with the offloaded RTE flows. Similarly, if hw-offload is > > enabled while some ports already have cp-protection enabled, RTE > > flow offloading will be disabled on these ports. > > I'm not sure if you are referring the features hw-offload and/or > cp-prot when you say RTE flow offloading will be disabled on these > ports. > > What I see is that if cp-prot is enabled on a port and then hw-offload > is enabled by the user, hw-offload will not enable because of cp-prot > AND cp-prot will also be disabled by virtue the user trying to enable > hw-offload. > > I was not expecting that last part, even if they are mutually > exclusive features I thought one would work. Perhaps that is > intentional and what you meant above, I wasn't 100% clear. This was not intentional. To be honest, I did not run extensive tests with hw-offload. I will have a look at this use case and see if I can at least leave cp-protection enabled. It may not be possible given that hw-offload is not enabled immediately upon the ovsdb update but asynchronously. Also, since cp-protection is a dpdk specific feature, and hw-offload is dpif-netdev, I'm not sure if there is a way to check anything when the user enables hw-offload. > Anyway, I understand this is just experimental and there is a lot of > scenarios to cover between the features (e.g. what if one of the > features could not rte_flow_validate(), could the other be used then?) > > I think you would need to them tease out in the future and if they > stay mutually exclusive that there is a clear precedence either by > feature or order enabled etc. You might even need to put a table in > docs to show the user. I think that making both features work without interference would not be easy. We would need a way to ensure that the cp-protection flow happens before the RSS flow that is inserted by hw-offload when a datapath flow cannot be offloaded because of missing matcher/action support in hw. That could be done via flow priorities/tables but these are only supported by a very limited number of NICs. That RSS flow would also need to be altered to only redirect on RXQs that are not reserved by the cp-protection flow. > > + /* User input for n_rxq (see netdev_dpdk_reconfigure). */ > > This comment is nice. minor: 'requested_n_rxq' could get a comment now > that it is no longer the user requested value and isn't the same as > the other requested_* members anymore. Sure that makes sense. > I have seen that ixgbe sets the error struct when returning success > which seems a violation of the rte_flow_validation/create API ("PMDs > initialize this structure in case of error only".) > > It's ok because you rightly ignore the error message, but it got me > thinking that we should reset/validate the error messages. See below. > > + set_error(&error, RTE_FLOW_ERROR_TYPE_NONE); Ack, I will add this. Thanks for the review. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
