Re: [dpdk-dev] [04/12] vhost: introduce postcopy's advise message

[Ilya Maximets <i.maximets@samsung.com>]

On 28.09.2018 15:13, Bruce Richardson wrote:
> On Fri, Sep 28, 2018 at 01:40:25PM +0300, Ilya Maximets wrote:
>> On 27.09.2018 11:28, Ilya Maximets wrote:
>>> On 26.09.2018 10:26, Maxime Coquelin wrote:
>>>> This patch opens a userfaultfd and sends it back to Qemu's
>>>> VHOST_USER_POSTCOPY_ADVISE request.
>>>>
>>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>> ---
>>>>  lib/librte_vhost/vhost.h      |  2 ++
>>>>  lib/librte_vhost/vhost_user.c | 37 +++++++++++++++++++++++++++++++++++
>>>>  lib/librte_vhost/vhost_user.h |  3 ++-
>>>>  3 files changed, 41 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
>>>> index 25ffd7614..21722d8a8 100644
>>>> --- a/lib/librte_vhost/vhost.h
>>>> +++ b/lib/librte_vhost/vhost.h
>>>> @@ -363,6 +363,8 @@ struct virtio_net {
>>>>  	int			slave_req_fd;
>>>>  	rte_spinlock_t		slave_req_lock;
>>>>  
>>>> +	int			postcopy_ufd;
>>>> +
>>>>  	/*
>>>>  	 * Device id to identify a specific backend device.
>>>>  	 * It's set to -1 for the default software implementation.
>>>> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
>>>> index a9b429598..bdfe2cac0 100644
>>>> --- a/lib/librte_vhost/vhost_user.c
>>>> +++ b/lib/librte_vhost/vhost_user.c
>>>> @@ -24,9 +24,13 @@
>>>>  #include <stdlib.h>
>>>>  #include <string.h>
>>>>  #include <unistd.h>
>>>> +#include <fcntl.h>
>>>> +#include <linux/userfaultfd.h>
>>>
>>> Maybe we need compile time check for this header existence?
>>> Otherwise, this will bump minimal kernel version for default linux build
>>> to something like 4.3.
>>
>> We'll need a config option here (disabled by default) and guard all
>> the postcopy related code.
>> Meson build will be able to detect the header file and enable
>> the config if possible. Like this:
>>
>> lib/librte_vhost/meson.build:
>> if cc.has_header('linux/userfaultfd.h')
>>        dpdk_conf.set10('RTE_LIBRTE_VHOST_POSTCOPY', true)
> 
> Are you sure you want 'set10' rather than 'set'. Set is probably easier
> because it ensures no define on false, while set10 has a define of 0. This
> has caught me out before.
> 
> FYI, you can also avoid the if by putting the condition into the define:
> 
> 	dpdk_conf.set('RTE_LIBRTE_VHOST_POSTCOPY', cc.has_header('...'))

Sure, this variant looks better. Thanks for suggestions.
I just copied my version from the similar code for 'RTE_HAS_LIBNUMA'.

Best regards, Ilya Maximets.