[net-next,4/5] net/socket: add helpers for recvmmsg

_skb_try_recv_datagram_batch dequeues multiple skb's from the
socket's receive queue, and runs the bulk_destructor callback under
the receive queue lock.

recvmmsg_ctx_from_user retrieves msghdr information from userspace,
and sets up the kernelspace context for processing one datagram.

recvmmsg_ctx_to_user copies to userspace the results of processing one
datagram.

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 include/linux/skbuff.h | 20 ++++++++++++++++
 include/net/sock.h     | 19 +++++++++++++++
 net/core/datagram.c    | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++
 net/socket.c           | 60 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 164 insertions(+)

Hi Paolo,

[auto build test WARNING on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Paolo-Abeni/net-add-protocol-level-recvmmsg-support/20161126-033729
config: i386-randconfig-i1-201647 (attached as .config)
compiler: gcc-4.8 (Debian 4.8.4-1) 4.8.4
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

   net/socket.c: In function 'recvmmsg_ctx_to_user':
>> net/socket.c:2263:11: warning: passing argument 3 of 'copy_msghdr_to_user_gen' from incompatible pointer type [enabled by default]
              ctx->uaddr, &ctx->addr, ctx->cmsg_ptr);
              ^
   net/socket.c:2115:12: note: expected 'struct msghdr *' but argument is of type 'struct user_msghdr *'
    static int copy_msghdr_to_user_gen(struct msghdr *msg_sys, int flags,
               ^

vim +/copy_msghdr_to_user_gen +2263 net/socket.c

  2247			return 0;
  2248	
  2249		return security_socket_recvmsg(sk->sk_socket, &ctx->msg_sys,
  2250					      msg_data_left(&ctx->msg_sys), flags);
  2251	}
  2252	
  2253	int recvmmsg_ctx_to_user(struct mmsghdr __user **mmsg_ptr, int len,
  2254				 unsigned int flags, struct recvmmsg_ctx *ctx)
  2255	{
  2256		struct compat_mmsghdr __user *mmsg_compat;
  2257		struct mmsghdr __user *mmsg = *mmsg_ptr;
  2258		int err;
  2259	
  2260		mmsg_compat = (struct compat_mmsghdr __user *)mmsg;
  2261		err = copy_msghdr_to_user_gen(&ctx->msg_sys, flags,
  2262					      &mmsg_compat->msg_hdr, &mmsg->msg_hdr,
> 2263					      ctx->uaddr, &ctx->addr, ctx->cmsg_ptr);
  2264		if (err)
  2265			return err;
  2266	
  2267		if (MSG_CMSG_COMPAT & flags) {
  2268			err = __put_user(len, &mmsg_compat->msg_len);
  2269			if (err < 0)
  2270				return err;
  2271	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Hi Paolo,

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Paolo-Abeni/net-add-protocol-level-recvmmsg-support/20161126-033729
config: i386-randconfig-s0-201647 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   net/socket.c: In function 'recvmmsg_ctx_to_user':
>> net/socket.c:2262:11: error: passing argument 3 of 'copy_msghdr_to_user_gen' from incompatible pointer type [-Werror=incompatible-pointer-types]
              &mmsg_compat->msg_hdr, &mmsg->msg_hdr,
              ^
   net/socket.c:2115:12: note: expected 'struct msghdr *' but argument is of type 'struct user_msghdr *'
    static int copy_msghdr_to_user_gen(struct msghdr *msg_sys, int flags,
               ^~~~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/copy_msghdr_to_user_gen +2262 net/socket.c

  2256		struct compat_mmsghdr __user *mmsg_compat;
  2257		struct mmsghdr __user *mmsg = *mmsg_ptr;
  2258		int err;
  2259	
  2260		mmsg_compat = (struct compat_mmsghdr __user *)mmsg;
  2261		err = copy_msghdr_to_user_gen(&ctx->msg_sys, flags,
> 2262					      &mmsg_compat->msg_hdr, &mmsg->msg_hdr,
  2263					      ctx->uaddr, &ctx->addr, ctx->cmsg_ptr);
  2264		if (err)
  2265			return err;

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

On Fri, 2016-11-25 at 16:39 +0100, Paolo Abeni wrote:
> _skb_try_recv_datagram_batch dequeues multiple skb's from the
> socket's receive queue, and runs the bulk_destructor callback under
> the receive queue lock.

...

> +	last = (struct sk_buff *)queue;
> +	first = (struct sk_buff *)queue->next;
> +	skb_queue_walk(queue, skb) {
> +		last = skb;
> +		totalsize += skb->truesize;
> +		if (++datagrams == batch)
> +			break;
> +	}

This is absolutely not good.

Walking through a list, bringing 2 cache lines per skb, is not the
proper way to deal with bulking.

And I do not see where 'batch' value coming from user space is capped ?

Is it really vlen argument coming from recvmmsg() system call ???

This code runs with BH masked, so you do not want to give user a way to
make you loop there 1000 times 

Bulking is nice, only if you do not compromise with system stability and
latency requirements from other users/applications.

Hi Eric,

On Fri, 2016-11-25 at 14:30 -0800, Eric Dumazet wrote:
> On Fri, 2016-11-25 at 16:39 +0100, Paolo Abeni wrote:
> > _skb_try_recv_datagram_batch dequeues multiple skb's from the
> > socket's receive queue, and runs the bulk_destructor callback under
> > the receive queue lock.
> 
> ...
> 
> > +	last = (struct sk_buff *)queue;
> > +	first = (struct sk_buff *)queue->next;
> > +	skb_queue_walk(queue, skb) {
> > +		last = skb;
> > +		totalsize += skb->truesize;
> > +		if (++datagrams == batch)
> > +			break;
> > +	}
> 
> This is absolutely not good.
> 
> Walking through a list, bringing 2 cache lines per skb, is not the
> proper way to deal with bulking.
> 
> And I do not see where 'batch' value coming from user space is capped ?
> 
> Is it really vlen argument coming from recvmmsg() system call ???
> 
> This code runs with BH masked, so you do not want to give user a way to
> make you loop there 1000 times 
> 
> Bulking is nice, only if you do not compromise with system stability and
> latency requirements from other users/applications.

Thank you for reviewing this.

You are right, the cacheline miss while accessing skb->truesize has
measurable performance impact, and the max burst length comes in from
recvmmsg(). 

We can easily cap the burst to some max value (e.g. 64 or less) and we
can pass to the bulk destructor the skb list and burst length without
accessing skb truesize beforehand. If the burst is short, say 8 skbs or
less, the bulk destructor walk again the list and release the memory,
elsewhere it defers the release after __skb_try_recv_datagram_batch()
completion: we walk the list without the lock held and we acquire it
later again to release all the memory. 

Thank you again,

Paolo