mpls_xmit() calls skb_orphan()

mpls_xmit() calls skb_orphan()

[Christoph Paasch]

Hello,

we observed an issue when running a TCP-connection with BBR on top of an MPLS-tunnel in that we saw a lot of CPU-time spent coming from tcp_pace_kick(), although sch_fq was configured on this host.

The reason for this seems to be because mpls_xmit() calls skb_orphan(), thus settings skb->sk to NULL, preventing the qdisc to set sk_pacing_status (which would allow to avoid the call to tcp_pace_kick()).

The question is: Why is this call to skb_orphan in mpls_xmit necessary ?


Thanks,
Christoph

Re: mpls_xmit() calls skb_orphan()

[Christoph Paasch]

> On Dec 8, 2023, at 1:06 PM, Christoph Paasch <cpaasch@apple.com> wrote:
> 
> Hello,
> 
> we observed an issue when running a TCP-connection with BBR on top of an MPLS-tunnel in that we saw a lot of CPU-time spent coming from tcp_pace_kick(), although sch_fq was configured on this host.
> 
> The reason for this seems to be because mpls_xmit() calls skb_orphan(), thus settings skb->sk to NULL, preventing the qdisc to set sk_pacing_status (which would allow to avoid the call to tcp_pace_kick()).
> 
> The question is: Why is this call to skb_orphan in mpls_xmit necessary ?
> 
> 
> Thanks,
> Christoph

Resend in plain-text. Sorry about that!


Christoph

Re: mpls_xmit() calls skb_orphan()

[Jakub Kicinski]

Updating Roopa's email.

On Fri, 08 Dec 2023 13:15:04 -0800 Christoph Paasch wrote:
> > On Dec 8, 2023, at 1:06 PM, Christoph Paasch <cpaasch@apple.com> wrote:
> > 
> > Hello,
> > 
> > we observed an issue when running a TCP-connection with BBR on top of an MPLS-tunnel in that we saw a lot of CPU-time spent coming from tcp_pace_kick(), although sch_fq was configured on this host.
> > 
> > The reason for this seems to be because mpls_xmit() calls skb_orphan(), thus settings skb->sk to NULL, preventing the qdisc to set sk_pacing_status (which would allow to avoid the call to tcp_pace_kick()).
> > 
> > The question is: Why is this call to skb_orphan in mpls_xmit necessary ?
> > 
> > 
> > Thanks,
> > Christoph  
> 
> Resend in plain-text. Sorry about that!

Re: mpls_xmit() calls skb_orphan()

[Paolo Abeni]

Reviving this old thread, as I stumbled upon it again...

On Fri, 2023-12-08 at 13:06 -0800, Christoph Paasch wrote:
> we observed an issue when running a TCP-connection with BBR on top 
> of an MPLS-tunnel in that we saw a lot of CPU-time spent coming 
> from tcp_pace_kick(), although sch_fq was configured on this host.
> 
> The reason for this seems to be because mpls_xmit() calls skb_orphan(), 
> thus settings skb->sk to NULL, preventing the qdisc to set 
> sk_pacing_status (which would allow to avoid the call to tcp_pace_kick()).
> 
> The question is: Why is this call to skb_orphan in mpls_xmit necessary ?

I guess the skb_orphan() call initially landed into mpls_xmit() to
provide isolation in case such xmit would correspond to netns crossing.

We had a similar situation for most IP tunnels before commit
9c4c325252c5 ("skbuff: preserve sock reference when scrubbing the
skb."). 

According to such commit changelog the skb socket reference is handled
carefully WRT netns boundaries elsewhere in the net stack, the
orphaning on TX is not required.

TL;DR: I believe removing the mentioned skb_orphan() is safe.

Cheers,

Paolo