| Message ID | 20230328092847.91643-1-xuanzhuo@linux.alibaba.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | virtio-net: split virtio-net.c | expand |
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
On Thu, 30 Mar 2023 02:17:43 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote: > 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. Yes, this does need some time and energy. But I think this always need to do, just when to do it. I think it is currently an opportunity. > > 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. That's what I do. > 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. Yes, I agree, with the development of time, common will indeed become a dumping group. This is something we should pay attention to after this. > drivers/net/virtio/virtnet_virtio.c is weird with > virt repeated three times in the path. Any good idea. > > 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. One day we will face this problem. > It's now also extra work to keep headers self-consistent. Can we make it simpler, first complete the split. > So it better be a big improvement to be worth it. Or about split, do you have any better thoughts? Or do you think we have always been like this and make Virtio-Net more and more complicated? Thanks. > > > > > > 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 >
On Fri, Mar 31, 2023 at 3:31 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote: > > On Thu, 30 Mar 2023 02:17:43 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote: > > 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. > > Yes, this does need some time and energy. But I think this always need to do, > just when to do it. I think it is currently an opportunity. > > > > > > 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. > > That's what I do. > > > 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. > > Yes, I agree, with the development of time, common will indeed become a dumping > group. This is something we should pay attention to after this. > > > > drivers/net/virtio/virtnet_virtio.c is weird with > > virt repeated three times in the path. > > Any good idea. > > > > > 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. > > One day we will face this problem. > > > It's now also extra work to keep headers self-consistent. > > Can we make it simpler, first complete the split. > > > > So it better be a big improvement to be worth it. > > > Or about split, do you have any better thoughts? Or do you think we have always > been like this and make Virtio-Net more and more complicated? My feeling is that maybe it's worth it to start using a separate file for xsk support. Thanks > > > Thanks. > > > > > > > > > > > > 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 > > >
On Fri, 31 Mar 2023 15:35:14 +0800, Jason Wang <jasowang@redhat.com> wrote: > On Fri, Mar 31, 2023 at 3:31 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote: > > > > On Thu, 30 Mar 2023 02:17:43 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > 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. > > > > Yes, this does need some time and energy. But I think this always need to do, > > just when to do it. I think it is currently an opportunity. > > > > > > > > > > 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. > > > > That's what I do. > > > > > 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. > > > > Yes, I agree, with the development of time, common will indeed become a dumping > > group. This is something we should pay attention to after this. > > > > > > > drivers/net/virtio/virtnet_virtio.c is weird with > > > virt repeated three times in the path. > > > > Any good idea. > > > > > > > > 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. > > > > One day we will face this problem. > > > > > It's now also extra work to keep headers self-consistent. > > > > Can we make it simpler, first complete the split. > > > > > > > So it better be a big improvement to be worth it. > > > > > > Or about split, do you have any better thoughts? Or do you think we have always > > been like this and make Virtio-Net more and more complicated? > > My feeling is that maybe it's worth it to start using a separate file > for xsk support. I agree. @Michael at this point, what is your thought? Thanks. > > Thanks > > > > > > > Thanks. > > > > > > > > > > > > > > > > > > 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 > > > > > > > _______________________________________________ > Virtualization mailing list > Virtualization@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
On Fri, Mar 31, 2023 at 03:48:00PM +0800, Xuan Zhuo wrote: > On Fri, 31 Mar 2023 15:35:14 +0800, Jason Wang <jasowang@redhat.com> wrote: > > On Fri, Mar 31, 2023 at 3:31 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote: > > > > > > On Thu, 30 Mar 2023 02:17:43 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > 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. > > > > > > Yes, this does need some time and energy. But I think this always need to do, > > > just when to do it. I think it is currently an opportunity. > > > > > > > > > > > > > > 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. > > > > > > That's what I do. > > > > > > > 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. > > > > > > Yes, I agree, with the development of time, common will indeed become a dumping > > > group. This is something we should pay attention to after this. > > > > > > > > > > drivers/net/virtio/virtnet_virtio.c is weird with > > > > virt repeated three times in the path. > > > > > > Any good idea. > > > > > > > > > > > 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. > > > > > > One day we will face this problem. > > > > > > > It's now also extra work to keep headers self-consistent. > > > > > > Can we make it simpler, first complete the split. > > > > > > > > > > So it better be a big improvement to be worth it. > > > > > > > > > Or about split, do you have any better thoughts? Or do you think we have always > > > been like this and make Virtio-Net more and more complicated? > > > > My feeling is that maybe it's worth it to start using a separate file > > for xsk support. > > I agree. > > @Michael at this point, what is your thought? > > > Thanks. > I am fine with either adding just xsk in a new file or even just adding in same file working on a split later. > > > > Thanks > > > > > > > > > > > Thanks. > > > > > > > > > > > > > > > > > > > > > > > > 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 > > > > > > > > > > > _______________________________________________ > > Virtualization mailing list > > Virtualization@lists.linux-foundation.org > > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
On Fri, 31 Mar 2023 04:00:22 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Fri, Mar 31, 2023 at 03:48:00PM +0800, Xuan Zhuo wrote: > > On Fri, 31 Mar 2023 15:35:14 +0800, Jason Wang <jasowang@redhat.com> wrote: > > > On Fri, Mar 31, 2023 at 3:31 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote: > > > > > > > > On Thu, 30 Mar 2023 02:17:43 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > 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. > > > > > > > > Yes, this does need some time and energy. But I think this always need to do, > > > > just when to do it. I think it is currently an opportunity. > > > > > > > > > > > > > > > > > > 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. > > > > > > > > That's what I do. > > > > > > > > > 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. > > > > > > > > Yes, I agree, with the development of time, common will indeed become a dumping > > > > group. This is something we should pay attention to after this. > > > > > > > > > > > > > drivers/net/virtio/virtnet_virtio.c is weird with > > > > > virt repeated three times in the path. > > > > > > > > Any good idea. > > > > > > > > > > > > > > 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. > > > > > > > > One day we will face this problem. > > > > > > > > > It's now also extra work to keep headers self-consistent. > > > > > > > > Can we make it simpler, first complete the split. > > > > > > > > > > > > > So it better be a big improvement to be worth it. > > > > > > > > > > > > Or about split, do you have any better thoughts? Or do you think we have always > > > > been like this and make Virtio-Net more and more complicated? > > > > > > My feeling is that maybe it's worth it to start using a separate file > > > for xsk support. > > > > I agree. > > > > @Michael at this point, what is your thought? > > > > > > Thanks. > > > > I am fine with either adding just xsk in a new file or even > just adding in same file working on a split later. Okay, we are on the same page with xsk. So the remaining problem is whether do we need to split ethtool, ctrl command, and virtio out? I think at least ctrl command and ethtool are very clear. The split of virtio has indeed encountered some problems. I have independent modifications into a separate commit. The patches to move the code do not include any other modifications. Thanks. > > > > > > > > Thanks > > > > > > > > > > > > > > > Thanks. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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 > > > > > > > > > > > > > > > _______________________________________________ > > > Virtualization mailing list > > > Virtualization@lists.linux-foundation.org > > > https://lists.linuxfoundation.org/mailman/listinfo/virtualization >
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. 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