| Message ID | 20210916122943.19849-1-yajun.deng@linux.dev |
|---|---|
| State | New |
| Headers | show |
| Series | [net-next] net: socket: add the case sock_no_xxx support | expand |
On Thu, 16 Sep 2021 20:29:43 +0800 Yajun Deng wrote: > Those sock_no_{mmap, socketpair, listen, accept, connect, shutdown, > sendpage} functions are used many times in struct proto_ops, but they are > meaningless. So we can add them support in socket and delete them in struct > proto_ops. So the reason to do this is.. what exactly? Removing a couple empty helpers (which is not even part of this patch)? I'm not sold, sorry.
September 18, 2021 9:33 AM, "Jakub Kicinski" <kuba@kernel.org> wrote: > On Thu, 16 Sep 2021 20:29:43 +0800 Yajun Deng wrote: > >> Those sock_no_{mmap, socketpair, listen, accept, connect, shutdown, >> sendpage} functions are used many times in struct proto_ops, but they are >> meaningless. So we can add them support in socket and delete them in struct >> proto_ops. > > So the reason to do this is.. what exactly? > > Removing a couple empty helpers (which is not even part of this patch)? > > I'm not sold, sorry. When we define a struct proto_ops xxx, we only need to assign meaningful member variables that we need. Those {mmap, socketpair, listen, accept, connect, shutdown, sendpage} members we don't need assign it if we don't need. We just need do once in socket, not in every struct proto_ops. These members are assigned meaningless values far more often than meaningful ones, so this patch I used likely(!!sock->ops->xxx) for this case. This is the reason why I send this patch. I would send a set of patchs remove most of the meaningless members assigned rather than remove sock_no_{mmap, socketpair, listen, accept, connect, shutdown, sendpage} functions if this patch was accepted. e.g. diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c index 1d816a5fd3eb..86422eb440cb 100644 --- a/net/ipv4/af_inet.c +++ b/net/ipv4/af_inet.c @@ -1059,20 +1059,16 @@ const struct proto_ops inet_dgram_ops = { .release = inet_release, .bind = inet_bind, .connect = inet_dgram_connect, - .socketpair = sock_no_socketpair, - .accept = sock_no_accept, --- a/net/ipv4/af_inet.c +++ b/net/ipv4/af_inet.c @@ -1059,20 +1059,16 @@ const struct proto_ops inet_dgram_ops = { .release = inet_release, .bind = inet_bind, .connect = inet_dgram_connect, - .socketpair = sock_no_socketpair, - .accept = sock_no_accept, .getname = inet_getname, .gettstamp = sock_gettstamp, - .listen = sock_no_listen, .shutdown = inet_shutdown, .setsockopt = sock_common_setsockopt, .getsockopt = sock_common_getsockopt, .sendmsg = inet_sendmsg, .read_sock = udp_read_sock, .recvmsg = inet_recvmsg, - .mmap = sock_no_mmap, .sendpage = inet_sendpage, .set_peek_off = sk_set_peek_off, #ifdef CONFIG_COMPAT @@ -1091,19 +1087,15 @@ static const struct proto_ops inet_sockraw_ops = { .release = inet_release, .bind = inet_bind, .connect = inet_dgram_connect, - .socketpair = sock_no_socketpair, - .accept = sock_no_accept, .getname = inet_getname, .poll = datagram_poll, .ioctl = inet_ioctl, .gettstamp = sock_gettstamp, - .listen = sock_no_listen, .shutdown = inet_shutdown, .setsockopt = sock_common_setsockopt, .getsockopt = sock_common_getsockopt, .sendmsg = inet_sendmsg, .recvmsg = inet_recvmsg, - .mmap = sock_no_mmap, .sendpage = inet_sendpage, #ifdef CONFIG_COMPAT .compat_ioctl = inet_compat_ioctl,
On Sat, Sep 18, 2021 at 5:11 AM <yajun.deng@linux.dev> wrote: > > September 18, 2021 9:33 AM, "Jakub Kicinski" <kuba@kernel.org> wrote: > > > On Thu, 16 Sep 2021 20:29:43 +0800 Yajun Deng wrote: > > > >> Those sock_no_{mmap, socketpair, listen, accept, connect, shutdown, > >> sendpage} functions are used many times in struct proto_ops, but they are > >> meaningless. So we can add them support in socket and delete them in struct > >> proto_ops. > > > > So the reason to do this is.. what exactly? > > > > Removing a couple empty helpers (which is not even part of this patch)? > > > > I'm not sold, sorry. > > When we define a struct proto_ops xxx, we only need to assign meaningful member variables that we need. > Those {mmap, socketpair, listen, accept, connect, shutdown, sendpage} members we don't need assign > it if we don't need. We just need do once in socket, not in every struct proto_ops. > > These members are assigned meaningless values far more often than meaningful ones, so this patch I used likely(!!sock->ops->xxx) for this case. This is the reason why I send this patch. But you end up adding more code: 1 file changed, 58 insertions(+), 13 deletions(-) I don't see this as a gain from any perspective. Thanks.
From: Cong Wang Date: 2021-09-20 07:52 To: Yajun Deng CC: Jakub Kicinski; David Miller; Linux Kernel Network Developers; LKML Subject: Re: [PATCH net-next] net: socket: add the case sock_no_xxx support On Sat, Sep 18, 2021 at 5:11 AM <yajun.deng@linux.dev> wrote: > > September 18, 2021 9:33 AM, "Jakub Kicinski" <kuba@kernel.org> wrote: > > > On Thu, 16 Sep 2021 20:29:43 +0800 Yajun Deng wrote: > > > >> Those sock_no_{mmap, socketpair, listen, accept, connect, shutdown, > >> sendpage} functions are used many times in struct proto_ops, but they are > >> meaningless. So we can add them support in socket and delete them in struct > >> proto_ops. > > > > So the reason to do this is.. what exactly? > > > > Removing a couple empty helpers (which is not even part of this patch)? > > > > I'm not sold, sorry. > > When we define a struct proto_ops xxx, we only need to assign meaningful member variables that we need. > Those {mmap, socketpair, listen, accept, connect, shutdown, sendpage} members we don't need assign > it if we don't need. We just need do once in socket, not in every struct proto_ops. > > These members are assigned meaningless values far more often than meaningful ones, so this patch I used likely(!!sock->ops->xxx) for this case. This is the reason why I send this patch. But you end up adding more code: 1 file changed, 58 insertions(+), 13 deletions(-) Yes，This would add more code, but this is at the cost of reducing other codes. At the same time, the code will only run likely(!sock->ops->xxx) in most cases. Don’t you think that this kind of meaningless thing shouldn’t be done by socket? I don't see this as a gain from any perspective. Thanks.
On Mon, Sep 20, 2021 at 5:28 AM yajun.deng@linux.dev <yajun.deng@linux.dev> wrote: > > From: Cong Wang > Date: 2021-09-20 07:52 > To: Yajun Deng > CC: Jakub Kicinski; David Miller; Linux Kernel Network Developers; LKML > Subject: Re: [PATCH net-next] net: socket: add the case sock_no_xxx support > On Sat, Sep 18, 2021 at 5:11 AM <yajun.deng@linux.dev> wrote: > > > > September 18, 2021 9:33 AM, "Jakub Kicinski" <kuba@kernel.org> wrote: > > > > > On Thu, 16 Sep 2021 20:29:43 +0800 Yajun Deng wrote: > > > > > >> Those sock_no_{mmap, socketpair, listen, accept, connect, shutdown, > > >> sendpage} functions are used many times in struct proto_ops, but they are > > >> meaningless. So we can add them support in socket and delete them in struct > > >> proto_ops. > > > > > > So the reason to do this is.. what exactly? > > > > > > Removing a couple empty helpers (which is not even part of this patch)? > > > > > > I'm not sold, sorry. > > > > When we define a struct proto_ops xxx, we only need to assign meaningful member variables that we need. > > Those {mmap, socketpair, listen, accept, connect, shutdown, sendpage} members we don't need assign > > it if we don't need. We just need do once in socket, not in every struct proto_ops. > > > > These members are assigned meaningless values far more often than meaningful ones, so this patch I used likely(!!sock->ops->xxx) for this case. This is the reason why I send this patch. > > But you end up adding more code: > > 1 file changed, 58 insertions(+), 13 deletions(-) > > Yes，This would add more code, but this is at the cost of reducing other codes. At the same time, the code will only run likely(!sock->ops->xxx) in most cases. Don’t you think that this kind of meaningless thing shouldn’t be done by socket? I have no idea why you call it reducing code while adding 45 lines of code. So this does not make sense to me. Thanks.
diff --git a/net/socket.c b/net/socket.c index 7f64a6eccf63..4d0e1a2970fb 100644 --- a/net/socket.c +++ b/net/socket.c @@ -1306,6 +1306,9 @@ static int sock_mmap(struct file *file, struct vm_area_struct *vma) { struct socket *sock = file->private_data; + if (likely(!sock->ops->mmap)) + return -ENODEV; + return sock->ops->mmap(file, sock, vma); } @@ -1629,11 +1632,19 @@ int __sys_socketpair(int family, int type, int protocol, int __user *usockvec) goto out; } - err = sock1->ops->socketpair(sock1, sock2); - if (unlikely(err < 0)) { + if (likely(!sock1->ops->socketpair)) { + err = -EOPNOTSUPP; sock_release(sock2); sock_release(sock1); goto out; + + } else { + err = sock1->ops->socketpair(sock1, sock2); + if (unlikely(err < 0)) { + sock_release(sock2); + sock_release(sock1); + goto out; + } } newfile1 = sock_alloc_file(sock1, flags, NULL); @@ -1704,6 +1715,14 @@ SYSCALL_DEFINE3(bind, int, fd, struct sockaddr __user *, umyaddr, int, addrlen) return __sys_bind(fd, umyaddr, addrlen); } +static int __sock_listen(struct socket *sock, int backlog) +{ + if (likely(!sock->ops->listen)) + return -EOPNOTSUPP; + + return sock->ops->listen(sock, backlog); +} + /* * Perform a listen. Basically, we allow the protocol to do anything * necessary for a listen, and if that works, we mark the socket as @@ -1724,7 +1743,7 @@ int __sys_listen(int fd, int backlog) err = security_socket_listen(sock, backlog); if (!err) - err = sock->ops->listen(sock, backlog); + err = __sock_listen(sock, backlog); fput_light(sock->file, fput_needed); } @@ -1736,6 +1755,15 @@ SYSCALL_DEFINE2(listen, int, fd, int, backlog) return __sys_listen(fd, backlog); } +static int __sock_accept(struct socket *sock, struct socket *newsock, + int flags, bool kern) +{ + if (likely(!sock->ops->accept)) + return -EOPNOTSUPP; + + return sock->ops->accept(sock, newsock, flags, kern); +} + struct file *do_accept(struct file *file, unsigned file_flags, struct sockaddr __user *upeer_sockaddr, int __user *upeer_addrlen, int flags) @@ -1770,8 +1798,8 @@ struct file *do_accept(struct file *file, unsigned file_flags, if (err) goto out_fd; - err = sock->ops->accept(sock, newsock, sock->file->f_flags | file_flags, - false); + err = __sock_accept(sock, newsock, sock->file->f_flags | file_flags, + false); if (err < 0) goto out_fd; @@ -1864,6 +1892,15 @@ SYSCALL_DEFINE3(accept, int, fd, struct sockaddr __user *, upeer_sockaddr, return __sys_accept4(fd, upeer_sockaddr, upeer_addrlen, 0); } +static int __sock_connect(struct socket *sock, struct sockaddr *saddr, + int len, int flags) +{ + if (likely(!sock->ops->connect)) + return -EOPNOTSUPP; + + return sock->ops->connect(sock, saddr, len, flags); +} + /* * Attempt to connect to a socket with the server address. The address * is in user space so we verify it is OK and move it to kernel space. @@ -1893,8 +1930,8 @@ int __sys_connect_file(struct file *file, struct sockaddr_storage *address, if (err) goto out; - err = sock->ops->connect(sock, (struct sockaddr *)address, addrlen, - sock->file->f_flags | file_flags); + err = __sock_connect(sock, (struct sockaddr *)address, addrlen, + sock->file->f_flags | file_flags); out: return err; } @@ -2235,6 +2272,14 @@ SYSCALL_DEFINE5(getsockopt, int, fd, int, level, int, optname, return __sys_getsockopt(fd, level, optname, optval, optlen); } +static int __sock_shutdown(struct socket *sock, int how) +{ + if (likely(!sock->ops->shutdown)) + return -EOPNOTSUPP; + + return sock->ops->shutdown(sock, how); +} + /* * Shutdown a socket. */ @@ -2245,7 +2290,7 @@ int __sys_shutdown_sock(struct socket *sock, int how) err = security_socket_shutdown(sock, how); if (!err) - err = sock->ops->shutdown(sock, how); + err = __sock_shutdown(sock, how); return err; } @@ -3394,7 +3439,7 @@ EXPORT_SYMBOL(kernel_bind); int kernel_listen(struct socket *sock, int backlog) { - return sock->ops->listen(sock, backlog); + return __sock_listen(sock, backlog); } EXPORT_SYMBOL(kernel_listen); @@ -3419,7 +3464,7 @@ int kernel_accept(struct socket *sock, struct socket **newsock, int flags) if (err < 0) goto done; - err = sock->ops->accept(sock, *newsock, flags, true); + err = __sock_accept(sock, *newsock, flags, true); if (err < 0) { sock_release(*newsock); *newsock = NULL; @@ -3450,7 +3495,7 @@ EXPORT_SYMBOL(kernel_accept); int kernel_connect(struct socket *sock, struct sockaddr *addr, int addrlen, int flags) { - return sock->ops->connect(sock, addr, addrlen, flags); + return __sock_connect(sock, addr, addrlen, flags); } EXPORT_SYMBOL(kernel_connect); @@ -3498,7 +3543,7 @@ EXPORT_SYMBOL(kernel_getpeername); int kernel_sendpage(struct socket *sock, struct page *page, int offset, size_t size, int flags) { - if (sock->ops->sendpage) { + if (unlikely(sock->ops->sendpage)) { /* Warn in case the improper page to zero-copy send */ WARN_ONCE(!sendpage_ok(page), "improper page for zero-copy send"); return sock->ops->sendpage(sock, page, offset, size, flags); @@ -3542,7 +3587,7 @@ EXPORT_SYMBOL(kernel_sendpage_locked); int kernel_sock_shutdown(struct socket *sock, enum sock_shutdown_cmd how) { - return sock->ops->shutdown(sock, how); + return __sock_shutdown(sock, how); } EXPORT_SYMBOL(kernel_sock_shutdown);
Those sock_no_{mmap, socketpair, listen, accept, connect, shutdown, sendpage} functions are used many times in struct proto_ops, but they are meaningless. So we can add them support in socket and delete them in struct proto_ops. Signed-off-by: Yajun Deng <yajun.deng@linux.dev> --- net/socket.c | 71 ++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 58 insertions(+), 13 deletions(-)