Re: [PATCH V2 17/23] xen/ioreq: Introduce domain_has_ioreq_server()

[Oleksandr <olekstysh@gmail.com>]

On 11.11.20 10:08, Jan Beulich wrote:

Hi Jan

> On 10.11.2020 21:53, Oleksandr wrote:
>> On 20.10.20 13:51, Paul Durrant wrote:
>>
>> Hi Paul.
>>
>> Sorry for the late response.
>>
>>>> -----Original Message-----
>>>> From: Oleksandr Tyshchenko <olekstysh@gmail.com>
>>>> Sent: 15 October 2020 17:44
>>>> To: xen-devel@lists.xenproject.org
>>>> Cc: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>; Stefano Stabellini <sstabellini@kernel.org>;
>>>> Julien Grall <julien@xen.org>; Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Andrew Cooper
>>>> <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>; Ian Jackson
>>>> <iwj@xenproject.org>; Jan Beulich <jbeulich@suse.com>; Wei Liu <wl@xen.org>; Paul Durrant
>>>> <paul@xen.org>; Julien Grall <julien.grall@arm.com>
>>>> Subject: [PATCH V2 17/23] xen/ioreq: Introduce domain_has_ioreq_server()
>>>>
>>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>
>>>> This patch introduces a helper the main purpose of which is to check
>>>> if a domain is using IOREQ server(s).
>>>>
>>>> On Arm the current benefit is to avoid calling handle_io_completion()
>>>> (which implies iterating over all possible IOREQ servers anyway)
>>>> on every return in leave_hypervisor_to_guest() if there is no active
>>>> servers for the particular domain.
>>>> Also this helper will be used by one of the subsequent patches on Arm.
>>>>
>>>> This involves adding an extra per-domain variable to store the count
>>>> of servers in use.
>>>>
>>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>> CC: Julien Grall <julien.grall@arm.com>
>>>>
>>>> ---
>>>> Please note, this is a split/cleanup/hardening of Julien's PoC:
>>>> "Add support for Guest IO forwarding to a device emulator"
>>>>
>>>> Changes RFC -> V1:
>>>>      - new patch
>>>>
>>>> Changes V1 -> V2:
>>>>      - update patch description
>>>>      - guard helper with CONFIG_IOREQ_SERVER
>>>>      - remove "hvm" prefix
>>>>      - modify helper to just return d->arch.hvm.ioreq_server.nr_servers
>>>>      - put suitable ASSERT()s
>>>>      - use ASSERT(d->ioreq_server.server[id] ? !s : !!s) in set_ioreq_server()
>>>>      - remove d->ioreq_server.nr_servers = 0 from hvm_ioreq_init()
>>>> ---
>>>>    xen/arch/arm/traps.c    | 15 +++++++++------
>>>>    xen/common/ioreq.c      |  7 ++++++-
>>>>    xen/include/xen/ioreq.h | 14 ++++++++++++++
>>>>    xen/include/xen/sched.h |  1 +
>>>>    4 files changed, 30 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>>>> index 507c095..a8f5fdf 100644
>>>> --- a/xen/arch/arm/traps.c
>>>> +++ b/xen/arch/arm/traps.c
>>>> @@ -2261,14 +2261,17 @@ static bool check_for_vcpu_work(void)
>>>>        struct vcpu *v = current;
>>>>
>>>>    #ifdef CONFIG_IOREQ_SERVER
>>>> -    bool handled;
>>>> +    if ( domain_has_ioreq_server(v->domain) )
>>>> +    {
>>>> +        bool handled;
>>>>
>>>> -    local_irq_enable();
>>>> -    handled = handle_io_completion(v);
>>>> -    local_irq_disable();
>>>> +        local_irq_enable();
>>>> +        handled = handle_io_completion(v);
>>>> +        local_irq_disable();
>>>>
>>>> -    if ( !handled )
>>>> -        return true;
>>>> +        if ( !handled )
>>>> +            return true;
>>>> +    }
>>>>    #endif
>>>>
>>>>        if ( likely(!v->arch.need_flush_to_ram) )
>>>> diff --git a/xen/common/ioreq.c b/xen/common/ioreq.c
>>>> index bcd4961..a72bc0e 100644
>>>> --- a/xen/common/ioreq.c
>>>> +++ b/xen/common/ioreq.c
>>>> @@ -39,9 +39,14 @@ static void set_ioreq_server(struct domain *d, unsigned int id,
>>>>                                 struct ioreq_server *s)
>>>>    {
>>>>        ASSERT(id < MAX_NR_IOREQ_SERVERS);
>>>> -    ASSERT(!s || !d->ioreq_server.server[id]);
>>>> +    ASSERT(d->ioreq_server.server[id] ? !s : !!s);
>>> That looks odd. How about ASSERT(!s ^ !d->ioreq_server.server[id])?
>> ok, looks like it will work.
>>
>>
>>>     Paul
>>>
>>>>        d->ioreq_server.server[id] = s;
>>>> +
>>>> +    if ( s )
>>>> +        d->ioreq_server.nr_servers++;
>>>> +    else
>>>> +        d->ioreq_server.nr_servers--;
>>>>    }
>>>>
>>>>    #define GET_IOREQ_SERVER(d, id) \
>>>> diff --git a/xen/include/xen/ioreq.h b/xen/include/xen/ioreq.h
>>>> index 7b03ab5..0679fef 100644
>>>> --- a/xen/include/xen/ioreq.h
>>>> +++ b/xen/include/xen/ioreq.h
>>>> @@ -55,6 +55,20 @@ struct ioreq_server {
>>>>        uint8_t                bufioreq_handling;
>>>>    };
>>>>
>>>> +#ifdef CONFIG_IOREQ_SERVER
>>>> +static inline bool domain_has_ioreq_server(const struct domain *d)
>>>> +{
>>>> +    ASSERT((current->domain == d) || atomic_read(&d->pause_count));
>>>> +
>>> This seems like an odd place to put such an assertion.
>> I might miss something or interpreted incorrectly but these asserts are
>> the result of how I understood the review comment on previous version [1].
>>
>> I will copy a comment here for the convenience:
>> "This is safe only when d == current->domain and it's not paused,
>> or when they're distinct and d is paused. Otherwise the result is
>> stale before the caller can inspect it. This wants documenting by
>> at least a comment, but perhaps better by suitable ASSERT()s."
> The way his reply was worded, I think Paul was wondering about the
> place where you put the assertion, not what you actually assert.

Shall I put the assertion at the call sites of this helper instead?


>   
>
>>>> +    return d->ioreq_server.nr_servers;
>>>> +}
>>>> +#else
>>>> +static inline bool domain_has_ioreq_server(const struct domain *d)
>>>> +{
>>>> +    return false;
>>>> +}
>>>> +#endif
>>>> +
>>> Can this be any more compact? E.g.
>>>
>>> return IS_ENABLED(CONFIG_IOREQ_SERVER) && d->ioreq_server.nr_servers;
>>>
>>> ?
>> I have got a compilation error this way (if CONFIG_IOREQ_SERVER is
>> disabled):
>>
>> ...xen/4.14.0+gitAUTOINC+ee22110219-r0/git/xen/include/xen/ioreq.h:62:48:
>> error: ‘const struct domain’ has no member named ‘ioreq_server’
>>        return IS_ENABLED(CONFIG_IOREQ_SERVER) && d->ioreq_server.nr_servers;
>>                                                   ^
>> as domain's ioreq_server struct is guarded by CONFIG_IOREQ_SERVER as well.
> The #ifdef is unavoidable here, I agree, but it should be inside
> the function's body. There's no need to duplicate the rest of it.


Got it, will do.


-- 
Regards,

Oleksandr Tyshchenko