Re: RING_HAS_UNCONSUMED_REQUESTS oddness

[Ian Campbell <Ian.Campbell@citrix.com>]

On Wed, 2014-03-12 at 21:10 +0000, Zoltan Kiss wrote:
> On 12/03/14 17:43, Ian Campbell wrote:
> > On Wed, 2014-03-12 at 17:14 +0000, Zoltan Kiss wrote:
> >> On 12/03/14 15:37, Ian Campbell wrote:
> >>> On Wed, 2014-03-12 at 15:14 +0000, Zoltan Kiss wrote:
> >>>> On 12/03/14 14:30, Ian Campbell wrote:
> >>>>> On Wed, 2014-03-12 at 14:27 +0000, Zoltan Kiss wrote:
> >>>>>> On 12/03/14 10:28, Ian Campbell wrote:
> >>>>>>> On Tue, 2014-03-11 at 23:24 +0000, Zoltan Kiss wrote:
> >>>>>>>> On 11/03/14 15:44, Ian Campbell wrote:
> >>>>>>>
> >>>>>>>>> Is it the case that this macro considers a request to be unconsumed if
> >>>>>>>>> the *response* to a request is outstanding as well as if the request
> >>>>>>>>> itself is still on the ring?
> >>>>>>>> I don't think that would make sense. I think everywhere where this macro
> >>>>>>>> is called the caller is not interested in pending request (pending means
> >>>>>>>> consumed but not responded)
> >>>>>>>
> >>>>>>> It might be interested in such pending requests in some of the
> >>>>>>> pathological cases I allude to in the next paragraph though?
> >>>>>>>
> >>>>>>> For example if the ring has unconsumed requests but there are no slots
> >>>>>>> free for a response, it would be better to treat it as no unconsumed
> >>>>>>> requests until space opens up for a response, otherwise something else
> >>>>>>> just has to abort the processing of the request when it notices the lack
> >>>>>>> of space.
> >>>>>>>
> >>>>>>> (I'm totally speculating here BTW, I don't have any concrete idea why
> >>>>>>> things are done this way...)
> >>>>>>>
> >>>>>>>
> >>>>>>>>> I wonder if this apparently weird construction is due to pathological
> >>>>>>>>> cases when one or the other end is not picking up requests/responses?
> >>>>>>>>> i.e. trying to avoid deadlocking the ring or generating an interrupt
> >>>>>>>>> storm when the ring it is full of one or the other or something along
> >>>>>>>>> those lines?
> >>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>> Also, let me quote again my example about when rsp makes sense:
> >>>>>>
> >>>>>> "To clarify what does this do, let me show an example:
> >>>>>> req_prod = 253
> >>>>>> req_cons = 256
> >>>>>> rsp_prod_pvt = 0
> >>>>>
> >>>>> I think to make sense of this I need to see the sequence of reads/writes
> >>>>> from both parties in a sensible ordering which would result in reads
> >>>>> showing the above. i.e. a demonstration of the race not just an
> >>>>> assertion that if the values are read as is things makes sense.
> >>>>
> >>>> Let me extend it:
> >>>>
> >>>> - callback reads req_prod = 253
> >>>
> >>> callback == backend? Which context is this code running in? Which part
> >>> of the system is the callback logically part of?
> >> Yes, it is part of the backend, the function which handles when we can
> >> release a slot back. With grant copy we don't have such thing, but with
> >> mapping xenvif_zerocopy_callback does this (or in classic kernel, it had
> >> a different name, but we called it page destructor). It can run from any
> >> context, it depends on who calls kfree_skb.
> >
> > I think this is the root of the problem. The pv protocols really assume
> > one entity on either end is moving/updating the ring pointers. If you
> There is only _one_ entity moving/updating the ring pointers. Everyone 
> else is just reading it. The callback, xenvif_tx_interrupt, 
> xenvif_check_rx_xenvif.

Perhaps I should have said "accessing" rather than "moving/updating".
Even if only one entity is reading the ring if two entities are reading
at least one of them is going to see inconsistencies.

> > In any case, it seems like doing the poke from the callback is wrong and
> > we should revert the patches which DaveM already applied and revisit
> > this aspect of things, do you agree?
> I've just sent in a patch to fix that.

Thanks, I'll take a look.

>  I think the reason why I haven't 
> seen any issue is that the in this situation there are plenty of 
> outstanding packets, and all of their callback will schedule NAPI again. 
> Chances are quite small that the dealloc thread couldn't release enough 
> slots in the meantime.

Seems plausible enough. That means that the issue would be seen rarely
on quiet systems, which would be a nightmare to diagnose I think.

Ian.