Re: [PATCH] xen/events/fifo: Consume unprocessed events when a CPU dies

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 06/29/2015 06:19 AM, Ross Lagerwall wrote:
> On 06/19/2015 05:06 PM, David Vrabel wrote:
>> On 19/06/15 17:02, Boris Ostrovsky wrote:
>>> On 06/19/2015 11:15 AM, Ross Lagerwall wrote:
>>>> When a CPU is offlined, there may be unprocessed events on a port for
>>>> that CPU.  If the port is subsequently reused on a different CPU, it
>>>> could be in an unexpected state with the link bit set, resulting in
>>>> interrupts being missed. Fix this by consuming any unprocessed events
>>>> for a particular CPU when that CPU dies.
>>>>
>>>> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
>>>> ---
>>>>    drivers/xen/events/events_fifo.c | 23 ++++++++++++++++++-----
>>>>    1 file changed, 18 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/xen/events/events_fifo.c
>>>> b/drivers/xen/events/events_fifo.c
>>>> index 417415d..1dd0ba12 100644
>>>> --- a/drivers/xen/events/events_fifo.c
>>>> +++ b/drivers/xen/events/events_fifo.c
>>>> @@ -281,7 +281,8 @@ static void handle_irq_for_port(unsigned port)
>>>>      static void consume_one_event(unsigned cpu,
>>>>                      struct evtchn_fifo_control_block *control_block,
>>>> -                  unsigned priority, unsigned long *ready)
>>>> +                  unsigned priority, unsigned long *ready,
>>>> +                  bool drop)
>>>>    {
>>>>        struct evtchn_fifo_queue *q = &per_cpu(cpu_queue, cpu);
>>>>        uint32_t head;
>>>> @@ -313,13 +314,17 @@ static void consume_one_event(unsigned cpu,
>>>>        if (head == 0)
>>>>            clear_bit(priority, ready);
>>>>    -    if (evtchn_fifo_is_pending(port) && 
>>>> !evtchn_fifo_is_masked(port))
>>>> -        handle_irq_for_port(port);
>>>> +    if (evtchn_fifo_is_pending(port) && 
>>>> !evtchn_fifo_is_masked(port)) {
>>>> +        if (unlikely(drop))
>>>> +            pr_warn("Dropping pending event for port %u\n", port);
>>>
>>> Maybe pr_info (or pr_notice)?
>>
>> We want a warning here because we think this shouldn't happen -- if it
>> does we actually need to retrigger the event on its new CPU.
>>
>>> Also, why not do this (testing for unprocessed events) in
>>> xen_evtchn_close()?
>>
>> We can't do anything about them when closing because they may be in the
>> middle of a queue.

(Sorry, I missed this)

Why can't (actually, why doesn't) the cpu that is being offlined drain 
its queue?

-boris

>>
>> David
>>
>
> Ping. Is this change OK?
>