Re: [Qemu-devel] [RFC PATCH v4 14/15] slirp: handle race condition

[liu ping fan]

On Mon, Apr 22, 2013 at 1:55 PM, liu ping fan <address@hidden> wrote:
> On Fri, Apr 19, 2013 at 4:21 PM, Jan Kiszka <address@hidden> wrote:
>> On 2013-04-19 02:18, liu ping fan wrote:
>>> On Thu, Apr 18, 2013 at 3:13 PM, Jan Kiszka <address@hidden> wrote:
>>>> On 2013-04-17 10:39, Liu Ping Fan wrote:
>>>>> From: Liu Ping Fan <address@hidden>
>>>>>
>>>>> Slirp and its peer can run on different context at the same time.
>>>>> Using lock to protect
>>>>
>>>> What are the usage rules for this lock, what precisely is it protecting?
>>>> Is it ensured that we do not take the BQL while holding this one?
>>>>
>>> It protect the slirp state, since slirp can be touched by slrip_input
>>> called by frontend(ex, e1000), also it can be touched by its event
>>> handler.  With this lock, we do not need BQL
>>
>> ...but the BQL will, at least initially, remain to be everywhere. Every
>> non-converted device model will hold it while calling into Slirp. So we
>> have the ordering "BQL before Slirp lock" already. And we must ensure
>> that there is no "BQL after Slirp lock". Can you guarantee this?
>>
> Oh, yes, there is a potential ABBA lock problem. Especially for slirp
> backend, the NetClientState's receive() can be nested, the scene:
> e1000 send packet -> net_slirp_receive() ...->arp_input()..->
> ->slirp_output() ->e1000_receive()
> Although currently, there is no extra lock required by
> e1000_receive(), but the nested model does cause the potential of ABBA
> lock problem, when beginning to covert the frontend(e1000).
> What about introducing  slirp->lockstate  and slirp->lock only used to
> protect it. So the users of slirp will protect agaist each other by
> slirp->lockstate, meanwhile, we can advoid the potential dead lock.
>
Drop the bad idea. The slirp->lock should be dropped before calling
out direction method.  And if_start() is need to fix

> Regards,
> Pingfan
>> Jan
>>
>> --
>> Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
>> Corporate Competence Center Embedded Linux