On 7/12/22 11:47, Roi Dayan wrote: > > > On 2022-07-12 12:23 PM, Ilya Maximets wrote: >> On 7/7/22 09:01, Roi Dayan wrote: >>> >>> >>> On 2022-07-07 9:48 AM, Roi Dayan wrote: >>>> >>>> >>>> On 2022-07-06 5:00 PM, Roi Dayan wrote: >>>>> >>>>> >>>>> On 2022-07-06 3:34 PM, Roi Dayan wrote: >>>>>> >>>>>> >>>>>> On 2022-07-06 1:26 PM, Eelco Chaudron wrote: >>>>>>> >>>>>>> >>>>>>> On 5 Jul 2022, at 0:45, Ilya Maximets wrote: >>>>>>> >>>>>>>> TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK is supported by TC, and OVS may >>>>>>>> create a masked match on this field. >>>>>>>> >>>>>>>> This change is important as we're not clearing the masks which wasn't >>>>>>>> really used, so if OVS requests match on ports, we should use the >>>>>>>> mask and clear, otherwise offloading will fail. >>>>>>>> >>>>>>>> Signed-off-by: Ilya Maximets <[email protected]> >>>>>>> >>>>>>> Changes look good to me, and all system-traffic.at tests that were >>>>>>> passing without the change are still passing, including the failing >>>>>>> ERSPAN ones. >>>>>>> >>>>>>> Acked-by: Eelco Chaudron <[email protected]> >>>>>>> >>>>>> >>>>>> can we postpone to merge this? >>>>>> After this patch simple vxlan rules won't have enc dst port on decap >>>>>> rules and currently mlx5 driver must have enc dst port to do the >>>>>> offload of the decap rule. >>>>>> I'm checking about this limitation and if we can remove it but >>>>>> if we can postpone a bit so we won't break users with our nics.. >>>>>> >>>>>> thanks, >>>>>> Roi >>>>> >>>>> >>>>> I finished with the testing and checking I needed. >>>>> I don't have a problem with the patch. if something else will raise i'll >>>>> start a different discussion. >>>>> >>>>> Acked-by: Roi Dayan <[email protected]> >>>> >>>> >>>> I have a concern now. today for an offload driver to offload >>>> a tunnel decap rule it registers to TC indirect callback. >>>> >>>> Adding a TC rule on a tunnel device calls all registered drivers >>>> to offload the rule. >>>> With a single tunnel on a device this is ok but if a system have multiple >>>> tunnels with same src/dst ip but different enc dst port, >>>> adding a tc rule on one of the tunnel devices without matching >>>> enc dst port will call a driver to offload such a rule in the hw, >>>> but that rule is potentially matching all sw tunnels. a driver >>>> will have to match implicit on the the enc_dst_port to distinguish >>>> between the tunnels. >>>> >>>> OVS doesn't match explicit on enc_dst_port also when having multiple >>>> tunnels as the in SW the packets will be on the correct >>>> tunnel device and then pass tc or ovs datapath. >>>> >>>> After this patch offload drivers must to implicit match the >>>> enc_dst_port to avoid having hw rule matching all the tunnels >>>> with same src/dst ip. taking the tunnel maybe. >>>> e.g. for vxlan tunnel from vxlan->cfg.dst_port >>>> >>>> What do you think? >>> >>> to support what i said about implicit match for geneve and mpls for >>> example will require kernel change to actually expose their struct >>> in a header file. >> >> But the match contains the input port (tunnel port) right? And that >> should be sufficient to distinguish tunnels. Does it mean that HW >> offload in the kernel ignores that match? Or am I reading that wrong? >> > > in sw the matching of the rules is done after the packet is already > placed on the sw port. so rule on vxlan1 will patch only packets on > vxlan1. > > for offloaded rule, the input port is ignored for tunnels as once > offloaded, the hw match an incoming packet and is not aware of the > sw ports (tunnels).
Sounds like an architectural bug in the kernel. Should the driver reject offload of rules with non-exact match on tunnel dst port in this case? OTOH, doesn't the original flow that is matching on the tunnel packet have a match on tp_dst? I'm not 100% sure that is always true though, but it probably is in most cases. > >>> >>> I think we should keep ovs TC to keep explicit match on enc_dst_port. >>> We can add a comment explaining why. >> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
