Re: [PATCH v4 2/6] introduce UFFD-WP low-level interface helpers

[Andrey Gruzdev <andrey.gruzdev@virtuozzo.com>]

On 30.11.2020 18:34, Peter Xu wrote:
> On Sun, Nov 29, 2020 at 11:12:10PM +0300, Andrey Gruzdev wrote:
>>>> +void ram_write_tracking_stop(void)
>>>> +{
>>>> +#ifdef CONFIG_LINUX
>>>> +    RAMState *rs = ram_state;
>>>> +    RAMBlock *bs;
>>>> +    assert(rs->uffdio_fd >= 0);
>>>
>>> Maybe too harsh - we can return if it's invalid.
>>>
>>> Meanwhile, better rcu_read_lock(), as well?
>>>
>>
>> Yep, RCU lock, I'll add. Why too harsh? Just a debug assertion.
> 
> I was afraid some special path could trigger ram_write_tracking_stop() being
> called before ram_write_tracking_start(), then vm could crash.  If we can
> guarantee that not happening, then it's also ok with assert().
> 
> [...]
> 

It can't really happen, but I agree that assert() is a bit excessive here.

>>>> +/**
>>>> + * uffd_poll_events: poll UFFD file descriptor for read
>>>> + *
>>>> + * Returns true if events are available for read, false otherwise
>>>> + *
>>>> + * @uffd: UFFD file descriptor
>>>> + * @tmo: timeout in milliseconds, 0 for non-blocking operation,
>>>> + *       negative value for infinite wait
>>>> + */
>>>> +bool uffd_poll_events(int uffd, int tmo)
>>>
>>> Shall we spell "tmo" out?
>> In the comment? I think it's ok.
> 
> I'd suggest to spell it out everywhere, especially in the codes.  But feel free
> to take your own preference.  Thanks,
> 

Thanks,

-- 
Andrey Gruzdev, Principal Engineer
Virtuozzo GmbH  +7-903-247-6397
                 virtuzzo.com