Re: [RFC PATCH 00/47] Unifying LKL into UML

From: Hajime Tazaki<thehajime@gmail.com>
To: johannes@sipsolutions.net
Cc: tavi.purdila@gmail.com, linux-um@lists.infradead.org,
	retrage01@gmail.com
Subject: Re: [RFC PATCH 00/47] Unifying LKL into UML
Date: Wed, 30 Oct 2019 00:45:43 +0900	[thread overview]
Message-ID: <m27e4n4kt4.wl-thehajime@gmail.com> (raw)
In-Reply-To: <179d1a35e32288b22b96545f21e141ba57265025.camel@sipsolutions.net>

Hello Johannes,

Thanks for the review.

On Tue, 29 Oct 2019 16:57:54 +0900,
Johannes Berg wrote:
> 
> On Wed, 2019-10-23 at 13:37 +0900, Hajime Tazaki wrote:
> > 
> > LKL (Linux Kernel Library) is aiming to allow reusing the Linux kernel code
> > as extensively as possible with minimal effort and reduced maintenance
> > overhead.
> 
> [snip]
> 
> Can you comment a bit on what's what?
> 
> For example, I look at patch 24 ("lkl tools: virtio: add network device
> support") and wonder what that really is?

Indeed....  We will describe more to be understandable.

> Is it a hypervisor-side implementation of the virtio-net device? If so,
> why is that considered "tools"? (In ARCH=um, the hv-code usually lives in
> arch/um/drivers/*_user.c or similar.)

Yes, this was a device (hv)-side implementation.

LKL currently takes 2-stage build: pure kernel part (arch/um/lkl) and
userspace one (tools/lkl).  Though I think it's good to have clear
distinction in different directories, we can move tools/lkl under arch/um, if
this is the better way.

> Also, taking that as an example again, I think that's something we
> should rather leave out initially - it looks like it has hooks into DPDK
> and all kinds of other network interfaces on the host, which duplicates
> a lot of existing functionality in ARCH=um.
> 
> Additionally, we (Intel) recently contributed a vhost-user backend, so
> we don't really *need* a hypervisor implementation of e.g. DPDK
> integration at all, that should be possible over vhost-user instead.

I understand.

If there is a need for in-process virtio device implementation (as LKL
does), connecting to vanilla virtio mmio driver, I hope it's still
valuable.

> Looking further at the series - many of your patches really need better
> commit logs explaining what and why they do something. Particularly the
> reverts, but even trivial patches like the first one in the series.

thanks.  the revert commits are required for Windows host target, which
uses '_' prefix for the symbols.

We will refine the log thorough the all commits and get you back.

> patch 2: doesn't explain why it's necessary - how is this not covered by
> adding a "config SOMETHING\n  def_bool y" in the architecture?

ah, good catch.
maybe giving an example of the contents of auto.conf should be helpful.
We will address this in next patchset.

> patch 4: kernel-doc doesn't parse, it's also very awkward to add this
> without any users, why not add a very simple version of the struct with
> the first patch needing it, and then extend it in each next patch?

I agree to start with simple struct and expand it afterward.  We will
address this in next round.

kernel-doc is something we didn't test so far.
We will also test in advance.

> [oops, out of time, will continue later]

Thank you again for the detailed review.

-- Hajime

_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um