Re: [Xen-devel] [PATCH] evtchn/fifo: don't corrupt queues if an old tail is linked

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.12.13 at 12:49, David Vrabel <david.vrabel@xxxxxxxxxx> wrote:
> On 09/12/13 09:32, Jan Beulich wrote:
>>>>> On 06.12.13 at 18:38, David Vrabel <david.vrabel@xxxxxxxxxx> wrote:
>>> --- a/xen/include/xen/sched.h
>>> +++ b/xen/include/xen/sched.h
>>> @@ -98,6 +98,8 @@ struct evtchn
>>>      } u;
>>>      u8 priority;
>>>      u8 pending:1;
>>> +    u16 last_vcpu_id;
>>> +    u8 last_priority;
>> 
>> Is it really correct for these two new fields to remain uninitialized
>> until evtchn_fifo_set_pending() would get run the first time (and
>> hence thinking there was a move this first time through)?
> 
> They're initialized to zero and I think this is fine. The code as-is is
> simpler than having to special case events that have never been on a queue.

I'm not asking to add a special case, I'm only asking to initialize all
fields correctly. Just like you ought to set up ->priority, you
likely ought to set up the two new fields.

>> Which also gets me to ask whether it's really correct to only set
>> the priority to EVTCHN_FIFO_PRIORITY_DEFAULT in setup_ports(),
>> but not on any subsequently allocated/bound ones?
> 
> This patch fixes this but would you prefer a new evtchn_port_op hook for
> the init?

Not really, and yes please (or perhaps it would be cheaper to have
struct evtchn_port_ops specify the intended default priority, and
common code simply copy that field).

> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -141,6 +141,7 @@ static struct evtchn *alloc_evtchn_bucket(struct
> domain *d, unsigned int port)
>              return NULL;
>          }
>          chn[i].port = port + i;
> +        chn[i].priority = EVTCHN_FIFO_PRIORITY_DEFAULT;

"Not really" because this takes care of only the case where the
bucket gets allocated, but doesn't seem to take care of event
channel slots getting re-used.

Jan

> --- a/xen/common/event_fifo.c
> +++ b/xen/common/event_fifo.c
> @@ -444,7 +444,6 @@ static void setup_ports(struct domain *d)
>       * For each port that is already bound:
>       *
>       * - save its pending state.
> -     * - set default priority.
>       */
>      for ( port = 1; port < d->max_evtchns; port++ )
>      {
> @@ -457,8 +456,6 @@ static void setup_ports(struct domain *d)
> 
>          if ( test_bit(port, &shared_info(d, evtchn_pending)) )
>              evtchn->pending = 1;
> -
> -        evtchn_fifo_set_priority(d, evtchn, EVTCHN_FIFO_PRIORITY_DEFAULT);
>      }
>  }
> 
> David




_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel