Re: [Xen-devel] [PATCH] Kernel OOPS in xen_netbk_rx_action / xenvif_gop_skb

[Philipp Hahn <hahn@xxxxxxxxxxxxx>]

Hello Wei Liu,

On 11.07.2014 12:32, Wei Liu wrote:
> On Fri, Jul 11, 2014 at 11:41:22AM +0200, Philipp Hahn wrote:
>> On 10.07.2014 14:41, Wei Liu wrote:
>>> On Wed, Jul 02, 2014 at 09:45:44AM +0200, Philipp Hahn wrote:
>>>> @Wei Liu: You said that the patch is only a quick hack to 
>>>> detect, if my analysis is correct and a proper fix would be 
>>>> needed. For us the attached patch works, as the problem does 
>>>> not happen that often and is hard to reproduce anyway, so 
>>>> spending more time on that issue is probably not worth it. And 
>>>> that flag doesn't look that ugly.
>> ...
>>> I agree that we would like to avoid spending too much time on 
>>> this issue.
>> 
>> This is also what I'm thinking.
>> 
>>> Since the problem is confirmed, I think a proper fix will be to 
>>> reference count vif and prevent it from unmapping the ring
>>> before all queued SKBs are consumed.
>> 
>> vif is already ref-counted; see attached *untested* patch for a 
>> start.

v2 = using refcnt

>> The quick hack now runs on that system for several weeks now 
>> without problems.

v1 = Your quick hack patch + my additional dev_kfree_skb(skb);

> I'm a bit confused, you said that patch was *untested*, then you said
> the hack runs fine.
> 
> I guess what you meant is: the approach is generally working but the 
> patch you submitted is not the one running in your system? Is there 
> significant difference between the patch you submitted and the one 
> you're using in production?

The PATCH form 02.07.2014 09:45 using your "vif->mapped" is running fine
on the system for ~2 weeks now.

The PATCHv2 using refcnt I posted today is untested and for RFC.


On 11.07.2014 11:53, Wei Liu wrote:
>> What I don't like is xenbus_unmap_ring_vfree() potentially
>> printing an error on double-free when called from 
>> xen_netbk_unmap_frontend_rings().
> 
> How can it be called twice? Does it happen before or after applying 
> my patch?

I've not seen it called twice, but while studying the code I asked
myself if it's safe to be called twice. I've not gone down to road to
proof that it never is. Just being cautious.

Worst thing which could happen is an error message:
|       xenbus_dev_error(dev, -ENOENT,
|                        "can't find mapped virtual address %p", vaddr);

> I will need to think further if this hack is really a proper 
> solution. I will keep you posted if there's any update on this.

Thank you for your feedback and support.

> In the mean time you can carry that patch in your own tree for a 
> while.

We will keep using v1 for now and I will keep you updated on this as well.

Sincerely
Philipp

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