Re: [PATCH 16/25] argo: implement the notify op

["Jan Beulich" <JBeulich@suse.com>]

>>> On 20.12.18 at 07:12, <christopher.w.clark@gmail.com> wrote:
> On Thu, Dec 13, 2018 at 6:06 AM Jan Beulich <JBeulich@suse.com> wrote:
>>
>> >>> On 01.12.18 at 02:32, <christopher.w.clark@gmail.com> wrote:
>> > +static uint32_t
>> > +argo_ringbuf_payload_space(struct domain *d, struct argo_ring_info *ring_info)
>> > +{
>> > +    argo_ring_t ring;
>> > +    int32_t ret;
>> > +
>> > +    ASSERT(spin_is_locked(&ring_info->lock));
>> > +
>> > +    ring.len = ring_info->len;
>> > +    if ( !ring.len )
>> > +        return 0;
>> > +
>> > +    ring.tx_ptr = ring_info->tx_ptr;
>> > +
>> > +    if ( argo_ringbuf_get_rx_ptr(ring_info, &ring.rx_ptr) )
>> > +        return 0;
>> > +
>> > +    argo_dprintk("argo_ringbuf_payload_space: tx_ptr=%d rx_ptr=%d\n",
>> > +                 ring.tx_ptr, ring.rx_ptr);
>> > +
>> > +    if ( ring.rx_ptr == ring.tx_ptr )
>> > +        return ring.len - sizeof(struct argo_ring_message_header);
>> > +
>> > +    ret = ring.rx_ptr - ring.tx_ptr;
>> > +    if ( ret < 0 )
>> > +        ret += ring.len;
>>
>> Seeing these two if()-s - how is an empty ring distinguished from
>> a completely full one? I'm getting the impression that
>> ring.rx_ptr == ring.tx_ptr in both cases.
> 
> The subtraction from ring.len above is missing an additional subtraction of
> ARGO_ROUNDUP(1), which doesn't help reasoning about this. (Fixed in v2.)
> 
> If rx_ptr == tx_ptr, then the ring is empty. The ring insertion
> functions won't allow filling the ring, and I've added more comments
> in the v2 code to explain.
> 
>> > +    ret -= sizeof(struct argo_ring_message_header);
>> > +    ret -= ARGO_ROUNDUP(1);
>>
>> Wouldn't you instead better round ret to a suitable multiple of
>> whatever granularity you try to arrange for here? Otherwise
>> what is this extra subtraction supposed to do?
> 
> re: subtraction, have added new comment:
> /*
>  * The maximum size payload for a message that will be accepted is:
>  * (the available space between the ring indexes)
>  *    minus (space for a message header)
>  *    minus (space for one message slot)
>  * since argo_ringbuf_insert requires that one message slot be left
>  * unfilled, to avoid filling the ring to capacity and confusing a full
>  * ring with an empty one.
>  */
> 
> re: rounding: Possibly. Not sure. In practice, both sides are
> updating the indexes in quantized steps matching the
> ARGO_ROUNDUP unit. Not sure it needs to change.

Here you appear to talk about both sides being well behaved. Did
you also consider misbehaving partners?

>> > +typedef struct argo_ring_data
>> > +{
>> > +    uint64_t magic;
>>
>> What is this good for?
> 
> New comment added:
> /*
>  * Contents of the 'magic' field are inspected to verify that they contain
>  * an expected value before the hypervisor will perform writes into this
>  * structure in guest-supplied memory.
>  */

But this does not help understand what this verification is good
for (or what it guards against). This again looks to be a reduction
of likelihood of misbehavior, instead of its exclusion.

As things accumulate: Personally I'd consider it better to wait
with posting a new version until discussions have settled. At
this point I'm already uncertain whether it'll be worthwhile for
me to thoroughly look at v2, when I'm likely to re-encounter
things I've already commented on in v1.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel