| Message ID | 20240414195213.106209-1-yick.xie@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] udp: don't be set unconnected if only UDP cmsg | expand |
Yick Xie wrote: > If "udp_cmsg_send()" returned 0 (i.e. only UDP cmsg), > "connected" should not be set to 0. Otherwise it stops > the connected socket from using the cached route. > > Signed-off-by: Yick Xie <yick.xie@gmail.com> This either needs to target net-next, or have Fixes: 2e8de8576343 ("udp: add gso segment cmsg") I think it can be argued either way. This situation existed from the start, and is true for other cmsg that don't affect routing as well. If the impact of the route lookup is significant, it couls be argued to be a performance bug. I steer towards net-next. In which case it would be nice to also move the ipc.opt branch and perhaps even exclude other common cmsgs, such as SCM_TXTIME and SCM_TIMESTAMPING.
On Mon, Apr 15, 2024 at 10:11 PM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > Yick Xie wrote: > > If "udp_cmsg_send()" returned 0 (i.e. only UDP cmsg), > > "connected" should not be set to 0. Otherwise it stops > > the connected socket from using the cached route. > > > > Signed-off-by: Yick Xie <yick.xie@gmail.com> > > This either needs to target net-next, or have > > Fixes: 2e8de8576343 ("udp: add gso segment cmsg") I should have added that, sorry for the mess. > I think it can be argued either way. This situation existed from the > start, and is true for other cmsg that don't affect routing as well. > > If the impact of the route lookup is significant, it couls be argued > to be a performance bug. With sendmsg(), any smaller gso_size could be picked up dynamically. Then it depends, "ip_route_output_flow()" could be as expensive as "ip_make_skb()". > I steer towards net-next. In which case it would be nice to also > move the ipc.opt branch and perhaps even exclude other common cmsgs, > such as SCM_TXTIME and SCM_TIMESTAMPING. Both are fine. Though could it be better to take an easy backporting at first?
Yick Xie wrote: > On Mon, Apr 15, 2024 at 10:11 PM Willem de Bruijn > <willemdebruijn.kernel@gmail.com> wrote: > > > > Yick Xie wrote: > > > If "udp_cmsg_send()" returned 0 (i.e. only UDP cmsg), > > > "connected" should not be set to 0. Otherwise it stops > > > the connected socket from using the cached route. > > > > > > Signed-off-by: Yick Xie <yick.xie@gmail.com> > > > > This either needs to target net-next, or have > > > > Fixes: 2e8de8576343 ("udp: add gso segment cmsg") > > I should have added that, sorry for the mess. No worries. > > I think it can be argued either way. This situation existed from the > > start, and is true for other cmsg that don't affect routing as well. > > > > If the impact of the route lookup is significant, it couls be argued > > to be a performance bug. > > With sendmsg(), any smaller gso_size could be picked up dynamically. > Then it depends, "ip_route_output_flow()" could be as expensive as > "ip_make_skb()". > > > I steer towards net-next. In which case it would be nice to also > > move the ipc.opt branch and perhaps even exclude other common cmsgs, > > such as SCM_TXTIME and SCM_TIMESTAMPING. > > Both are fine. Though could it be better to take an easy backporting at first? Sounds good. Please just resend with the Fixes tag.
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c index c02bf011d4a6..420905be5f30 100644 --- a/net/ipv4/udp.c +++ b/net/ipv4/udp.c @@ -1123,16 +1123,17 @@ int udp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len) if (msg->msg_controllen) { err = udp_cmsg_send(sk, msg, &ipc.gso_size); - if (err > 0) + if (err > 0) { err = ip_cmsg_send(sk, msg, &ipc, sk->sk_family == AF_INET6); + connected = 0; + } if (unlikely(err < 0)) { kfree(ipc.opt); return err; } if (ipc.opt) free = 1; - connected = 0; } if (!ipc.opt) { struct ip_options_rcu *inet_opt;
If "udp_cmsg_send()" returned 0 (i.e. only UDP cmsg), "connected" should not be set to 0. Otherwise it stops the connected socket from using the cached route. Signed-off-by: Yick Xie <yick.xie@gmail.com> --- net/ipv4/udp.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)