On Wed, Feb 5, 2025 at 4:20 AM Pavel Begunkov <asml.silence@xxxxxxxxx> wrote:
>
> On 2/3/25 22:39, Mina Almasry wrote:
> ...
> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > index bb2b751d274a..3ff8f568c382 100644
> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -1711,9 +1711,12 @@ struct ubuf_info *msg_zerocopy_realloc(struct sock *sk, size_t size,
> ...
> > int zerocopy_fill_skb_from_iter(struct sk_buff *skb,
> > struct iov_iter *from, size_t length);
> > @@ -1721,12 +1724,14 @@ int zerocopy_fill_skb_from_iter(struct sk_buff *skb,
> > static inline int skb_zerocopy_iter_dgram(struct sk_buff *skb,
> > struct msghdr *msg, int len)
> > {
> > - return __zerocopy_sg_from_iter(msg, skb->sk, skb, &msg->msg_iter, len);
> > + return __zerocopy_sg_from_iter(msg, skb->sk, skb, &msg->msg_iter, len,
> > + NULL);
>
> Instead of propagating it all the way down and carving a new path, why
> not reuse the existing infra? You already hook into where ubuf is
> allocated, you can stash the binding in there. And
It looks like it's not possible to increase the side of ubuf_info at
all, otherwise the BUILD_BUG_ON in msg_zerocopy_alloc() fires.
It's asserting that sizeof(ubuf_info_msgzc) <= sizeof(skb->cb), and
I'm guessing increasing skb->cb size is not really the way to go.
What I may be able to do here is stash the binding somewhere in
ubuf_info_msgzc via union with fields we don't need for devmem, and/or
stashing the binding in ubuf_info_ops (very hacky). Neither approach
seems ideal, but the former may work and may be cleaner.
I'll take a deeper look here. I had looked before and concluded that
we're piggybacking devmem TX on MSG_ZEROCOPY path, because we need
almost all of the functionality there (no copying, send complete
notifications, etc), with one minor change in the skb filling. I had
concluded that if MSG_ZEROCOPY was never updated to use the existing
infra, then it's appropriate for devmem TX piggybacking on top of it
to follow that. I would not want to get into a refactor of
MSG_ZEROCOPY for no real reason.
But I'll take a deeper look here and see if I can make something
slightly cleaner work.
> zerocopy_fill_skb_from_devmem can implement ->sg_from_iter,
> see __zerocopy_sg_from_iter().
>
> ...
> > diff --git a/net/core/datagram.c b/net/core/datagram.c
> > index f0693707aece..c989606ff58d 100644
> > --- a/net/core/datagram.c
> > +++ b/net/core/datagram.c
> > @@ -63,6 +63,8 @@
> > +static int
> > +zerocopy_fill_skb_from_devmem(struct sk_buff *skb, struct iov_iter *from,
> > + int length,
> > + struct net_devmem_dmabuf_binding *binding)
> > +{
> > + int i = skb_shinfo(skb)->nr_frags;
> > + size_t virt_addr, size, off;
> > + struct net_iov *niov;
> > +
> > + while (length && iov_iter_count(from)) {
> > + if (i == MAX_SKB_FRAGS)
> > + return -EMSGSIZE;
> > +
> > + virt_addr = (size_t)iter_iov_addr(from);
>
> Unless I missed it somewhere it needs to check that the iter
> is iovec based.
>
How do we end up here with an iterator that is not iovec based? Is the
user able to trigger that somehow and I missed it?
--
Thanks,
Mina
