Re: [Xen-devel] RING_HAS_UNCONSUMED_REQUESTS oddness

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Zoltan Kiss
> Sent: 12 March 2014 19:06
> To: Paul Durrant; Wei Liu
> Cc: Ian Campbell; xen-devel@xxxxxxxxxxxxxxxxxxxx; Tim (Xen.org)
> Subject: Re: [Xen-devel] RING_HAS_UNCONSUMED_REQUESTS oddness
> 
> On 12/03/14 16:42, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Zoltan Kiss
> >> Sent: 12 March 2014 16:14
> >> To: Wei Liu; Paul Durrant
> >> Cc: Ian Campbell; xen-devel@xxxxxxxxxxxxxxxxxxxx; 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.
> 
> It only works because netback puts the responses here to the place of
> the original requests. If you check, frontend doesn't even check what's
> the id in the response, it only use it in an error printout. Seems like
> it is a bit fragile ...
> 

... and completely non-obvious. Perhaps we should just bin the id field from 
the response struct in the canonical headers to avoid new frontends believing 
that it is useful, and document that they should use their own mapping of idx 
to id, and guarantee that netback will always respond in order on the rx ring.

  Paul

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