netdev - Re: [PATCH 00/16] virtio-net: split virtio-net.c

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20230330015412-mutt-send-email-mst@kernel.org>
Date:   Thu, 30 Mar 2023 02:17:43 -0400
From:   "Michael S. Tsirkin" <mst@...hat.com>
To:     Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
Cc:     netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Jason Wang <jasowang@...hat.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        John Fastabend <john.fastabend@...il.com>,
        virtualization@...ts.linux-foundation.org, bpf@...r.kernel.org
Subject: Re: [PATCH 00/16] virtio-net: split virtio-net.c

On Tue, Mar 28, 2023 at 05:28:31PM +0800, Xuan Zhuo wrote:
> Considering the complexity of virtio-net.c and the new features we want
> to add, it is time to split virtio-net.c into multiple independent
> module files.
> 
> This is beneficial to the maintenance and adding new functions.
> 
> And AF_XDP support will be added later, then a separate xsk.c file will
> be added.
> 
> This patchset split virtio-net.c into these parts:
> 
> * virtnet.c:         virtio net device ops (napi, tx, rx, device ops, ...)
> * virtnet_common.c:  virtio net common code
> * virtnet_ethtool.c: virtio net ethtool callbacks
> * virtnet_ctrl.c:    virtio net ctrl queue command APIs
> * virtnet_virtio.c:  virtio net virtio callbacks/ops (driver register, virtio probe, virtio free, ...)
> 
> Please review.
> 
> Thanks.


I don't feel this is an improvement as presented, will need more work
to make code placement more logical.

For example where do I find code to update rq stats?
Rx data path should be virtnet.c?
No it's in virtnet_ethtool.c because rq stats can be
accessed by ethtool.
A bunch of stuff seems to be in headers just because of technicalities.
virtio common seems to be a dumping ground with no guiding principle at
all.
drivers/net/virtio/virtnet_virtio.c is weird with
virt repeated three times in the path.

These things only get murkier with time, at the point of reorg
I would expect very logical placement, since
without clear guiding rule finding where something is becomes harder but
more importantly we'll now get endless heartburn about where does each new
function go.


The reorg is unfortunately not free - for example git log --follow will
no longer easily match virtio because --follow works with exactly one
path.  It's now also extra work to keep headers self-consistent.
So it better be a big improvement to be worth it.




> Xuan Zhuo (16):
>   virtio_net: add a separate directory for virtio-net
>   virtio_net: move struct to header file
>   virtio_net: add prefix to the struct inside header file
>   virtio_net: separating cpu-related funs
>   virtio_net: separate virtnet_ctrl_set_queues()
>   virtio_net: separate virtnet_ctrl_set_mac_address()
>   virtio_net: remove lock from virtnet_ack_link_announce()
>   virtio_net: separating the APIs of cq
>   virtio_net: introduce virtnet_rq_update_stats()
>   virtio_net: separating the funcs of ethtool
>   virtio_net: introduce virtnet_dev_rx_queue_group()
>   virtio_net: introduce virtnet_get_netdev()
>   virtio_net: prepare for virtio
>   virtio_net: move virtnet_[en/dis]able_delayed_refill to header file
>   virtio_net: add APIs to register/unregister virtio driver
>   virtio_net: separating the virtio code
> 
>  MAINTAINERS                                   |    2 +-
>  drivers/net/Kconfig                           |    8 +-
>  drivers/net/Makefile                          |    2 +-
>  drivers/net/virtio/Kconfig                    |   11 +
>  drivers/net/virtio/Makefile                   |   10 +
>  .../net/{virtio_net.c => virtio/virtnet.c}    | 2368 ++---------------
>  drivers/net/virtio/virtnet.h                  |  213 ++
>  drivers/net/virtio/virtnet_common.c           |  138 +
>  drivers/net/virtio/virtnet_common.h           |   14 +
>  drivers/net/virtio/virtnet_ctrl.c             |  272 ++
>  drivers/net/virtio/virtnet_ctrl.h             |   45 +
>  drivers/net/virtio/virtnet_ethtool.c          |  578 ++++
>  drivers/net/virtio/virtnet_ethtool.h          |    8 +
>  drivers/net/virtio/virtnet_virtio.c           |  880 ++++++
>  drivers/net/virtio/virtnet_virtio.h           |    8 +
>  15 files changed, 2366 insertions(+), 2191 deletions(-)
>  create mode 100644 drivers/net/virtio/Kconfig
>  create mode 100644 drivers/net/virtio/Makefile
>  rename drivers/net/{virtio_net.c => virtio/virtnet.c} (50%)
>  create mode 100644 drivers/net/virtio/virtnet.h
>  create mode 100644 drivers/net/virtio/virtnet_common.c
>  create mode 100644 drivers/net/virtio/virtnet_common.h
>  create mode 100644 drivers/net/virtio/virtnet_ctrl.c
>  create mode 100644 drivers/net/virtio/virtnet_ctrl.h
>  create mode 100644 drivers/net/virtio/virtnet_ethtool.c
>  create mode 100644 drivers/net/virtio/virtnet_ethtool.h
>  create mode 100644 drivers/net/virtio/virtnet_virtio.c
>  create mode 100644 drivers/net/virtio/virtnet_virtio.h
> 
> --
> 2.32.0.3.g01195cf9f