rapido-linux / rapido

Quickly test Linux kernel changes
GNU Lesser General Public License v2.1
22 stars 22 forks source link

Add net test #230

Closed acooks closed 11 months ago

acooks commented 12 months ago

Thank you for creating and maintaining rapido!

It's been really useful for testing a network device driver I'm working on and I would like to share the changes I've made.

ddiss commented 11 months ago

Thanks for the PR! As @pevik indicated, this appears to add quite a bit of extra complexity to rapido.conf. Hopefully some of it can be avoided by reworking the changes a little:

I'll add a few other minor style comments inline...

acooks commented 11 months ago

Thank you for the quick review and feedback!

* your use case appears to target tn40xx-driver development, so I think it'd be nice to have tn40xx specific cut/autorun scripts instead of generic net-test

Are you sure you'd want to be so specific about the driver?

I started from the very specific case, but thought it was too specific to upstream, because it wouldn't be useful to anyone else. However, the generic case for testing drivers for passed-through devices seems like more than just a once-off need.

For example, it might also be useful for these similar out-of-mainline network drivers: https://github.com/morrownr/8814au https://github.com/Mange/rtl8192eu-linux-driver/ https://github.com/lwfinger/rtl8192du

The goal for these drivers is probably to cease stand-alone development and be superseded by a mainline driver, but even then, the rapido developer workflow that enables passing the peripheral and driver into a VM is so much better than any of the alternatives I've seen (and not specific to network devices, or pci devices, or even device drivers).

ddiss commented 11 months ago

Thank you for the quick review and feedback!

No worries :-)

* your use case appears to target tn40xx-driver development, so I think it'd be nice to have tn40xx specific cut/autorun scripts instead of generic net-test

Are you sure you'd want to be so specific about the driver?

I started from the very specific case, but thought it was too specific to upstream, because it wouldn't be useful to anyone else. However, the generic case for testing drivers for passed-through devices seems like more than just a once-off need.

For example, it might also be useful for these similar out-of-mainline network drivers: https://github.com/morrownr/8814au https://github.com/Mange/rtl8192eu-linux-driver/ https://github.com/lwfinger/rtl8192du

Understood. Given that the test environment, kernel module and test suite currently appear to be tn40xx specific, I think it still makes sense to keep it separate for now. Similar future (e.g. rtlX) network driver environments could reuse your scripts where needed (e.g. via autorun/lib/).

The goal for these drivers is probably to cease stand-alone development and be superseded by a mainline driver, but even then, the rapido developer workflow that enables passing the peripheral and driver into a VM is so much better than any of the alternatives I've seen (and not specific to network devices, or pci devices, or even device drivers).

Sounds like a worthy goal. Glad to hear that the rapido workflow is useful for others.

ddiss commented 11 months ago

Thanks for the update, but this doesn't appear to address many of the previous comments:

+       --include "${KERNEL_SRC}/samples/pktgen" "pktgen" \
+       --include "${NET_TEST_SUITE}" "driver-tests" \

Please use the host path in the VM:

+       --include "${KERNEL_SRC}/samples/pktgen" "${KERNEL_SRC}/samples/pktgen" \
+       --include "${TN40XX_DRIVER_SRC}/test" "${TN40XX_DRIVER_SRC}/test" \
+/driver-tests/start.sh ${NET_TEST_DEV_VM} ${NET_TEST_KMOD}

Depending on whether you think you'll only ever be running start.sh, you might want the autorun script to just have:

cd ${TN40XX_DRIVER_SRC}/test

you could then trigger the script via ./rapido cut -x "./start.sh DEV tn40xx" tn40xx

acooks commented 11 months ago

Thanks for the update, but this doesn't appear to address many of the previous comments:

* please rename it to match the specific functionality/driver to be tested, e.g. `cut/tn40xx.sh` and `autorun/tn40xx.sh`

* avoid adding new rapido.conf variables where possible

  * hardcode `NET_TEST_KMOD` as `tn40xx`
  * add a `TN40XX_DRIVER_SRC` variable, which is used instead of `NET_TEST_SUITE`
  * drop `NET_TEST_DEV_HOST`

    * instead you could check that `QEMU_EXTRA_ARGS` includes a `vfio-pci` device, similar to what we do in `cut/ocfs2.sh`
  * drop `NET_TEST_DEV_VM`: can this be hardcoded or discovered in the autorun script?
+       --include "${KERNEL_SRC}/samples/pktgen" "pktgen" \
+       --include "${NET_TEST_SUITE}" "driver-tests" \

Please use the host path in the VM:

+       --include "${KERNEL_SRC}/samples/pktgen" "${KERNEL_SRC}/samples/pktgen" \
+       --include "${TN40XX_DRIVER_SRC}/test" "${TN40XX_DRIVER_SRC}/test" \
+/driver-tests/start.sh ${NET_TEST_DEV_VM} ${NET_TEST_KMOD}

Depending on whether you think you'll only ever be running start.sh, you might want the autorun script to just have:

cd ${TN40XX_DRIVER_SRC}/test

you could then trigger the script via ./rapido cut -x "./start.sh DEV tn40xx" tn40xx

Thank you for the feedback. It's clear enough what you expect and I am still trying to incorporate it, but I'm not yet able to see a way to implement it in such a way that this software does what I need it to do.

With the changes in the PR I was able to do basic network device driver tests, like:

These tests are obviously still quite basic and immature, but I couldn't find an existing reusable test suite and I need it, so I guess I'm building something.

The next tests I need to write will have to send and receive some actual network traffic and to do that, I'm working towards spinning up multiple VMs, passing a different PCI device with its appropriate driver to each VM from the host, and running the appropriate scripts to support support the test.

And that work will be balanced with working on the actual tn40xx driver.

I'll close this PR for now and perhaps try again when I have a better idea of how to get to an acceptable solution that still meets my needs.

Thanks again!

ddiss commented 11 months ago

...

Thank you for the feedback. It's clear enough what you expect and I am still trying to incorporate it, but I'm not yet able to see a way to implement it in such a way that this software does what I need it to do.

With the changes in the PR I was able to do basic network device driver tests, like:

* loading and unloading the kernel module,

* setting the network interface up and down,

* changing MTU

These tests are obviously still quite basic and immature, but I couldn't find an existing reusable test suite and I need it, so I guess I'm building something.

The next tests I need to write will have to send and receive some actual network traffic and to do that, I'm working towards spinning up multiple VMs, passing a different PCI device with its appropriate driver to each VM from the host, and running the appropriate scripts to support support the test.

Sounds good Andrew. FWIW, I'd still very much like to carry your changes upstream. I'd just prefer to keep things relatively self-contained and non-generic.

And that work will be balanced with working on the actual tn40xx driver.

I'll close this PR for now and perhaps try again when I have a better idea of how to get to an acceptable solution that still meets my needs.

Please do try to resubmit when you have something somewhat self-contained that you're happy with.