Re: RING_HAS_UNCONSUMED_REQUESTS oddness

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Zoltan Kiss
> Sent: 12 March 2014 16:14
> To: Wei Liu; Paul Durrant
> Cc: Ian Campbell; xen-devel@lists.xenproject.org; Tim (Xen.org)
> Subject: Re: [Xen-devel] RING_HAS_UNCONSUMED_REQUESTS oddness
> 
> On 12/03/14 15:42, Wei Liu wrote:
> > On Wed, Mar 12, 2014 at 03:23:09PM +0000, Paul Durrant wrote:
> > [...]
> >>>> Actually ancient memory tells me that, unfortunately, netback's
> backend-
> >>>> frontend GSO protocol is broken in this way... it requires one more
> >>> response slot than the number of requests it consumes (for the extra
> >>> metadata), which means that if the frontend keeps the ring full you can
> get
> >>> overflow. It's a bit of a tangent though, because that code doesn't use
> this
> >>> macro (or in fact check the ring has space in any way IIRC). The prefix
> variant
> >>> of the protocol is ok though.
> >>>
> >>> I think it's not: it consumes a request for the metadata, and when the
> >>> packet is grant copied to the guest, it creates a response for that slot
> >>> as well.
> >>
> >> As explained verbally, it doesn't consume a request for the 'extra' info.
> Let me elaborate here for the benefit of the list...
> >>
> >> In xenvif_gop_skb(), in the non-prefix GSO case, a single request is
> consumed for the header along with a meta slot which is used to hold the
> GSO data. Later on in xenvif_rx_action() the code calls make_rx_response()
> for the header, but then *before* moving onto the next meta slot it makes
> an 'extra' response for the GSO metadata. So - one meta slot - one request
> consumed, but two responses produced.
> >> So this mechanism totally relies on the netfront driver not completely
> filling the shared ring. If it ever does, you'll get overflow.
> >>
> >
> > (... which reminds me of the heisenbug Sander is seeing.)
> >
> > But do we not check for there's enough space in the ring before
> > procceeding?
> 
> Thinking further what Paul said, we might be actually OK. At the end of
> xenvif_gop_frag_copy there is this:
> 
> if (*head && ((1 << gso_type) & vif->gso_mask))
> 	vif->rx.req_cons++;
> 
> So although netback doesn't call RING_GET_REQUEST to consume the
> request, it does so by increasing req_cons. And netfront automagically
> detect that in xennet_get_extras, and calls xennet_move_rx_slot to move
> that buffer to req_prod_pvt. Same happens when the backend send a
> response that it couldn't use the slot and it doesn't contain any data.
> 

Still leaves the issue that the frontend has no idea which request id was consumed to create the 'hole' for the extra info, unless it has its own mapping of ring idx to id - i.e. the id in the xen_netif_rx_response struct is basically useless.

  Paul

> Zoli