| Message ID | 20201222000926.1054993-10-jonathan.lemon@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Generic zcopy_* functions | expand |
On Mon, Dec 21, 2020 at 7:09 PM Jonathan Lemon <jonathan.lemon@gmail.com> wrote: > > From: Jonathan Lemon <bsd@fb.com> > > Currently, an ubuf is attached to a new skb, the skb zc_flags > is initialized to a fixed value. Instead of doing this, set > the default zc_flags in the ubuf, and have new skb's inherit > from this default. > > This is needed when setting up different zerocopy types. > > Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com> > --- > include/linux/skbuff.h | 3 ++- > net/core/skbuff.c | 1 + > 2 files changed, 3 insertions(+), 1 deletion(-) > > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > index da0c1dddd0da..b90be4b0b2de 100644 > --- a/include/linux/skbuff.h > +++ b/include/linux/skbuff.h > @@ -478,6 +478,7 @@ struct ubuf_info { > }; > }; > refcount_t refcnt; > + u8 zc_flags; > > struct mmpin { > struct user_struct *user; When allocating ubuf_info for msg_zerocopy, we actually allocate the notification skb, to be sure that notifications won't be dropped due to memory pressure at notification time. We actually allocate the skb and place ubuf_info in skb->cb[]. The struct is exactly 48 bytes on 64-bit platforms, filling all of cb. This new field fills a 4B hole, so it should still be fine. Just being very explicit, as this is a fragile bit of code. Come to think of it, this probably deserves a BUILD_BUG_ON.
On Tue, Dec 22, 2020 at 10:00:37AM -0500, Willem de Bruijn wrote: > On Mon, Dec 21, 2020 at 7:09 PM Jonathan Lemon <jonathan.lemon@gmail.com> wrote: > > > > From: Jonathan Lemon <bsd@fb.com> > > > > Currently, an ubuf is attached to a new skb, the skb zc_flags > > is initialized to a fixed value. Instead of doing this, set > > the default zc_flags in the ubuf, and have new skb's inherit > > from this default. > > > > This is needed when setting up different zerocopy types. > > > > Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com> > > --- > > include/linux/skbuff.h | 3 ++- > > net/core/skbuff.c | 1 + > > 2 files changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > > index da0c1dddd0da..b90be4b0b2de 100644 > > --- a/include/linux/skbuff.h > > +++ b/include/linux/skbuff.h > > @@ -478,6 +478,7 @@ struct ubuf_info { > > }; > > }; > > refcount_t refcnt; > > + u8 zc_flags; > > > > struct mmpin { > > struct user_struct *user; > > When allocating ubuf_info for msg_zerocopy, we actually allocate the > notification skb, to be sure that notifications won't be dropped due > to memory pressure at notification time. We actually allocate the skb > and place ubuf_info in skb->cb[]. > > The struct is exactly 48 bytes on 64-bit platforms, filling all of cb. > This new field fills a 4B hole, so it should still be fine. Yes, I was careful not to increase the size. I have future changes for this structure, moving 'struct mmpin' into a union. Making the flags 16 bits shouldn't be a problem either. > Just being very explicit, as this is a fragile bit of code. Come to > think of it, this probably deserves a BUILD_BUG_ON. You mean like the one which exists in sock_zerocopy_alloc()? BUILD_BUG_ON(sizeof(*uarg) > sizeof(skb->cb));
On Tue, Dec 22, 2020 at 1:17 PM Jonathan Lemon <jonathan.lemon@gmail.com> wrote: > > On Tue, Dec 22, 2020 at 10:00:37AM -0500, Willem de Bruijn wrote: > > On Mon, Dec 21, 2020 at 7:09 PM Jonathan Lemon <jonathan.lemon@gmail.com> wrote: > > > > > > From: Jonathan Lemon <bsd@fb.com> > > > > > > Currently, an ubuf is attached to a new skb, the skb zc_flags > > > is initialized to a fixed value. Instead of doing this, set > > > the default zc_flags in the ubuf, and have new skb's inherit > > > from this default. > > > > > > This is needed when setting up different zerocopy types. > > > > > > Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com> > > > --- > > > include/linux/skbuff.h | 3 ++- > > > net/core/skbuff.c | 1 + > > > 2 files changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > > > index da0c1dddd0da..b90be4b0b2de 100644 > > > --- a/include/linux/skbuff.h > > > +++ b/include/linux/skbuff.h > > > @@ -478,6 +478,7 @@ struct ubuf_info { > > > }; > > > }; > > > refcount_t refcnt; > > > + u8 zc_flags; > > > > > > struct mmpin { > > > struct user_struct *user; > > > > When allocating ubuf_info for msg_zerocopy, we actually allocate the > > notification skb, to be sure that notifications won't be dropped due > > to memory pressure at notification time. We actually allocate the skb > > and place ubuf_info in skb->cb[]. > > > > The struct is exactly 48 bytes on 64-bit platforms, filling all of cb. > > This new field fills a 4B hole, so it should still be fine. > > Yes, I was careful not to increase the size. I have future changes > for this structure, moving 'struct mmpin' into a union. Making the > flags 16 bits shouldn't be a problem either. > > > > Just being very explicit, as this is a fragile bit of code. Come to > > think of it, this probably deserves a BUILD_BUG_ON. > > You mean like the one which exists in sock_zerocopy_alloc()? > > BUILD_BUG_ON(sizeof(*uarg) > sizeof(skb->cb)); Ah good. Yes. I should have remembered that was there.
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index da0c1dddd0da..b90be4b0b2de 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -478,6 +478,7 @@ struct ubuf_info { }; }; refcount_t refcnt; + u8 zc_flags; struct mmpin { struct user_struct *user; @@ -1454,7 +1455,7 @@ static inline void skb_zcopy_set(struct sk_buff *skb, struct ubuf_info *uarg, else skb_zcopy_get(uarg); skb_shinfo(skb)->destructor_arg = uarg; - skb_shinfo(skb)->zc_flags |= SKBZC_FRAGMENTS; + skb_shinfo(skb)->zc_flags |= uarg->zc_flags; } } diff --git a/net/core/skbuff.c b/net/core/skbuff.c index 8352da29f052..463078ba663f 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -1118,6 +1118,7 @@ struct ubuf_info *sock_zerocopy_alloc(struct sock *sk, size_t size) uarg->len = 1; uarg->bytelen = size; uarg->zerocopy = 1; + uarg->zc_flags = SKBZC_FRAGMENTS; refcount_set(&uarg->refcnt, 1); sock_hold(sk);