| Message ID | 07031c43d3e5c005fbfc76b60a58e30c66d7c620.1641863490.git.asml.silence@gmail.com (mailing list archive) |
|---|---|
| State | Deferred |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | udp optimisation | expand |
On Mon, Jan 10, 2022 at 8:25 PM Pavel Begunkov <asml.silence@gmail.com> wrote: > > During cork->dst setup, ip6_make_skb() gets an additional reference to > a passed in dst. However, udpv6_sendmsg() doesn't need dst after calling > ip6_make_skb(), and so we can save two additional atomics by passing > dst references to ip6_make_skb(). udpv6_sendmsg() is the only caller, so > it's enough to make sure it doesn't use dst afterwards. > > Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> > --- There are two patches 9/14 > net/ipv6/ip6_output.c | 9 ++++++--- > net/ipv6/udp.c | 3 ++- > 2 files changed, 8 insertions(+), 4 deletions(-) > > diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c > index 0cc490f2cfbf..6a7bba4dd04d 100644 > --- a/net/ipv6/ip6_output.c > +++ b/net/ipv6/ip6_output.c > @@ -1356,6 +1356,8 @@ static int ip6_setup_cork(struct sock *sk, struct inet_cork_full *cork, > unsigned int mtu; > struct ipv6_txoptions *nopt, *opt = ipc6->opt; > > + cork->base.dst = &rt->dst; > + Is there a reason to move this up from its original location next to the other cork initialization assignments? That the reference is taken in ip6_append_data for corked requests (once, in setup cork branch) and inherited from udpv6_send_skb otherwise is non-trivial. Worth a comment. > /* > * setup for corking > */ > @@ -1389,8 +1391,6 @@ static int ip6_setup_cork(struct sock *sk, struct inet_cork_full *cork, > > /* need source address above miyazawa*/ > } > - dst_hold(&rt->dst); > - cork->base.dst = &rt->dst; > v6_cork->hop_limit = ipc6->hlimit; > v6_cork->tclass = ipc6->tclass; > if (rt->dst.flags & DST_XFRM_TUNNEL) > @@ -1784,6 +1784,7 @@ int ip6_append_data(struct sock *sk, > /* > * setup for corking > */ > + dst_hold(&rt->dst); > err = ip6_setup_cork(sk, &inet->cork, &np->cork, > ipc6, rt); > if (err) > @@ -1974,8 +1975,10 @@ struct sk_buff *ip6_make_skb(struct sock *sk, > int exthdrlen = (ipc6->opt ? ipc6->opt->opt_flen : 0); > int err; > > - if (flags & MSG_PROBE) > + if (flags & MSG_PROBE) { > + dst_release(&rt->dst); > return NULL; > + } > > __skb_queue_head_init(&queue); > > diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c > index eec83e34ae27..3039dff7fe64 100644 > --- a/net/ipv6/udp.c > +++ b/net/ipv6/udp.c > @@ -1541,7 +1541,8 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len) > err = PTR_ERR(skb); > if (!IS_ERR_OR_NULL(skb)) > err = udp_v6_send_skb(skb, fl6, &cork.base); > - goto out; > + /* ip6_make_skb steals dst reference */ > + goto out_no_dst; > } > > lock_sock(sk); > -- > 2.34.1 >
On 1/11/22 15:39, Willem de Bruijn wrote: > On Mon, Jan 10, 2022 at 8:25 PM Pavel Begunkov <asml.silence@gmail.com> wrote: >> >> During cork->dst setup, ip6_make_skb() gets an additional reference to >> a passed in dst. However, udpv6_sendmsg() doesn't need dst after calling >> ip6_make_skb(), and so we can save two additional atomics by passing >> dst references to ip6_make_skb(). udpv6_sendmsg() is the only caller, so >> it's enough to make sure it doesn't use dst afterwards. >> >> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> >> --- > > There are two patches 9/14 Weird, thanks for letting know > >> net/ipv6/ip6_output.c | 9 ++++++--- >> net/ipv6/udp.c | 3 ++- >> 2 files changed, 8 insertions(+), 4 deletions(-) >> >> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c >> index 0cc490f2cfbf..6a7bba4dd04d 100644 >> --- a/net/ipv6/ip6_output.c >> +++ b/net/ipv6/ip6_output.c >> @@ -1356,6 +1356,8 @@ static int ip6_setup_cork(struct sock *sk, struct inet_cork_full *cork, >> unsigned int mtu; >> struct ipv6_txoptions *nopt, *opt = ipc6->opt; >> >> + cork->base.dst = &rt->dst; >> + > > Is there a reason to move this up from its original location next to > the other cork initialization assignments? ip6_setup_cork() consumes a dst ref now even in error cases, moved it to not patch up all error returns in there. On the other hand I can add dst_release() to callers when it failed. > That the reference is taken in ip6_append_data for corked requests > (once, in setup cork branch) and inherited from udpv6_send_skb > otherwise is non-trivial. Worth a comment. Will update in v2, thanks
On Tue, 2022-01-11 at 01:21 +0000, Pavel Begunkov wrote: > During cork->dst setup, ip6_make_skb() gets an additional reference to > a passed in dst. However, udpv6_sendmsg() doesn't need dst after calling > ip6_make_skb(), and so we can save two additional atomics by passing > dst references to ip6_make_skb(). udpv6_sendmsg() is the only caller, so > it's enough to make sure it doesn't use dst afterwards. What about the corked path in udp6_sendmsg()? I mean: udp6_sendmsg(MSG_MORE) -> ip6_append_data() -> ip6_setup_cork() what if ip6_setup_cork() errors out in that path? Thanks! Paolo
On 1/11/22 17:11, Paolo Abeni wrote: > On Tue, 2022-01-11 at 01:21 +0000, Pavel Begunkov wrote: >> During cork->dst setup, ip6_make_skb() gets an additional reference to >> a passed in dst. However, udpv6_sendmsg() doesn't need dst after calling >> ip6_make_skb(), and so we can save two additional atomics by passing >> dst references to ip6_make_skb(). udpv6_sendmsg() is the only caller, so >> it's enough to make sure it doesn't use dst afterwards. > > What about the corked path in udp6_sendmsg()? I mean: It doesn't change it for callers, so the ref stays with udp6_sendmsg() when corking. To compensate for ip6_setup_cork() there is an explicit dst_hold() in ip6_append_data, should be fine. @@ -1784,6 +1784,7 @@ int ip6_append_data(struct sock *sk, /* * setup for corking */ + dst_hold(&rt->dst); err = ip6_setup_cork(sk, &inet->cork, &np->cork, ipc6, rt); I don't care much about corking perf, but might be better to implement this "handing away" for ip6_append_data() as well to be more consistent with ip6_make_skb(). > udp6_sendmsg(MSG_MORE) -> ip6_append_data() -> ip6_setup_cork() > > what if ip6_setup_cork() errors out in that path?
On Tue, 2022-01-11 at 20:39 +0000, Pavel Begunkov wrote: > On 1/11/22 17:11, Paolo Abeni wrote: > > On Tue, 2022-01-11 at 01:21 +0000, Pavel Begunkov wrote: > > > During cork->dst setup, ip6_make_skb() gets an additional reference to > > > a passed in dst. However, udpv6_sendmsg() doesn't need dst after calling > > > ip6_make_skb(), and so we can save two additional atomics by passing > > > dst references to ip6_make_skb(). udpv6_sendmsg() is the only caller, so > > > it's enough to make sure it doesn't use dst afterwards. > > > > What about the corked path in udp6_sendmsg()? I mean: > > It doesn't change it for callers, so the ref stays with udp6_sendmsg() when > corking. To compensate for ip6_setup_cork() there is an explicit dst_hold() > in ip6_append_data, should be fine. Whoops, I underlooked that chunk, thanks for pointing it out! Yes, it looks fine. > @@ -1784,6 +1784,7 @@ int ip6_append_data(struct sock *sk, > /* > * setup for corking > */ > + dst_hold(&rt->dst); > err = ip6_setup_cork(sk, &inet->cork, &np->cork, > ipc6, rt); > > > I don't care much about corking perf, but might be better to implement > this "handing away" for ip6_append_data() as well to be more consistent > with ip6_make_skb(). I'm personally fine with the the added dst_hold() in ip6_append_data() Thanks! Paolo
On 1/12/22 11:15, Paolo Abeni wrote: > On Tue, 2022-01-11 at 20:39 +0000, Pavel Begunkov wrote: >> On 1/11/22 17:11, Paolo Abeni wrote: >>> On Tue, 2022-01-11 at 01:21 +0000, Pavel Begunkov wrote: >>>> During cork->dst setup, ip6_make_skb() gets an additional reference to >>>> a passed in dst. However, udpv6_sendmsg() doesn't need dst after calling >>>> ip6_make_skb(), and so we can save two additional atomics by passing >>>> dst references to ip6_make_skb(). udpv6_sendmsg() is the only caller, so >>>> it's enough to make sure it doesn't use dst afterwards. >>> >>> What about the corked path in udp6_sendmsg()? I mean: >> >> It doesn't change it for callers, so the ref stays with udp6_sendmsg() when >> corking. To compensate for ip6_setup_cork() there is an explicit dst_hold() >> in ip6_append_data, should be fine. > > Whoops, I underlooked that chunk, thanks for pointing it out! > > Yes, it looks fine. perfect, thanks >> @@ -1784,6 +1784,7 @@ int ip6_append_data(struct sock *sk, >> /* >> * setup for corking >> */ >> + dst_hold(&rt->dst); >> err = ip6_setup_cork(sk, &inet->cork, &np->cork, >> ipc6, rt); >> >> >> I don't care much about corking perf, but might be better to implement >> this "handing away" for ip6_append_data() as well to be more consistent >> with ip6_make_skb(). > > I'm personally fine with the the added dst_hold() in ip6_append_data()
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c index 0cc490f2cfbf..6a7bba4dd04d 100644 --- a/net/ipv6/ip6_output.c +++ b/net/ipv6/ip6_output.c @@ -1356,6 +1356,8 @@ static int ip6_setup_cork(struct sock *sk, struct inet_cork_full *cork, unsigned int mtu; struct ipv6_txoptions *nopt, *opt = ipc6->opt; + cork->base.dst = &rt->dst; + /* * setup for corking */ @@ -1389,8 +1391,6 @@ static int ip6_setup_cork(struct sock *sk, struct inet_cork_full *cork, /* need source address above miyazawa*/ } - dst_hold(&rt->dst); - cork->base.dst = &rt->dst; v6_cork->hop_limit = ipc6->hlimit; v6_cork->tclass = ipc6->tclass; if (rt->dst.flags & DST_XFRM_TUNNEL) @@ -1784,6 +1784,7 @@ int ip6_append_data(struct sock *sk, /* * setup for corking */ + dst_hold(&rt->dst); err = ip6_setup_cork(sk, &inet->cork, &np->cork, ipc6, rt); if (err) @@ -1974,8 +1975,10 @@ struct sk_buff *ip6_make_skb(struct sock *sk, int exthdrlen = (ipc6->opt ? ipc6->opt->opt_flen : 0); int err; - if (flags & MSG_PROBE) + if (flags & MSG_PROBE) { + dst_release(&rt->dst); return NULL; + } __skb_queue_head_init(&queue); diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c index eec83e34ae27..3039dff7fe64 100644 --- a/net/ipv6/udp.c +++ b/net/ipv6/udp.c @@ -1541,7 +1541,8 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len) err = PTR_ERR(skb); if (!IS_ERR_OR_NULL(skb)) err = udp_v6_send_skb(skb, fl6, &cork.base); - goto out; + /* ip6_make_skb steals dst reference */ + goto out_no_dst; } lock_sock(sk);
During cork->dst setup, ip6_make_skb() gets an additional reference to a passed in dst. However, udpv6_sendmsg() doesn't need dst after calling ip6_make_skb(), and so we can save two additional atomics by passing dst references to ip6_make_skb(). udpv6_sendmsg() is the only caller, so it's enough to make sure it doesn't use dst afterwards. Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> --- net/ipv6/ip6_output.c | 9 ++++++--- net/ipv6/udp.c | 3 ++- 2 files changed, 8 insertions(+), 4 deletions(-)