Thanks Roi, On Thu, 31 Jan 2019 at 15:52, Roi Dayan <[email protected]> wrote:
> > > On 31/01/2019 15:32, Roi Dayan wrote: > > > > On 31/01/2019 11:58, Simon Horman wrote: > >> On Mon, Jan 21, 2019 at 05:32:37PM +0200, Adi Nissim wrote: > >>> Currently the TC tunnel_key action is always > >>> initialized with the given tunnel id value. However, > >>> some tunneling protocols define the tunnel id as an optional field. > >>> > >>> This patch initializes the id field of tunnel_key:set and > tunnel_key:unset > >>> only if a value is provided. > >>> > >>> In the case that a tunnel key value is not provided by the user > >>> the key flag will not be set. > >>> > >>> Signed-off-by: Adi Nissim <[email protected]> > >>> Acked-by: Paul Blakey <[email protected]> > >>> --- > >>> v1->v2: check if mask.tunnel.id == OVS_BE64_MAX > >>> so we won't do match in the case of a partial mask. > >> I am still a bit concerned about the partial mask case. > >> It looks like the code will now silently not offload such matches. > >> > >> I think that a partial mask should either be offloaded or > >> offload of the entire flow should be rejected. > > thanks. you are right. I didn't notice it. partial masks should be > rejected > > to fallback to ovs dp instead of ignoring the mask. > > > > > Hi Simon, > > I did some checks and think the correct fix is to offload exact match. > if key is partial we can ignore the mask and offload exact match and > it will be correct as we do more strict matching. > > it is also documented that the kernel datapath is doing the same > (from datapath.rst) > > "The kernel can ignore the mask attribute, installing an exact > match flow" > > So I think the first patch V0 is actually correct as we > check the tunnel key flag exists and offload exact match if > there was any mask or offload without a key if the mask is 0 > or no key. > > in netdev-tc-offloads.c > > + flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? > + tnl_mask->tun_id : 0; > I think this is fine so long as tun_id is all-ones. Is that always the case? Should the code check that it is the case? Am I missing the point? > > > in tc.c > > - nl_msg_put_be32(request, TCA_FLOWER_KEY_ENC_KEY_ID, id); > + if (id_mask) { > + nl_msg_put_be32(request, TCA_FLOWER_KEY_ENC_KEY_ID, id); > + } > > > let me know what you think. > > Thanks, > Roi > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
