Re: [Xen-devel] RING_HAS_UNCONSUMED_REQUESTS oddness

[Zoltan Kiss <zoltan.kiss@xxxxxxxxxx>]

On 11/03/14 15:55, Ian Campbell wrote:
> On Thu, 2014-03-06 at 21:39 +0000, Zoltan Kiss wrote:
> > On 06/03/14 17:30, Tim Deegan wrote:
> > > At 16:31 +0000 on 06 Mar (1394119880), Zoltan Kiss wrote:
> > > > On 06/03/14 15:53, Ian Campbell wrote:
> > > > > On Thu, 2014-03-06 at 15:47 +0000, Zoltan Kiss wrote:
> > > > > > By my understanding, there is no way rsp could be smaller than req, so
> > > > > > there is no point having this. Am I missing something?
> > > > > It happens during wraparound, i.e. after req has wrapped but rsp hasn't
> > > > > yet.
> > > > The name of the macro suggest we are interested whether the ring has
> > > > unconsumed requests, and netback uses it that way. The answer to that
> > > > question is req_prod - req_cons. And it works if prod wrapped but cons
> > > > didn't.
> > > Yes.
> > >
> > > > rsp calculates the number of "consumed but not responded" requests (it
> > > > also works well if req_cons wrapped but rsp_prod_pvt didn't), then
> > > > subtract it from the ring size.
> > > That is indeed an odd thing to check, since it seems like it could only
> > > be relevant if the request producer overran the response producer.
> > > It's been there in one form or another since the original ring.h,
> > > and RING_REQUEST_CONS_OVERFLOW does something similar.
> > >
> > > I can't remember the original reasoning, and so I'm reluctant to
> > > suggest removing it without some more eyes on the code...
> > I've added the following printk before the "req < rsp" part:
> >
> >         if (rsp < req)                                                       \
> >                 pr_err("req %u rsp %u req_prod %u req_cons %u rsp_prod_pvt 
> > %u\n", req,
> > rsp, (_r)->sring->req_prod, (_r)->req_cons, (_r)->rsp_prod_pvt); \
> >
> > And it gave me such results:
> >
> > xen_netback:xenvif_zerocopy_callback: req 4294967279 rsp 52 req_prod
> > 1770663942 req_cons 1770663959 rsp_prod_pvt 1770663755
> >
> > So it can happen that req_prod is behind req_cons, sometimes even with
> > 17! But it always happen in this callback of my new grant mapping
> > series, which runs outside the NAPI instance. My theory why this can happen:
> > - callback reads req_prod
> > - frontend writes it
> > - backend picks it up, and consumes those slots
> > - callback reads req_cons
> I'm a bit confused by what you mean by "it" in your theory, so perhaps I
> misunderstand. Can you use the actual variable names for clarity.
I meant req_prod all the time.

> Are you sure this is a problem with the actual code and not just with
> your debug print? I would expect the real code to be snapshotting things
> as appropriate etc, and also for the public/private state to not
> necessarily be totally in sync when RING_HAS_UNCONSUMED_REQUESTS is
> being called.
I think my above code does the right thing. For double check I printed 
out req, rsp, and the values they are calculated from. I guess req_prod 
is cached in the printk and that's why it is still the same value as we 
read when calculating req. I'll test that with a memory barrier.
> > So req can be near UINT_MAX if you call this macro outside the backend.
> > The only place where the actual return value of this macro matters is
> > xenvif_tx_build_gops, and it should be correct there. At other places we
> > are only looking for the fact whether the ring has unconsumed requests
> > or not. If prod is smaller than cons, we clearly read a wrong value. I
> > think what we can do:
> > 1. try again until its correct
> > 2. just return a non-zero value, it shouldn't cause too much trouble if
> > we say yes here
> > 3. we can't see rsp_cons, so try to figure out if the ring is full of
> > consumed but not responded requests, and return zero then, otherwise a
> > positive value. That's what we do know.
> s/know/now/? Is #3 here the status quo?
Yes, that's the status quo. It's a best effort calculation, rsp takes 
only effect if req ~ UINT_MAX.
> > Does this make sense? Should we rather go option 1? Should I post a
> > comment patch to document this, and spare a few hours for future
> > generations? :)
> Docs are always good IMHO.
>
> Ian.

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