[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PULL 0/4] RDMA patches

From: Marcel Apfelbaum
Subject: Re: [Qemu-devel] [PULL 0/4] RDMA patches
Date: Thu, 8 Feb 2018 16:12:17 +0200
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:52.0) Gecko/20100101 Thunderbird/52.6.0

On 08/02/2018 16:01, Philippe Mathieu-Daudé wrote:
> Hi Marcel,
> On 02/08/2018 10:38 AM, Marcel Apfelbaum wrote:
>> On 08/02/2018 14:59, Peter Maydell wrote:
>>> On 5 February 2018 at 10:26, Marcel Apfelbaum <address@hidden> wrote:
>>>> The following changes since commit 
>>>> f24ee107a07f093bd7ed475dd48d7ba57ea3d8fe:
>>> Hi. The technical details of this pullreq are all fine (pgp
>>> key, format, etc), and it passes my build tests. But I gave
>>> this pullreq a bit of a closer inspection than I normally
>>> would, since it's your first, and there are a few things I
>>> thought worth bringing up:
>> Thanks for doing it!
>>> (1) I notice that some of the new files in this pullreq are licensed
>>> as "GPL, version 2", rather than "version 2 or any later version".
>>> Did you really mean that? Per 'LICENSE', we have a strong preference
>>> for 2-or-later for new code.
>> No real preference, I will modify the license.
>>> (2) Some new files have no copyright or license comment at the
>>> top of them. Can you fix that, please?
>> Sure.
>>> (3) Some of the new headers use kernel-internals __u32 etc types.
>>> This isn't portable. ('HACKING' has some suggestions for types you
>>> might want instead.)
>> We do not "use" the __u32 types, we just copied a kernel file
>> for structures used for communication between the guest driver
>> and the QEMU code. We had a look on how it is done and
>> we use the model that adds macros __u32 -> uint32_t,
>> so the "__types" do not really create such problems.
>>> (4) One of your patches doesn't have any reviewed-by tags.
>>> We don't always manage to review everything, but it is
>>> nicer if we can get review, especially for patches from
>>> new submaintainers.
>> The patch did receive several questions/comments and all
>> of them were addressed, but indeed no RB tag was given.
>> Since the patch was in the mailing list for over a month
>> and *was* reviewed, I thought is enough.
>> I will ping Eduardo, he had the latest comments for it.
>>> (5) This is an absolutely enormous diffstat for a single commit:
>>>  26 files changed, 5149 insertions(+), 4 deletions(-)
>> On the github where the project was developed we have thousands of commits,
>> so it can't be used.
>> It was reviewed closely by one reviewer and got a lot
>> of comments from others.
>> That being said, we will try to split it in a few patches
>> and send a new version.
> I spent some time to review this but got lost when it became too
> specific (this is not really my area).
> I was hoping some of the VMware folks could review this.

The code was reviewed by someone from Mellanox and
we have an RDMA developer from Oracle looking into it to.
For V10 we will have a second RB for the device, it should be enough.

> KVM related stuffs are hard to test, but we have some qtests (migration
> mostly). Adding some tests for this huge code addition would be really
> great.

It is in our plans, yes.
We saw the avocado guys are returning to QEMU, I will ask their help in setting
up an avocado test. It should take some time and we are developing the device
over an year offline making it had to maintain/review, so
we will work on the tests in parallel with the device submission.


> Regards,
> Phil.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]