Re: [Qemu-devel] [PATCH v2 0/2] e1000e: Reimplement e1000 as a variant of e1000e

[Daniel P. Berrange]

FWIW, it is generally recommended that new versions of patch series be
posted as a new top level thread, not in-reply-to the previous version.
That way the new posting gets attention near top of a dev's email inbox,
as opposed to hidden away as a reply to a weeks old msg at the back of
the inbox.

On Wed, Nov 08, 2017 at 08:39:15PM -0800, Ed Swierk via Qemu-devel wrote:
> From a hardware functionality and register perspective, the Intel
> 8254x Gigabit Ethernet Controller[1] is roughly a subset of the Intel
> 82574[2], making it practical to implement e1000 device emulation as
> an extension of e1000e.
> 
> That would be a step towards eliminating the existing e1000
> implementation--a bunch of semi-redundant code with its own bugs (like
> the faulty tx checksum offload I reported recently[3]).
> 
> [1] 
> https://www.intel.com/content/dam/doc/manual/pci-pci-x-family-gbe-controllers-software-dev-manual.pdf
> [2] 
> https://www.intel.com/content/dam/doc/datasheet/82574l-gbe-controller-datasheet.pdf
> [3] https://lists.nongnu.org/archive/html/qemu-devel/2017-10/msg03008.html
> 
> This patch series adds a new e1000-ng device but leaves the existing
> e1000 device alone. Linux 4.1 and Windows 10 e1000 guest drivers
> recognize the e1000-ng device and successfully pass traffic on a Linux
> host.
> 
> Preliminary performance measurements are encouraging: with e1000-ng,
> single-threaded TCP iperf shows about a 2x speedup in tx and rx
> throughput vs e1000, and CPU usage is slighly lower.

That is certainly attractive.

> A ton of work is needed before e1000-ng will be ready to supplant
> e1000: testing with every random guest OS, fixing bugs, figuring out
> migration and other missing functionality. There's no way I can do
> this myself.
> 
> I'd like to propose a more modest and achievable short-term goal: find
> and fix any regressions that might affect users of e1000e and e1000,
> so that the patches can be applied and folks can easily start trying
> out e1000-ng with their favorite guest OS. That means accepting (for
> now) known deficiencies in the new e1000-ng devices, which can be
> addressed by myself and (hopefully) others in follow-up patches. Does
> that sound reasonable?

My fear is that this approach of building a new e1000-ng device in
parallel with having the existing e1000 device is going to cause
long term pain, possibly never getting to a state where the e1000-ng
device can replace the e1000 device. Any time there needs to be a
"big bang" to switch from one impl to another completely different
impl always causes trouble IME. With need for migration wire format
& state compatibility, this is even more difficult. From a code review
POV it will be essentially impossible to have confidence that the new
impl can be a viable drop-in replacement for the old impl.

Is there really no way that you can change the approach to do a more
incremental conversion of the existing code base, but still end up in
the same place at the very end ?

eg just copy all the e1000.c code into the e1000e.c file to start with.
Then gradually merge functional areas over a longish series of patches
to eliminate the duplication. This would make it far more practical to
identify where any regressions come from, and will give reviewers more
confidence that we're not breaking migration compat.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|