Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210919154337.9243-1-stachecki.tyler@gmail.com> <CAM_iQpUeH7sOWRDfaAsER4HdRJQUuODB-ht0NyEZgnCXEmm2RQ@mail.gmail.com>
In-Reply-To: <CAM_iQpUeH7sOWRDfaAsER4HdRJQUuODB-ht0NyEZgnCXEmm2RQ@mail.gmail.com>
From:   Tyler Stachecki <stachecki.tyler@gmail.com>
Date:   Mon, 20 Sep 2021 01:44:29 -0400
Message-ID: <CAC6wqPVopqP=erynBazgRh2QkB7=Nh-cXxLS2ZvmTzb7E0Gvaw@mail.gmail.com>
Subject: Re: [PATCH] ovs: Only clear tstamp when changing namespaces
To:     Cong Wang <xiyou.wangcong@gmail.com>
Cc:     fankaixi.li@bytedance.com, xiexiaohui.xxh@bytedance.com,
        "Cong Wang ." <cong.wang@bytedance.com>,
        Pravin B Shelar <pshelar@ovn.org>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Linux Kernel Network Developers <netdev@vger.kernel.org>,
        dev@openvswitch.org, LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sun, Sep 19, 2021 at 7:33 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Sun, Sep 19, 2021 at 10:59 AM Tyler J. Stachecki
> <stachecki.tyler@gmail.com> wrote:
> >
> > As of "ovs: clear skb->tstamp in forwarding path", the
> > tstamp is now being cleared unconditionally to fix fq qdisc
> > operation with ovs vports.
> >
> > While this is mostly correct and fixes forwarding for that
> > use case, a slight adjustment is necessary to ensure that
> > the tstamp is cleared *only when the forwarding is across
> > namespaces*.
>
> Hmm? I am sure timestamp has already been cleared when
> crossing netns:
>
> void skb_scrub_packet(struct sk_buff *skb, bool xnet)
> {
> ...
>         if (!xnet)
>                 return;
>
>         ipvs_reset(skb);
>         skb->mark = 0;
>         skb->tstamp = 0;
> }
>
> So, what are you trying to fix?
>
> >
> > Signed-off-by: Tyler J. Stachecki <stachecki.tyler@gmail.com>
> > ---
> >  net/openvswitch/vport.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/openvswitch/vport.c b/net/openvswitch/vport.c
> > index cf2ce5812489..c2d32a5c3697 100644
> > --- a/net/openvswitch/vport.c
> > +++ b/net/openvswitch/vport.c
> > @@ -507,7 +507,8 @@ void ovs_vport_send(struct vport *vport, struct sk_buff *skb, u8 mac_proto)
> >         }
> >
> >         skb->dev = vport->dev;
> > -       skb->tstamp = 0;
> > +       if (dev_net(skb->dev))
>
> Doesn't dev_net() always return a non-NULL pointer?
>
> If you really want to check whether it is cross-netns, you should
> use net_eq() to compare src netns with dst netns, something like:
> if (!net_eq(dev_net(vport->dev), dev_net(skb->dev))).
>
> Thanks.

Sorry if this is a no-op -- I'm admittedly not familiar with this part
of the tree.  I had added this check based on this discussion on the
OVS mailing list:
https://mail.openvswitch.org/pipermail/ovs-discuss/2021-February/050966.html

The motivation to add it was based on the fact that skb_scrub_packet
is doing it conditionally as well, but you seem to indicate that
skb_scrub_packet itself is already being done somewhere?

Cheers,
Tyler