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

[liu ping fan]

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.

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