Re: [PATCH net v3 9/9] tap: Use tun's vnet-related code

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

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.