| Message ID | 20210919154337.9243-1-stachecki.tyler@gmail.com |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
| Series | [ovs-dev] ovs: Only clear tstamp when changing namespaces | expand |
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.
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
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.
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;
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(-)