All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ovs: Only clear tstamp when changing namespaces
@ 2021-09-19 15:43 Tyler J. Stachecki
  2021-09-19 23:33 ` Cong Wang
  0 siblings, 1 reply; 4+ messages in thread
From: Tyler J. Stachecki @ 2021-09-19 15:43 UTC (permalink / raw)
  Cc: fankaixi.li, stachecki.tyler, xiexiaohui.xxh, cong.wang,
	Pravin B Shelar, David S. Miller, Jakub Kicinski, netdev, dev,
	linux-kernel

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*.

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))
+		skb->tstamp = 0;
 	vport->ops->send(skb);
 	return;
 
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] ovs: Only clear tstamp when changing namespaces
  2021-09-19 15:43 [PATCH] ovs: Only clear tstamp when changing namespaces Tyler J. Stachecki
@ 2021-09-19 23:33 ` Cong Wang
  2021-09-20  5:44   ` Tyler Stachecki
  0 siblings, 1 reply; 4+ messages in thread
From: Cong Wang @ 2021-09-19 23:33 UTC (permalink / raw)
  To: Tyler J. Stachecki
  Cc: fankaixi.li, xiexiaohui.xxh, Cong Wang .,
	Pravin B Shelar, David S. Miller, Jakub Kicinski,
	Linux Kernel Network Developers, dev, LKML

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.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] ovs: Only clear tstamp when changing namespaces
  2021-09-19 23:33 ` Cong Wang
@ 2021-09-20  5:44   ` Tyler Stachecki
  2021-09-20 16:40     ` Cong Wang
  0 siblings, 1 reply; 4+ messages in thread
From: Tyler Stachecki @ 2021-09-20  5:44 UTC (permalink / raw)
  To: Cong Wang
  Cc: fankaixi.li, xiexiaohui.xxh, Cong Wang .,
	Pravin B Shelar, David S. Miller, Jakub Kicinski,
	Linux Kernel Network Developers, dev, LKML

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] ovs: Only clear tstamp when changing namespaces
  2021-09-20  5:44   ` Tyler Stachecki
@ 2021-09-20 16:40     ` Cong Wang
  0 siblings, 0 replies; 4+ messages in thread
From: Cong Wang @ 2021-09-20 16:40 UTC (permalink / raw)
  To: Tyler Stachecki
  Cc: fankaixi.li, xiexiaohui.xxh, Cong Wang .,
	Pravin B Shelar, David S. Miller, Jakub Kicinski,
	Linux Kernel Network Developers, dev, LKML

On Sun, Sep 19, 2021 at 10:44 PM Tyler Stachecki
<stachecki.tyler@gmail.com> wrote:
> 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?

I mean, skb->tstamp has been cleared when crossing netns,
so: 1) you don't need to clear it again for this case; 2) clearly we
fix other cases with commit 01634047bf0d, so your patch break
it again.

Thanks.

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-09-20 16:41 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-19 15:43 [PATCH] ovs: Only clear tstamp when changing namespaces Tyler J. Stachecki
2021-09-19 23:33 ` Cong Wang
2021-09-20  5:44   ` Tyler Stachecki
2021-09-20 16:40     ` Cong Wang

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.