Message-ID: <672781359b9d1_312cad29465@willemb.c.googlers.com.notmuch>
Date: Sun, 03 Nov 2024 08:57:09 -0500
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Anna Nyiri <annaemesenyiri@...il.com>, 
 Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: netdev@...r.kernel.org, 
 fejes@....elte.hu, 
 edumazet@...gle.com, 
 kuba@...nel.org, 
 pabeni@...hat.com
Subject: Re: [PATCH net-next v2 2/2] support SO_PRIORITY cmsg

Anna Nyiri wrote:
> Willem de Bruijn <willemdebruijn.kernel@...il.com> ezt írta (időpont:
> 2024. nov. 3., V, 2:27):
> >
> > Anna Emese Nyiri wrote:
> > > The Linux socket API currently supports setting SO_PRIORITY at the
> > > socket level, which applies a uniform priority to all packets sent
> > > through that socket. The only exception is IP_TOS; if specified as
> > > ancillary data, the packet does not inherit the socket's priority.
> > > Instead, the priority value is computed when handling the ancillary
> > > data (as implemented in commit <f02db315b8d88>
> >
> > nit: drop the brackets
> >
> > > ("ipv4: IP_TOS and IP_TTL can be specified as ancillary data")). If
> > > the priority is set via IP_TOS, then skb->priority derives its value
> > > from the rt_tos2priority function, which calculates the priority
> > > based on the value of ipc->tos obtained from IP_TOS. However, if
> > > IP_TOS is not used and the priority has been set through a control
> > > message, skb->priority will take the value provided by that control
> > > message.
> >
> > The above describes the new situation? There is no way to set
> > priority to a control message prior to this patch.
> >
> > > Therefore, when both options are available, the primary
> > > source for skb->priority is the value set via IP_TOS.
> > >
> > > Currently, there is no option to set the priority directly from
> > > userspace on a per-packet basis. The following changes allow
> > > SO_PRIORITY to be set through control messages (CMSG), giving
> > > userspace applications more granular control over packet priorities.
> > >
> > > This patch enables setting skb->priority using CMSG. If SO_PRIORITY
> >
> > Duplicate statement. Overall, the explanation can perhaps be
> > condensed and made more clear.
> >
> > > is specified as ancillary data, the packet is sent with the priority
> > > value set through sockc->priority_cmsg_value, overriding the
> >
> > No longer matches the code.
> >
> > > socket-level values set via the traditional setsockopt() method. This
> > > is analogous to existing support for SO_MARK (as implemented in commit
> > > <c6af0c227a22> ("ip: support SO_MARK cmsg")).
> > >
> > > Suggested-by: Ferenc Fejes <fejes@....elte.hu>
> > > Signed-off-by: Anna Emese Nyiri <annaemesenyiri@...il.com>
> > > ---
> > >  include/net/inet_sock.h | 2 +-
> > >  include/net/ip.h        | 3 ++-
> > >  include/net/sock.h      | 4 +++-
> > >  net/can/raw.c           | 2 +-
> > >  net/core/sock.c         | 8 ++++++++
> > >  net/ipv4/ip_output.c    | 7 +++++--
> > >  net/ipv4/raw.c          | 2 +-
> > >  net/ipv6/ip6_output.c   | 3 ++-
> > >  net/ipv6/raw.c          | 2 +-
> > >  net/packet/af_packet.c  | 2 +-
> > >  10 files changed, 25 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/include/net/inet_sock.h b/include/net/inet_sock.h
> > > index 56d8bc5593d3..3ccbad881d74 100644
> > > --- a/include/net/inet_sock.h
> > > +++ b/include/net/inet_sock.h
> > > @@ -172,7 +172,7 @@ struct inet_cork {
> > >       u8                      tx_flags;
> > >       __u8                    ttl;
> > >       __s16                   tos;
> > > -     char                    priority;
> > > +     u32                     priority;
> >
> > Let's check with pahole how this affects struct size and holes.
> > It likely adds a hole, but unavoidably so.
> >
> > >       __u16                   gso_size;
> > >       u32                     ts_opt_id;
> > >       u64                     transmit_time;
> > > diff --git a/include/net/ip.h b/include/net/ip.h
> > > index 0e548c1f2a0e..e8f71a191277 100644
> > > --- a/include/net/ip.h
> > > +++ b/include/net/ip.h
> > > @@ -81,7 +81,7 @@ struct ipcm_cookie {
> > >       __u8                    protocol;
> > >       __u8                    ttl;
> > >       __s16                   tos;
> > > -     char                    priority;
> > > +     u32                     priority;
> >
> > No need for a field in ipcm_cookie, when also present in
> > sockcm_cookie. As SO_PRIORITY is not limited to IP, sockcm_cookie is
> > the right location.
> 
> I think there could be a problem if the priority is set by IP_TOS for
> some reason, and then also via cmsg. The latter value may overwrite
> it. In the ip_setup_cork() function, there is therefore a check for
> the value cork->tos != -1 to give priority to the value set by IP_TOS.
> And that's why I thought that there should be a priority field in both
> ipcm_cookie and sockcm_cookie. The priority field already existed in
> ipcm_cookie, I didn't add it. I just changed the type.

The existing behavior that adds a branch in the hot path is actually
not needed.

The preferred pattern is that the cookie is initialized with the sk
field, and then optionally overwritten when parsing cmsgs.

The path is slightly complicated by the fact that ipcm_init_sk does
not call sockcm_init, but more or less open codes that.

The callers of ipcm_init_sk are datagram sockets that have more
opportunities to override per-socket options on a per-packet basis.

So I was wrong that the field only has to be initialized in
sockcm_init. It will have to be initialized by both initializers.

But still only a u32 single field is needed.

Powered by blists - more mailing lists