Re: [PATCH net-next 4/5] tcp: implement mmap() for zero copy receive

[Eric Dumazet <eric.dumazet@gmail.com>]

On 04/19/2018 04:15 PM, Eric Dumazet wrote:

> I am not sure we can keep mmap() API, since we probably need to first lock the socket,
> then grab vm semaphore.
> 

We can keep mmap() nice interface, granted we can add one hook like in following patch.

David, do you think such patch would be acceptable by lkml and mm/fs maintainers ?

Alternative would be implementing an ioctl() or getsockopt() operation,
but it seems less natural...

Thanks !

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 92efaf1f89775f7b017477617dd983c10e0dc4d2..016c711ac33e226b4285ee5bd688e14661dc0879 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1714,6 +1714,7 @@ struct file_operations {
        long (*unlocked_ioctl) (struct file *, unsigned int, unsigned long);
        long (*compat_ioctl) (struct file *, unsigned int, unsigned long);
        int (*mmap) (struct file *, struct vm_area_struct *);
+       void (*mmap_hook) (struct file *, bool);
        unsigned long mmap_supported_flags;
        int (*open) (struct inode *, struct file *);
        int (*flush) (struct file *, fl_owner_t id);
diff --git a/mm/util.c b/mm/util.c
index 1fc4fa7576f762bbbf341f056ca6d0be803a423f..b546c59a6169c4dfa9011c61e86da4d03496aa4d 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -350,11 +350,20 @@ unsigned long vm_mmap_pgoff(struct file *file, unsigned long addr,
 
        ret = security_mmap_file(file, prot, flag);
        if (!ret) {
-               if (down_write_killable(&mm->mmap_sem))
+               void (*mmap_hook)(struct file *, bool) = file ? file->f_op->mmap_hook : NULL;
+
+               if (mmap_hook)
+                       mmap_hook(file, true);
+               if (down_write_killable(&mm->mmap_sem)) {
+                       if (mmap_hook)
+                               mmap_hook(file, false);
                        return -EINTR;
+               }
                ret = do_mmap_pgoff(file, addr, len, prot, flag, pgoff,
                                    &populate, &uf);
                up_write(&mm->mmap_sem);
+               if (mmap_hook)
+                       mmap_hook(file, false);
                userfaultfd_unmap_complete(mm, &uf);
                if (populate)
                        mm_populate(ret, populate);
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 4022073b0aeea9d07af0fa825b640a00512908a3..79b05d6d41643e8c309dfb8bd9597dc8b00fb0e1 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -1756,8 +1756,6 @@ int tcp_mmap(struct file *file, struct socket *sock,
        /* TODO: Maybe the following is not needed if pages are COW */
        vma->vm_flags &= ~VM_MAYWRITE;
 
-       lock_sock(sk);
-
        ret = -ENOTCONN;
        if (sk->sk_state == TCP_LISTEN)
                goto out;
@@ -1833,7 +1831,6 @@ int tcp_mmap(struct file *file, struct socket *sock,
 
        ret = 0;
 out:
-       release_sock(sk);
        kvfree(pages_array);
        return ret;
 }
diff --git a/net/socket.c b/net/socket.c
index f10f1d947c78c193b49379b0ec641d81367fb4cf..bcabae3c37d765e5c0548a14fc93c19258972b48 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -131,6 +131,16 @@ static ssize_t sock_splice_read(struct file *file, loff_t *ppos,
                                struct pipe_inode_info *pipe, size_t len,
                                unsigned int flags);
 
+static void sock_mmap_hook(struct file *file, bool enter)
+{
+       struct socket *sock = file->private_data;
+       struct sock *sk = sock->sk;
+
+       if (enter)
+               lock_sock(sk);
+       else
+               release_sock(sk);
+}
 /*
  *     Socket files have a set of 'special' operations as well as the generic file ones. These don't appear
  *     in the operation structures but are done directly via the socketcall() multiplexor.
@@ -147,6 +157,7 @@ static const struct file_operations socket_file_ops = {
        .compat_ioctl = compat_sock_ioctl,
 #endif
        .mmap =         sock_mmap,
+       .mmap_hook =    sock_mmap_hook,
        .release =      sock_close,
        .fasync =       sock_fasync,
        .sendpage =     sock_sendpage,