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

Re: [Xen-devel] [PATCH net-next v7 4/9] xen-netback: Introduce TX grant mapping



On 13/03/14 13:56, Ian Campbell wrote:
On Thu, 2014-03-13 at 13:17 +0000, Zoltan Kiss wrote:
On 13/03/14 10:33, Ian Campbell wrote:
On Thu, 2014-03-06 at 21:48 +0000, Zoltan Kiss wrote:

+                               netdev_err(vif->dev,
+                                          "Page still granted! Index: %x\n",
+                                          i);
+                       i = -1;

Should there not be a break here? Otherwise don't we restart the for
loop from 0 again? If that is intentional then a comment would be very
useful.
Yes, that's intentional, we shouldn't exit this loop until everything is
unmapped. An i-- would be fine as well. I will put a comment there.

Yes please do, it's very non-obvious what is going on. I'm almost
inclined to suggest that this is one of the few places where a goto
retry might be appropriate.

Can you also add a comment saying what is doing the actual unmap work
which we are waiting for here since it is not actually part of the loop.
Might a barrier be needed to ensure we see that work happening?
I don't think a barrier is necessary here, if this function ran into !NETBACK_INVALID_HANDLE, it just starts again the checking.

On 13/03/14 13:17, Zoltan Kiss wrote:>>
>> [...]
>>> +    /* Btw. already unmapped? */
>>
>> What does this comment mean? Is it a fixme? An indicator that
>> xenvif_grant_handle_reset is supposed to handle this case or something
>> else?
> It comes from the time when xenvif_grant_handle_reset was not a
> standalone function. Yes, it refers to the check in the beginning of
> that function, and it should go there.

I ended up removing that comment, the error message in the function tells the same.

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