| Message ID | 20230621232129.3776944-1-leitao@debian.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | io_uring: Add io_uring command support for sockets | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Wed, Jun 21, 2023 at 04:21:26PM -0700, Breno Leitao wrote: > Enable io_uring commands on network sockets. Create two new > SOCKET_URING_OP commands that will operate on sockets. Since these > commands are similar to ioctl, uses the _IO{R,W} helpers to embedded the > argument size and operation direction. Also allocates a unused ioctl > chunk for uring command usage. > > In order to call ioctl on sockets, use the file_operations->uring_cmd > callbacks, and map it to a uring socket function, which handles the > SOCKET_URING_OP accordingly, and calls socket ioctls. > > This patches was tested by creating a new test case in liburing. > Link: https://github.com/leitao/liburing/commit/3340908b742c6a26f662a0679c4ddf9df84ef431 > > Signed-off-by: Breno Leitao <leitao@debian.org> > --- Isn't this a new version of an older patch? > .../userspace-api/ioctl/ioctl-number.rst | 1 + > include/linux/io_uring.h | 6 +++++ > include/uapi/linux/io_uring.h | 6 +++++ > io_uring/uring_cmd.c | 27 +++++++++++++++++++ > net/socket.c | 2 ++ > 5 files changed, 42 insertions(+) > > diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst > index 4f7b23faebb9..23348636f2ef 100644 > --- a/Documentation/userspace-api/ioctl/ioctl-number.rst > +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst > @@ -361,6 +361,7 @@ Code Seq# Include File Comments > 0xCB 00-1F CBM serial IEC bus in development: > <mailto:michael.klein@puffin.lb.shuttle.de> > 0xCC 00-0F drivers/misc/ibmvmc.h pseries VMC driver > +0xCC A0-BF uapi/linux/io_uring.h io_uring cmd subsystem This change is nice, but not totally related to this specific one, shouldn't it be separate? > 0xCD 01 linux/reiserfs_fs.h > 0xCE 01-02 uapi/linux/cxl_mem.h Compute Express Link Memory Devices > 0xCF 02 fs/smb/client/cifs_ioctl.h > diff --git a/include/linux/io_uring.h b/include/linux/io_uring.h > index 7fe31b2cd02f..d1b20e2a9fb0 100644 > --- a/include/linux/io_uring.h > +++ b/include/linux/io_uring.h > @@ -71,6 +71,7 @@ static inline void io_uring_free(struct task_struct *tsk) > if (tsk->io_uring) > __io_uring_free(tsk); > } > +int uring_sock_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags); > #else > static inline int io_uring_cmd_import_fixed(u64 ubuf, unsigned long len, int rw, > struct iov_iter *iter, void *ioucmd) > @@ -102,6 +103,11 @@ static inline const char *io_uring_get_opcode(u8 opcode) > { > return ""; > } > +static inline int uring_sock_cmd(struct io_uring_cmd *cmd, > + unsigned int issue_flags) > +{ > + return -EOPNOTSUPP; > +} > #endif > > #endif > diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h > index 0716cb17e436..e20ba410859d 100644 > --- a/include/uapi/linux/io_uring.h > +++ b/include/uapi/linux/io_uring.h > @@ -703,6 +703,12 @@ struct io_uring_recvmsg_out { > __u32 flags; > }; > > +/* > + * Argument for IORING_OP_URING_CMD when file is a socket > + */ > +#define SOCKET_URING_OP_SIOCINQ _IOR(0xcc, 0xa0, int) > +#define SOCKET_URING_OP_SIOCOUTQ _IOR(0xcc, 0xa1, int) > + > #ifdef __cplusplus > } > #endif > diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c > index 5e32db48696d..dcbe6493b03f 100644 > --- a/io_uring/uring_cmd.c > +++ b/io_uring/uring_cmd.c > @@ -7,6 +7,7 @@ > #include <linux/nospec.h> > > #include <uapi/linux/io_uring.h> > +#include <uapi/asm-generic/ioctls.h> > > #include "io_uring.h" > #include "rsrc.h" > @@ -156,3 +157,29 @@ int io_uring_cmd_import_fixed(u64 ubuf, unsigned long len, int rw, > return io_import_fixed(rw, iter, req->imu, ubuf, len); > } > EXPORT_SYMBOL_GPL(io_uring_cmd_import_fixed); > + > +int uring_sock_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags) > +{ > + struct socket *sock = cmd->file->private_data; > + struct sock *sk = sock->sk; > + int ret, arg = 0; > + > + if (!sk->sk_prot || !sk->sk_prot->ioctl) > + return -EOPNOTSUPP; > + > + switch (cmd->sqe->cmd_op) { > + case SOCKET_URING_OP_SIOCINQ: > + ret = sk->sk_prot->ioctl(sk, SIOCINQ, &arg); > + if (ret) > + return ret; > + return arg; > + case SOCKET_URING_OP_SIOCOUTQ: > + ret = sk->sk_prot->ioctl(sk, SIOCOUTQ, &arg); > + if (ret) > + return ret; > + return arg; > + default: > + return -EOPNOTSUPP; > + } > +} > +EXPORT_SYMBOL_GPL(uring_sock_cmd); Did you forget the "io_" prefix? thanks, greg k-h
On Thu, Jun 22, 2023 at 07:20:48AM +0200, Greg Kroah-Hartman wrote: > On Wed, Jun 21, 2023 at 04:21:26PM -0700, Breno Leitao wrote: > > Enable io_uring commands on network sockets. Create two new > > SOCKET_URING_OP commands that will operate on sockets. Since these > > commands are similar to ioctl, uses the _IO{R,W} helpers to embedded the > > argument size and operation direction. Also allocates a unused ioctl > > chunk for uring command usage. > > > > In order to call ioctl on sockets, use the file_operations->uring_cmd > > callbacks, and map it to a uring socket function, which handles the > > SOCKET_URING_OP accordingly, and calls socket ioctls. > > > > This patches was tested by creating a new test case in liburing. > > Link: https://github.com/leitao/liburing/commit/3340908b742c6a26f662a0679c4ddf9df84ef431 > > > > Signed-off-by: Breno Leitao <leitao@debian.org> > > --- > > Isn't this a new version of an older patch? Yes, this should have tagged as V2. [1] https://lore.kernel.org/lkml/20230406144330.1932798-1-leitao@debian.org/#r > > --- a/Documentation/userspace-api/ioctl/ioctl-number.rst > > +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst > > @@ -361,6 +361,7 @@ Code Seq# Include File Comments > > 0xCB 00-1F CBM serial IEC bus in development: > > <mailto:michael.klein@puffin.lb.shuttle.de> > > 0xCC 00-0F drivers/misc/ibmvmc.h pseries VMC driver > > +0xCC A0-BF uapi/linux/io_uring.h io_uring cmd subsystem > > This change is nice, but not totally related to this specific one, > shouldn't it be separate? This is related to this patch, since I am using it below, in the following part: +#define SOCKET_URING_OP_SIOCINQ _IOR(0xcc, 0xa0, int) +#define SOCKET_URING_OP_SIOCOUTQ _IOR(0xcc, 0xa1, int) Should I have a different patch, even if they are related? > > +EXPORT_SYMBOL_GPL(uring_sock_cmd); > > Did you forget the "io_" prefix? Yes, I will rename the function. Thanks for the review.
On Thu, Jun 22, 2023 at 08:02:37AM -0700, Breno Leitao wrote: > On Thu, Jun 22, 2023 at 07:20:48AM +0200, Greg Kroah-Hartman wrote: > > On Wed, Jun 21, 2023 at 04:21:26PM -0700, Breno Leitao wrote: > > > Enable io_uring commands on network sockets. Create two new > > > SOCKET_URING_OP commands that will operate on sockets. Since these > > > commands are similar to ioctl, uses the _IO{R,W} helpers to embedded the > > > argument size and operation direction. Also allocates a unused ioctl > > > chunk for uring command usage. > > > > > > In order to call ioctl on sockets, use the file_operations->uring_cmd > > > callbacks, and map it to a uring socket function, which handles the > > > SOCKET_URING_OP accordingly, and calls socket ioctls. > > > > > > This patches was tested by creating a new test case in liburing. > > > Link: https://github.com/leitao/liburing/commit/3340908b742c6a26f662a0679c4ddf9df84ef431 > > > > > > Signed-off-by: Breno Leitao <leitao@debian.org> > > > --- > > > > Isn't this a new version of an older patch? > > Yes, this should have tagged as V2. > > [1] https://lore.kernel.org/lkml/20230406144330.1932798-1-leitao@debian.org/#r Great, also add what changed below the --- line please. > > > --- a/Documentation/userspace-api/ioctl/ioctl-number.rst > > > +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst > > > @@ -361,6 +361,7 @@ Code Seq# Include File Comments > > > 0xCB 00-1F CBM serial IEC bus in development: > > > <mailto:michael.klein@puffin.lb.shuttle.de> > > > 0xCC 00-0F drivers/misc/ibmvmc.h pseries VMC driver > > > +0xCC A0-BF uapi/linux/io_uring.h io_uring cmd subsystem > > > > This change is nice, but not totally related to this specific one, > > shouldn't it be separate? > > This is related to this patch, since I am using it below, in the > following part: > > +#define SOCKET_URING_OP_SIOCINQ _IOR(0xcc, 0xa0, int) > +#define SOCKET_URING_OP_SIOCOUTQ _IOR(0xcc, 0xa1, int) > > Should I have a different patch, even if they are related? Yes, as you are not using the 0xa2-0xbf range that you just carved out here, right? Where did those numbers come from? thanks, greg k-h
On Thu, Jun 22, 2023 at 06:10:00PM +0200, Greg Kroah-Hartman wrote: > On Thu, Jun 22, 2023 at 08:02:37AM -0700, Breno Leitao wrote: > > On Thu, Jun 22, 2023 at 07:20:48AM +0200, Greg Kroah-Hartman wrote: > > > On Wed, Jun 21, 2023 at 04:21:26PM -0700, Breno Leitao wrote: > > > > --- a/Documentation/userspace-api/ioctl/ioctl-number.rst > > > > +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst > > > > @@ -361,6 +361,7 @@ Code Seq# Include File Comments > > > > 0xCB 00-1F CBM serial IEC bus in development: > > > > <mailto:michael.klein@puffin.lb.shuttle.de> > > > > 0xCC 00-0F drivers/misc/ibmvmc.h pseries VMC driver > > > > +0xCC A0-BF uapi/linux/io_uring.h io_uring cmd subsystem > > > > > > This change is nice, but not totally related to this specific one, > > > shouldn't it be separate? > > > > This is related to this patch, since I am using it below, in the > > following part: > > > > +#define SOCKET_URING_OP_SIOCINQ _IOR(0xcc, 0xa0, int) > > +#define SOCKET_URING_OP_SIOCOUTQ _IOR(0xcc, 0xa1, int) > > > > Should I have a different patch, even if they are related? > > Yes, as you are not using the 0xa2-0xbf range that you just carved out > here, right? Where did those numbers come from? Correct. For now we are just using 0xa0 and 0xa1, and eventually we might need more ioctls numbers. I got these numbers finding a unused block and having some room for expansion, as suggested by Documentation/userspace-api/ioctl/ioctl-number.rst, that says: If you are writing a driver for a new device and need a letter, pick an unused block with enough room for expansion: 32 to 256 ioctl commands.
On Thu, Jun 22, 2023 at 09:38:15AM -0700, Breno Leitao wrote: > On Thu, Jun 22, 2023 at 06:10:00PM +0200, Greg Kroah-Hartman wrote: > > On Thu, Jun 22, 2023 at 08:02:37AM -0700, Breno Leitao wrote: > > > On Thu, Jun 22, 2023 at 07:20:48AM +0200, Greg Kroah-Hartman wrote: > > > > On Wed, Jun 21, 2023 at 04:21:26PM -0700, Breno Leitao wrote: > > > > > --- a/Documentation/userspace-api/ioctl/ioctl-number.rst > > > > > +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst > > > > > @@ -361,6 +361,7 @@ Code Seq# Include File Comments > > > > > 0xCB 00-1F CBM serial IEC bus in development: > > > > > <mailto:michael.klein@puffin.lb.shuttle.de> > > > > > 0xCC 00-0F drivers/misc/ibmvmc.h pseries VMC driver > > > > > +0xCC A0-BF uapi/linux/io_uring.h io_uring cmd subsystem > > > > > > > > This change is nice, but not totally related to this specific one, > > > > shouldn't it be separate? > > > > > > This is related to this patch, since I am using it below, in the > > > following part: > > > > > > +#define SOCKET_URING_OP_SIOCINQ _IOR(0xcc, 0xa0, int) > > > +#define SOCKET_URING_OP_SIOCOUTQ _IOR(0xcc, 0xa1, int) > > > > > > Should I have a different patch, even if they are related? > > > > Yes, as you are not using the 0xa2-0xbf range that you just carved out > > here, right? Where did those numbers come from? > > Correct. For now we are just using 0xa0 and 0xa1, and eventually we > might need more ioctls numbers. > > I got these numbers finding a unused block and having some room for > expansion, as suggested by Documentation/userspace-api/ioctl/ioctl-number.rst, > that says: > > If you are writing a driver for a new device and need a letter, pick an > unused block with enough room for expansion: 32 to 256 ioctl commands. So is this the first io_uring ioctl? If so, why is this an ioctl and not just a "normal" io_uring call? thanks, greg k-h
On Thu, 22 Jun 2023 19:03:04 +0200 Greg Kroah-Hartman wrote: > > Correct. For now we are just using 0xa0 and 0xa1, and eventually we > > might need more ioctls numbers. > > > > I got these numbers finding a unused block and having some room for > > expansion, as suggested by Documentation/userspace-api/ioctl/ioctl-number.rst, > > that says: > > > > If you are writing a driver for a new device and need a letter, pick an > > unused block with enough room for expansion: 32 to 256 ioctl commands. > > So is this the first io_uring ioctl? If so, why is this an ioctl and > not just a "normal" io_uring call? +1, the mixing with classic ioctl seems confusing and I'm not sure if it buys us anything.
On Thu, Jun 22, 2023 at 07:03:04PM +0200, Greg Kroah-Hartman wrote: > On Thu, Jun 22, 2023 at 09:38:15AM -0700, Breno Leitao wrote: > > On Thu, Jun 22, 2023 at 06:10:00PM +0200, Greg Kroah-Hartman wrote: > > > On Thu, Jun 22, 2023 at 08:02:37AM -0700, Breno Leitao wrote: > > > > On Thu, Jun 22, 2023 at 07:20:48AM +0200, Greg Kroah-Hartman wrote: > > > > > On Wed, Jun 21, 2023 at 04:21:26PM -0700, Breno Leitao wrote: > > > > > > --- a/Documentation/userspace-api/ioctl/ioctl-number.rst > > > > > > +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst > > > > > > @@ -361,6 +361,7 @@ Code Seq# Include File Comments > > > > > > 0xCB 00-1F CBM serial IEC bus in development: > > > > > > <mailto:michael.klein@puffin.lb.shuttle.de> > > > > > > 0xCC 00-0F drivers/misc/ibmvmc.h pseries VMC driver > > > > > > +0xCC A0-BF uapi/linux/io_uring.h io_uring cmd subsystem > > > > > > > > > > This change is nice, but not totally related to this specific one, > > > > > shouldn't it be separate? > > > > > > > > This is related to this patch, since I am using it below, in the > > > > following part: > > > > > > > > +#define SOCKET_URING_OP_SIOCINQ _IOR(0xcc, 0xa0, int) > > > > +#define SOCKET_URING_OP_SIOCOUTQ _IOR(0xcc, 0xa1, int) > > > > > > > > Should I have a different patch, even if they are related? > > > > > > Yes, as you are not using the 0xa2-0xbf range that you just carved out > > > here, right? Where did those numbers come from? > > > > Correct. For now we are just using 0xa0 and 0xa1, and eventually we > > might need more ioctls numbers. > > > > I got these numbers finding a unused block and having some room for > > expansion, as suggested by Documentation/userspace-api/ioctl/ioctl-number.rst, > > that says: > > > > If you are writing a driver for a new device and need a letter, pick an > > unused block with enough room for expansion: 32 to 256 ioctl commands. > > So is this the first io_uring ioctl? If so, why is this an ioctl and > not just a "normal" io_uring call? This is a way to pass a generic command to file. This is not a ioctl per se (not called through ioctl). I am leveraging the ioctl to embedded the "direction" and "size" information in the command itself. I can defintely do something as the following, if it is a better implementation: #define SOCKET_URING_OP_SIOCINQ 0 #define SOCKET_URING_OP_SIOCOUTQ 1
On Thu, Jun 22, 2023 at 10:39:48AM -0700, Jakub Kicinski wrote: > On Thu, 22 Jun 2023 19:03:04 +0200 Greg Kroah-Hartman wrote: > > > Correct. For now we are just using 0xa0 and 0xa1, and eventually we > > > might need more ioctls numbers. > > > > > > I got these numbers finding a unused block and having some room for > > > expansion, as suggested by Documentation/userspace-api/ioctl/ioctl-number.rst, > > > that says: > > > > > > If you are writing a driver for a new device and need a letter, pick an > > > unused block with enough room for expansion: 32 to 256 ioctl commands. > > > > So is this the first io_uring ioctl? If so, why is this an ioctl and > > not just a "normal" io_uring call? > > +1, the mixing with classic ioctl seems confusing and I'm not sure > if it buys us anything. Sure, let me remove the dependency on ioctl()s then.
diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst index 4f7b23faebb9..23348636f2ef 100644 --- a/Documentation/userspace-api/ioctl/ioctl-number.rst +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst @@ -361,6 +361,7 @@ Code Seq# Include File Comments 0xCB 00-1F CBM serial IEC bus in development: <mailto:michael.klein@puffin.lb.shuttle.de> 0xCC 00-0F drivers/misc/ibmvmc.h pseries VMC driver +0xCC A0-BF uapi/linux/io_uring.h io_uring cmd subsystem 0xCD 01 linux/reiserfs_fs.h 0xCE 01-02 uapi/linux/cxl_mem.h Compute Express Link Memory Devices 0xCF 02 fs/smb/client/cifs_ioctl.h diff --git a/include/linux/io_uring.h b/include/linux/io_uring.h index 7fe31b2cd02f..d1b20e2a9fb0 100644 --- a/include/linux/io_uring.h +++ b/include/linux/io_uring.h @@ -71,6 +71,7 @@ static inline void io_uring_free(struct task_struct *tsk) if (tsk->io_uring) __io_uring_free(tsk); } +int uring_sock_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags); #else static inline int io_uring_cmd_import_fixed(u64 ubuf, unsigned long len, int rw, struct iov_iter *iter, void *ioucmd) @@ -102,6 +103,11 @@ static inline const char *io_uring_get_opcode(u8 opcode) { return ""; } +static inline int uring_sock_cmd(struct io_uring_cmd *cmd, + unsigned int issue_flags) +{ + return -EOPNOTSUPP; +} #endif #endif diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h index 0716cb17e436..e20ba410859d 100644 --- a/include/uapi/linux/io_uring.h +++ b/include/uapi/linux/io_uring.h @@ -703,6 +703,12 @@ struct io_uring_recvmsg_out { __u32 flags; }; +/* + * Argument for IORING_OP_URING_CMD when file is a socket + */ +#define SOCKET_URING_OP_SIOCINQ _IOR(0xcc, 0xa0, int) +#define SOCKET_URING_OP_SIOCOUTQ _IOR(0xcc, 0xa1, int) + #ifdef __cplusplus } #endif diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c index 5e32db48696d..dcbe6493b03f 100644 --- a/io_uring/uring_cmd.c +++ b/io_uring/uring_cmd.c @@ -7,6 +7,7 @@ #include <linux/nospec.h> #include <uapi/linux/io_uring.h> +#include <uapi/asm-generic/ioctls.h> #include "io_uring.h" #include "rsrc.h" @@ -156,3 +157,29 @@ int io_uring_cmd_import_fixed(u64 ubuf, unsigned long len, int rw, return io_import_fixed(rw, iter, req->imu, ubuf, len); } EXPORT_SYMBOL_GPL(io_uring_cmd_import_fixed); + +int uring_sock_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags) +{ + struct socket *sock = cmd->file->private_data; + struct sock *sk = sock->sk; + int ret, arg = 0; + + if (!sk->sk_prot || !sk->sk_prot->ioctl) + return -EOPNOTSUPP; + + switch (cmd->sqe->cmd_op) { + case SOCKET_URING_OP_SIOCINQ: + ret = sk->sk_prot->ioctl(sk, SIOCINQ, &arg); + if (ret) + return ret; + return arg; + case SOCKET_URING_OP_SIOCOUTQ: + ret = sk->sk_prot->ioctl(sk, SIOCOUTQ, &arg); + if (ret) + return ret; + return arg; + default: + return -EOPNOTSUPP; + } +} +EXPORT_SYMBOL_GPL(uring_sock_cmd); diff --git a/net/socket.c b/net/socket.c index b778fc03c6e0..db11e94d2259 100644 --- a/net/socket.c +++ b/net/socket.c @@ -88,6 +88,7 @@ #include <linux/xattr.h> #include <linux/nospec.h> #include <linux/indirect_call_wrapper.h> +#include <linux/io_uring.h> #include <linux/uaccess.h> #include <asm/unistd.h> @@ -159,6 +160,7 @@ static const struct file_operations socket_file_ops = { #ifdef CONFIG_COMPAT .compat_ioctl = compat_sock_ioctl, #endif + .uring_cmd = uring_sock_cmd, .mmap = sock_mmap, .release = sock_close, .fasync = sock_fasync,
Enable io_uring commands on network sockets. Create two new SOCKET_URING_OP commands that will operate on sockets. Since these commands are similar to ioctl, uses the _IO{R,W} helpers to embedded the argument size and operation direction. Also allocates a unused ioctl chunk for uring command usage. In order to call ioctl on sockets, use the file_operations->uring_cmd callbacks, and map it to a uring socket function, which handles the SOCKET_URING_OP accordingly, and calls socket ioctls. This patches was tested by creating a new test case in liburing. Link: https://github.com/leitao/liburing/commit/3340908b742c6a26f662a0679c4ddf9df84ef431 Signed-off-by: Breno Leitao <leitao@debian.org> --- .../userspace-api/ioctl/ioctl-number.rst | 1 + include/linux/io_uring.h | 6 +++++ include/uapi/linux/io_uring.h | 6 +++++ io_uring/uring_cmd.c | 27 +++++++++++++++++++ net/socket.c | 2 ++ 5 files changed, 42 insertions(+)