On Wed, 8 Mar 2017, Boris Ostrovsky wrote:
> On 03/08/2017 02:33 PM, Stefano Stabellini wrote:
> > On Wed, 8 Mar 2017, Boris Ostrovsky wrote:
> >>>>> +}
> >>>>> +
> >>>>> +static int p9_xen_write_todo(struct xen_9pfs_dataring *ring, RING_IDX
> >>>>> size)
> >>>>> +{
> >>>>> + RING_IDX cons, prod;
> >>>>> +
> >>>>> + cons = ring->intf->out_cons;
> >>>>> + prod = ring->intf->out_prod;
> >>>>> + mb();
> >>>>> +
> >>>>> + if (XEN_9PFS_RING_SIZE - xen_9pfs_queued(prod, cons,
> >>>>> XEN_9PFS_RING_SIZE) >= size)
> >>>>> + return 1;
> >>>>> + else
> >>>>> + return 0;
> >>>>> }
> >>>>>
> >>>>> static int p9_xen_request(struct p9_client *client, struct p9_req_t
> >>>>> *p9_req)
> >>>>> {
> >>>>> + struct xen_9pfs_front_priv *priv = NULL;
> >>>>> + RING_IDX cons, prod, masked_cons, masked_prod;
> >>>>> + unsigned long flags;
> >>>>> + uint32_t size = p9_req->tc->size;
> >>>>> + struct xen_9pfs_dataring *ring;
> >>>>> + int num;
> >>>>> +
> >>>>> + list_for_each_entry(priv, &xen_9pfs_devs, list) {
> >>>>> + if (priv->client == client)
> >>>>> + break;
> >>>>> + }
> >>>>> + if (priv == NULL || priv->client != client)
> >>>>> + return -EINVAL;
> >>>>> +
> >>>>> + num = p9_req->tc->tag % priv->num_rings;
> >>>>> + ring = &priv->rings[num];
> >>>>> +
> >>>>> +again:
> >>>>> + while (wait_event_interruptible(ring->wq,
> >>>>> + p9_xen_write_todo(ring, size) > 0) !=
> >>>>> 0);
> >>>>> +
> >>>>> + spin_lock_irqsave(&ring->lock, flags);
> >>>>> + cons = ring->intf->out_cons;
> >>>>> + prod = ring->intf->out_prod;
> >>>>> + mb();
> >>>>> +
> >>>>> + if (XEN_9PFS_RING_SIZE - xen_9pfs_queued(prod, cons,
> >>>>> XEN_9PFS_RING_SIZE) < size) {
> >>>> This looks like p9_xen_write_todo().
> >>> p9_xen_write_todo is just a wrapper around xen_9pfs_queued to provide
> >>> a return value that works well with wait_event_interruptible.
> >>>
> >>> I would prefer not to call p9_xen_write_todo here, because it's simpler
> >>> if we don't read prod and cons twice.
> >> I was referring to the whole code fragment after spin_lock_irqsave(),
> >> not just the last line. Isn't it exactly !p9_xen_write_todo()?
> > Yes, it is true they are almost the same. The difference, and the reason
> > for p9_xen_write_todo to exist, is that p9_xen_write_todo is called in
> > the wait_event_interruptible loop, as such it needs to read prod and
> > cons every time. On the other end, here we want to read them once. Does
> > it make sense?
>
>
> I am clearly being particularly dense here but what I was thinking was:
>
> again:
> while (wait_event_interruptible(ring->wq,
> p9_xen_write_todo(ring, size) > 0) != 0);
>
> spin_lock_irqsave(&ring->lock, flags);
> if (!p9_xen_write_todo(ring, size)) {
> spin_unlock_irqrestore(&ring->lock, flags);
> goto again;
> }
>
> There is no extra read of prod/cons.
Yes, there are: just after this if statement we would have to read them
again to calculate masked_prod and masked_cons.
