[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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 ...

Zoli

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.