On 7/14/22 14:23, Roi Dayan wrote: > > > On 2022-07-13 7:46 PM, Ilya Maximets wrote: >> 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 <i.maxim...@ovn.org> >>>>>>>>> >>>>>>>>> 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 <echau...@redhat.com> >>>>>>>>> >>>>>>>> >>>>>>>> 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 <r...@nvidia.com> >>>>>> >>>>>> >>>>>> 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? > > The drivers do reject such rules to avoid the issue described.
So the behavior is correct, it just won't be offloaded. OK. > >> >> 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 didn't understand the question. what original flow? > with this commit the match doesn't pass tp_dst match to tc. I meant 'tp_dst' match in the first datapath flow that matches on the encapsulated packet and sends it to a tunnel port for decapsulation. The second flow that matches on tunnel metadata after decapsulation will have tunnel metadata tp_dst with a zero mask, while the first flow will likely have an exact match on the tp_dst, because it was used to decide to which tunnel port this packet should go for decapsulation. I just was thinking that if the driver/HW can view these two flows as one, there should be sufficient data to not swallow the unrelated traffic. But I don't really know that looks like on a driver/HW level. > >>> >>>>> >>>>> 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 d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
