| Message ID | 20250116-tun-v3-9-c6b2871e97f7@daynix.com |
|---|---|
| State | New |
| Headers | show |
| Series | [net,v3,1/9] tun: Refactor CONFIG_TUN_VNET_CROSS_LE | expand |
On Mon, Jan 20, 2025 at 1:37 AM Jason Wang <jasowang@redhat.com> wrote: > > On Fri, Jan 17, 2025 at 6:35 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > > > > On 2025/01/17 18:23, Willem de Bruijn wrote: > > > Akihiko Odaki wrote: > > >> tun and tap implements the same vnet-related features so reuse the code. > > >> > > >> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > > >> --- > > >> drivers/net/Kconfig | 1 + > > >> drivers/net/Makefile | 6 +- > > >> drivers/net/tap.c | 152 +++++-------------------------------------------- > > >> drivers/net/tun_vnet.c | 5 ++ > > >> 4 files changed, 24 insertions(+), 140 deletions(-) > > >> > > >> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig > > >> index 1fd5acdc73c6..c420418473fc 100644 > > >> --- a/drivers/net/Kconfig > > >> +++ b/drivers/net/Kconfig > > >> @@ -395,6 +395,7 @@ config TUN > > >> tristate "Universal TUN/TAP device driver support" > > >> depends on INET > > >> select CRC32 > > >> + select TAP > > >> help > > >> TUN/TAP provides packet reception and transmission for user space > > >> programs. It can be viewed as a simple Point-to-Point or Ethernet > > >> diff --git a/drivers/net/Makefile b/drivers/net/Makefile > > >> index bb8eb3053772..2275309a97ee 100644 > > >> --- a/drivers/net/Makefile > > >> +++ b/drivers/net/Makefile > > >> @@ -29,9 +29,9 @@ obj-y += mdio/ > > >> obj-y += pcs/ > > >> obj-$(CONFIG_RIONET) += rionet.o > > >> obj-$(CONFIG_NET_TEAM) += team/ > > >> -obj-$(CONFIG_TUN) += tun-drv.o > > >> -tun-drv-y := tun.o tun_vnet.o > > >> -obj-$(CONFIG_TAP) += tap.o > > >> +obj-$(CONFIG_TUN) += tun.o > > > > > > Is reversing the previous changes to tun.ko intentional? > > > > > > Perhaps the previous approach with a new CONFIG_TUN_VNET is preferable > > > over this. In particular over making TUN select TAP, a new dependency. > > > > Jason, you also commented about CONFIG_TUN_VNET for the previous > > version. Do you prefer the old approach, or the new one? (Or if you have > > another idea, please tell me.) > > Ideally, if we can make TUN select TAP that would be better. But there > are some subtle differences in the multi queue implementation. We will > end up with some useless code for TUN unless we can unify the multi > queue logic. It might not be worth it to change the TUN's multi queue > logic so having a new file seems to be better. +1 on deduplicating further. But this series is complex enough. Let's not expand that. The latest approach with a separate .o file may have some performance cost by converting likely inlined code into real function calls. Another option is to move it all into tun_vnet.h. That also resolves the Makefile issues.
Akihiko Odaki wrote: > On 2025/01/20 20:19, Willem de Bruijn wrote: > > On Mon, Jan 20, 2025 at 1:37 AM Jason Wang <jasowang@redhat.com> wrote: > >> > >> On Fri, Jan 17, 2025 at 6:35 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > >>> > >>> On 2025/01/17 18:23, Willem de Bruijn wrote: > >>>> Akihiko Odaki wrote: > >>>>> tun and tap implements the same vnet-related features so reuse the code. > >>>>> > >>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > >>>>> --- > >>>>> drivers/net/Kconfig | 1 + > >>>>> drivers/net/Makefile | 6 +- > >>>>> drivers/net/tap.c | 152 +++++-------------------------------------------- > >>>>> drivers/net/tun_vnet.c | 5 ++ > >>>>> 4 files changed, 24 insertions(+), 140 deletions(-) > >>>>> > >>>>> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig > >>>>> index 1fd5acdc73c6..c420418473fc 100644 > >>>>> --- a/drivers/net/Kconfig > >>>>> +++ b/drivers/net/Kconfig > >>>>> @@ -395,6 +395,7 @@ config TUN > >>>>> tristate "Universal TUN/TAP device driver support" > >>>>> depends on INET > >>>>> select CRC32 > >>>>> + select TAP > >>>>> help > >>>>> TUN/TAP provides packet reception and transmission for user space > >>>>> programs. It can be viewed as a simple Point-to-Point or Ethernet > >>>>> diff --git a/drivers/net/Makefile b/drivers/net/Makefile > >>>>> index bb8eb3053772..2275309a97ee 100644 > >>>>> --- a/drivers/net/Makefile > >>>>> +++ b/drivers/net/Makefile > >>>>> @@ -29,9 +29,9 @@ obj-y += mdio/ > >>>>> obj-y += pcs/ > >>>>> obj-$(CONFIG_RIONET) += rionet.o > >>>>> obj-$(CONFIG_NET_TEAM) += team/ > >>>>> -obj-$(CONFIG_TUN) += tun-drv.o > >>>>> -tun-drv-y := tun.o tun_vnet.o > >>>>> -obj-$(CONFIG_TAP) += tap.o > >>>>> +obj-$(CONFIG_TUN) += tun.o > >>>> > >>>> Is reversing the previous changes to tun.ko intentional? > >>>> > >>>> Perhaps the previous approach with a new CONFIG_TUN_VNET is preferable > >>>> over this. In particular over making TUN select TAP, a new dependency. > >>> > >>> Jason, you also commented about CONFIG_TUN_VNET for the previous > >>> version. Do you prefer the old approach, or the new one? (Or if you have > >>> another idea, please tell me.) > >> > >> Ideally, if we can make TUN select TAP that would be better. But there > >> are some subtle differences in the multi queue implementation. We will > >> end up with some useless code for TUN unless we can unify the multi > >> queue logic. It might not be worth it to change the TUN's multi queue > >> logic so having a new file seems to be better. > > > > +1 on deduplicating further. But this series is complex enough. Let's not > > expand that. > > > > The latest approach with a separate .o file may have some performance > > cost by converting likely inlined code into real function calls. > > Another option is to move it all into tun_vnet.h. That also resolves > > the Makefile issues. > > I measured the size difference between the latest inlining approaches. > The numbers may vary depending on the system configuration of course, > but they should be useful for reference. > > The below shows sizes when having a separate module: 106496 bytes in total > > # lsmod > Module Size Used by > tap 28672 0 > tun 61440 0 > tun_vnet 16384 2 tun,tap > > The below shows sizes when inlining: 102400 bytes in total > > # lsmod > Module Size Used by > tap 32768 0 > tun 69632 0 > > So having a separate module costs 4096 bytes more. > > These two approaches should have similar tendency for run-time and > compile-time performance; the code is so trivial that the overhead of > having one additional module is dominant. The concern raised was not about object size, but about inlined vs true calls in the hot path. > The only downside of having all in tun_vnet.h is that it will expose its > internal macros and functions, which I think tolerable. As long as only included by tun.c and tap.c, I think that's okay. The slow path code (ioctl) could remain in a .c file. But it's simpler to just have the header file.
diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig index 1fd5acdc73c6..c420418473fc 100644 --- a/drivers/net/Kconfig +++ b/drivers/net/Kconfig @@ -395,6 +395,7 @@ config TUN tristate "Universal TUN/TAP device driver support" depends on INET select CRC32 + select TAP help TUN/TAP provides packet reception and transmission for user space programs. It can be viewed as a simple Point-to-Point or Ethernet diff --git a/drivers/net/Makefile b/drivers/net/Makefile index bb8eb3053772..2275309a97ee 100644 --- a/drivers/net/Makefile +++ b/drivers/net/Makefile @@ -29,9 +29,9 @@ obj-y += mdio/ obj-y += pcs/ obj-$(CONFIG_RIONET) += rionet.o obj-$(CONFIG_NET_TEAM) += team/ -obj-$(CONFIG_TUN) += tun-drv.o -tun-drv-y := tun.o tun_vnet.o -obj-$(CONFIG_TAP) += tap.o +obj-$(CONFIG_TUN) += tun.o +obj-$(CONFIG_TAP) += tap-drv.o +tap-drv-y := tap.o tun_vnet.o obj-$(CONFIG_VETH) += veth.o obj-$(CONFIG_VIRTIO_NET) += virtio_net.o obj-$(CONFIG_VXLAN) += vxlan/ diff --git a/drivers/net/tap.c b/drivers/net/tap.c index 7ee2e9ee2a89..4f3cc3b2e3c6 100644 --- a/drivers/net/tap.c +++ b/drivers/net/tap.c @@ -26,74 +26,9 @@ #include <linux/virtio_net.h> #include <linux/skb_array.h> -#define TAP_IFFEATURES (IFF_VNET_HDR | IFF_MULTI_QUEUE) - -#define TAP_VNET_LE 0x80000000 -#define TAP_VNET_BE 0x40000000 - -#ifdef CONFIG_TUN_VNET_CROSS_LE -static inline bool tap_legacy_is_little_endian(struct tap_queue *q) -{ - return q->flags & TAP_VNET_BE ? false : - virtio_legacy_is_little_endian(); -} - -static long tap_get_vnet_be(struct tap_queue *q, int __user *sp) -{ - int s = !!(q->flags & TAP_VNET_BE); - - if (put_user(s, sp)) - return -EFAULT; - - return 0; -} - -static long tap_set_vnet_be(struct tap_queue *q, int __user *sp) -{ - int s; - - if (get_user(s, sp)) - return -EFAULT; - - if (s) - q->flags |= TAP_VNET_BE; - else - q->flags &= ~TAP_VNET_BE; - - return 0; -} -#else -static inline bool tap_legacy_is_little_endian(struct tap_queue *q) -{ - return virtio_legacy_is_little_endian(); -} - -static long tap_get_vnet_be(struct tap_queue *q, int __user *argp) -{ - return -EINVAL; -} - -static long tap_set_vnet_be(struct tap_queue *q, int __user *argp) -{ - return -EINVAL; -} -#endif /* CONFIG_TUN_VNET_CROSS_LE */ - -static inline bool tap_is_little_endian(struct tap_queue *q) -{ - return q->flags & TAP_VNET_LE || - tap_legacy_is_little_endian(q); -} - -static inline u16 tap16_to_cpu(struct tap_queue *q, __virtio16 val) -{ - return __virtio16_to_cpu(tap_is_little_endian(q), val); -} +#include "tun_vnet.h" -static inline __virtio16 cpu_to_tap16(struct tap_queue *q, u16 val) -{ - return __cpu_to_virtio16(tap_is_little_endian(q), val); -} +#define TAP_IFFEATURES (IFF_VNET_HDR | IFF_MULTI_QUEUE) static struct proto tap_proto = { .name = "tap", @@ -655,25 +590,11 @@ static ssize_t tap_get_user(struct tap_queue *q, void *msg_control, if (q->flags & IFF_VNET_HDR) { vnet_hdr_len = READ_ONCE(q->vnet_hdr_sz); - err = -EINVAL; - if (iov_iter_count(from) < vnet_hdr_len) - goto err; - - err = -EFAULT; - if (!copy_from_iter_full(&vnet_hdr, sizeof(vnet_hdr), from)) - goto err; - iov_iter_advance(from, vnet_hdr_len - sizeof(vnet_hdr)); - if ((vnet_hdr.flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) && - tap16_to_cpu(q, vnet_hdr.csum_start) + - tap16_to_cpu(q, vnet_hdr.csum_offset) + 2 > - tap16_to_cpu(q, vnet_hdr.hdr_len)) - vnet_hdr.hdr_len = cpu_to_tap16(q, - tap16_to_cpu(q, vnet_hdr.csum_start) + - tap16_to_cpu(q, vnet_hdr.csum_offset) + 2); - err = -EINVAL; - if (tap16_to_cpu(q, vnet_hdr.hdr_len) > iov_iter_count(from)) + hdr_len = tun_vnet_hdr_get(vnet_hdr_len, q->flags, from, &vnet_hdr); + if (hdr_len < 0) { + err = hdr_len; goto err; - hdr_len = tap16_to_cpu(q, vnet_hdr.hdr_len); + } } len = iov_iter_count(from); @@ -731,8 +652,7 @@ static ssize_t tap_get_user(struct tap_queue *q, void *msg_control, skb->dev = tap->dev; if (vnet_hdr_len) { - err = virtio_net_hdr_to_skb(skb, &vnet_hdr, - tap_is_little_endian(q)); + err = tun_vnet_hdr_to_skb(q->flags, skb, &vnet_hdr); if (err) { rcu_read_unlock(); drop_reason = SKB_DROP_REASON_DEV_HDR; @@ -795,23 +715,17 @@ static ssize_t tap_put_user(struct tap_queue *q, int total; if (q->flags & IFF_VNET_HDR) { - int vlan_hlen = skb_vlan_tag_present(skb) ? VLAN_HLEN : 0; struct virtio_net_hdr vnet_hdr; vnet_hdr_len = READ_ONCE(q->vnet_hdr_sz); - if (iov_iter_count(iter) < vnet_hdr_len) - return -EINVAL; - if (virtio_net_hdr_from_skb(skb, &vnet_hdr, - tap_is_little_endian(q), true, - vlan_hlen)) - BUG(); - - if (copy_to_iter(&vnet_hdr, sizeof(vnet_hdr), iter) != - sizeof(vnet_hdr)) - return -EFAULT; + ret = tun_vnet_hdr_from_skb(q->flags, NULL, skb, &vnet_hdr); + if (ret) + return ret; - iov_iter_advance(iter, vnet_hdr_len - sizeof(vnet_hdr)); + ret = tun_vnet_hdr_put(vnet_hdr_len, iter, &vnet_hdr); + if (ret) + return ret; } total = vnet_hdr_len; total += skb->len; @@ -1070,42 +984,6 @@ static long tap_ioctl(struct file *file, unsigned int cmd, q->sk.sk_sndbuf = s; return 0; - case TUNGETVNETHDRSZ: - s = q->vnet_hdr_sz; - if (put_user(s, sp)) - return -EFAULT; - return 0; - - case TUNSETVNETHDRSZ: - if (get_user(s, sp)) - return -EFAULT; - if (s < (int)sizeof(struct virtio_net_hdr)) - return -EINVAL; - - q->vnet_hdr_sz = s; - return 0; - - case TUNGETVNETLE: - s = !!(q->flags & TAP_VNET_LE); - if (put_user(s, sp)) - return -EFAULT; - return 0; - - case TUNSETVNETLE: - if (get_user(s, sp)) - return -EFAULT; - if (s) - q->flags |= TAP_VNET_LE; - else - q->flags &= ~TAP_VNET_LE; - return 0; - - case TUNGETVNETBE: - return tap_get_vnet_be(q, sp); - - case TUNSETVNETBE: - return tap_set_vnet_be(q, sp); - case TUNSETOFFLOAD: /* let the user check for future flags */ if (arg & ~(TUN_F_CSUM | TUN_F_TSO4 | TUN_F_TSO6 | @@ -1149,7 +1027,7 @@ static long tap_ioctl(struct file *file, unsigned int cmd, return ret; default: - return -EINVAL; + return tun_vnet_ioctl(&q->vnet_hdr_sz, &q->flags, cmd, sp); } } @@ -1196,7 +1074,7 @@ static int tap_get_user_xdp(struct tap_queue *q, struct xdp_buff *xdp) skb->protocol = eth_hdr(skb)->h_proto; if (vnet_hdr_len) { - err = virtio_net_hdr_to_skb(skb, gso, tap_is_little_endian(q)); + err = tun_vnet_hdr_to_skb(q->flags, skb, gso); if (err) goto err_kfree; } diff --git a/drivers/net/tun_vnet.c b/drivers/net/tun_vnet.c index 5a6cbfb6eed0..960a5fa5a332 100644 --- a/drivers/net/tun_vnet.c +++ b/drivers/net/tun_vnet.c @@ -104,6 +104,7 @@ long tun_vnet_ioctl(int *sz, unsigned int *flags, return -EINVAL; } } +EXPORT_SYMBOL_GPL(tun_vnet_ioctl); int tun_vnet_hdr_get(int sz, unsigned int flags, struct iov_iter *from, struct virtio_net_hdr *hdr) @@ -125,6 +126,7 @@ int tun_vnet_hdr_get(int sz, unsigned int flags, struct iov_iter *from, return tun16_to_cpu(flags, hdr->hdr_len); } +EXPORT_SYMBOL_GPL(tun_vnet_hdr_get); int tun_vnet_hdr_put(int sz, struct iov_iter *iter, const struct virtio_net_hdr *hdr) @@ -139,12 +141,14 @@ int tun_vnet_hdr_put(int sz, struct iov_iter *iter, return 0; } +EXPORT_SYMBOL_GPL(tun_vnet_hdr_put); int tun_vnet_hdr_to_skb(unsigned int flags, struct sk_buff *skb, const struct virtio_net_hdr *hdr) { return virtio_net_hdr_to_skb(skb, hdr, tun_is_little_endian(flags)); } +EXPORT_SYMBOL_GPL(tun_vnet_hdr_to_skb); int tun_vnet_hdr_from_skb(unsigned int flags, const struct net_device *dev, @@ -173,3 +177,4 @@ int tun_vnet_hdr_from_skb(unsigned int flags, return 0; } +EXPORT_SYMBOL_GPL(tun_vnet_hdr_from_skb);
tun and tap implements the same vnet-related features so reuse the code. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> --- drivers/net/Kconfig | 1 + drivers/net/Makefile | 6 +- drivers/net/tap.c | 152 +++++-------------------------------------------- drivers/net/tun_vnet.c | 5 ++ 4 files changed, 24 insertions(+), 140 deletions(-)