On 04/09/2016 12:14 PM, Maxim Nestratov wrote: > Below is backtraces of two deadlocked threads: > > thread #1: > virDomainConfVMNWFilterTeardown > virNWFilterTeardownFilter > lock updateMutex <------------ > _virNWFilterTeardownFilter > try to lock interface <---------- > > thread #2: > learnIPAddressThread > lock interface <------- > virNWFilterInstantiateFilterLate > try to lock updateMutex <---------- > > The problem is fixed by unlocking interface before calling > virNWFilterInstantiateFilterLate to avoid updateMutex and interface ordering > deadlocks. Otherwise we are going to instantiate the filter while holding > interface lock, which will try to lock updateMutex, and if some other thread > instantiating a filter in parallel is holding updateMutex and is trying to > lock interface, both will deadlock. > Also it is safe to unlock interface before virNWFilterInstantiateFilterLate > because learnIPAddressThread stopped capturing packets and applied necessary > rules on the interface, while instantiating a new filter doesn't require a > locked interface. > > Signed-off-by: Maxim Nestratov <mnestratov(a)virtuozzo.com&gt; > --- > src/nwfilter/nwfilter_learnipaddr.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/src/nwfilter/nwfilter_learnipaddr.c b/src/nwfilter/nwfilter_learnipaddr.c > index 1adbadb..cfd92d9 100644 > --- a/src/nwfilter/nwfilter_learnipaddr.c > +++ b/src/nwfilter/nwfilter_learnipaddr.c > @@ -611,6 +611,16 @@ learnIPAddressThread(void *arg) > sa.data.inet4.sin_addr.s_addr = vmaddr; > char *inetaddr; > > + /* It is necessary to unlock interface here to avoid updateMutex and > + * interface ordering deadlocks. Otherwise we are going to > + * instantiate the filter, which will try to lock updateMutex, and > + * some other thread instantiating a filter in parallel is holding > + * updateMutex and is trying to lock interface, both will deadlock. > + * Also it is safe to unlock interface here because we stopped > + * capturing and applied necessary rules on the interface, while > + * instantiating a new filter doesn't require a locked interface.*/ > + virNWFilterUnlockIface(req->ifname); > + > if ((inetaddr = virSocketAddrFormat(&sa)) != NULL) { > if (virNWFilterIPAddrMapAddIPAddr(req->ifname, inetaddr) < 0) { > VIR_ERROR(_("Failed to add IP address %s to IP address " > @@ -636,11 +646,11 @@ learnIPAddressThread(void *arg) > req->ifname, req->ifindex); > > techdriver->applyDropAllRules(req->ifname); > + virNWFilterUnlockIface(req->ifname); > } > > VIR_DEBUG("pcap thread terminating for interface %s", req->ifname); > > - virNWFilterUnlockIface(req->ifname); > > err_no_lock: > virNWFilterDeregisterLearnReq(req->ifindex); > This looks sensible to me... the virNWFilterInstantiateFilterLate call tree certainly expects the iface to be unlocked at a certain point. This patch just moves the unlock a bit earlier. ACK, but maybe wait another day to see if anyone else wants to jump in, I'm not really familiar with this code (then again I doubt anyone watching the list is deeply familiar with it either)

On 04/20/2016 12:06 PM, Laine Stump wrote: > On 04/19/2016 05:45 PM, Cole Robinson wrote: >> On 04/09/2016 12:14 PM, Maxim Nestratov wrote: >>> Below is backtraces of two deadlocked threads: >>> >>> thread #1: >>> virDomainConfVMNWFilterTeardown >>> virNWFilterTeardownFilter >>> lock updateMutex <------------ >>> _virNWFilterTeardownFilter >>> try to lock interface <---------- >>> >>> thread #2: >>> learnIPAddressThread >>> lock interface <------- >>> virNWFilterInstantiateFilterLate >>> try to lock updateMutex <---------- >>> >>> The problem is fixed by unlocking interface before calling >>> virNWFilterInstantiateFilterLate to avoid updateMutex and interface >>> ordering >>> deadlocks. Otherwise we are going to instantiate the filter while >>> holding >>> interface lock, which will try to lock updateMutex, and if some >>> other thread >>> instantiating a filter in parallel is holding updateMutex and is >>> trying to >>> lock interface, both will deadlock. >>> Also it is safe to unlock interface before >>> virNWFilterInstantiateFilterLate >>> because learnIPAddressThread stopped capturing packets and applied >>> necessary >>> rules on the interface, while instantiating a new filter doesn't >>> require a >>> locked interface. >>> >>> Signed-off-by: Maxim Nestratov <mnestratov(a)virtuozzo.com&gt; >>> --- >>> src/nwfilter/nwfilter_learnipaddr.c | 12 +++++++++++- >>> 1 file changed, 11 insertions(+), 1 deletion(-) >>> >>> diff --git a/src/nwfilter/nwfilter_learnipaddr.c >>> b/src/nwfilter/nwfilter_learnipaddr.c >>> index 1adbadb..cfd92d9 100644 >>> --- a/src/nwfilter/nwfilter_learnipaddr.c >>> +++ b/src/nwfilter/nwfilter_learnipaddr.c >>> @@ -611,6 +611,16 @@ learnIPAddressThread(void *arg) >>> sa.data.inet4.sin_addr.s_addr = vmaddr; >>> char *inetaddr; >>> + /* It is necessary to unlock interface here to avoid >>> updateMutex and >>> + * interface ordering deadlocks. Otherwise we are going to >>> + * instantiate the filter, which will try to lock >>> updateMutex, and >>> + * some other thread instantiating a filter in parallel is >>> holding >>> + * updateMutex and is trying to lock interface, both will >>> deadlock. >>> + * Also it is safe to unlock interface here because we >>> stopped >>> + * capturing and applied necessary rules on the interface, >>> while >>> + * instantiating a new filter doesn't require a locked >>> interface.*/ >>> + virNWFilterUnlockIface(req->ifname); >>> + >>> if ((inetaddr = virSocketAddrFormat(&sa)) != NULL) { >>> if (virNWFilterIPAddrMapAddIPAddr(req->ifname, >>> inetaddr) < 0) { >>> VIR_ERROR(_("Failed to add IP address %s to IP >>> address " >>> @@ -636,11 +646,11 @@ learnIPAddressThread(void *arg) >>> req->ifname, req->ifindex); >>> techdriver->applyDropAllRules(req->ifname); >>> + virNWFilterUnlockIface(req->ifname); >>> } >>> VIR_DEBUG("pcap thread terminating for interface %s", >>> req->ifname); >>> - virNWFilterUnlockIface(req->ifname); >>> err_no_lock: >>> virNWFilterDeregisterLearnReq(req->ifindex); >>> >> This looks sensible to me... the virNWFilterInstantiateFilterLate >> call tree >> certainly expects the iface to be unlocked at a certain point. This >> patch just >> moves the unlock a bit earlier. >> >> ACK, but maybe wait another day to see if anyone else wants to jump >> in, I'm >> not really familiar with this code (then again I doubt anyone >> watching the >> list is deeply familiar with it either) > > Stefan Berger is, but I don't think he watches the list closely these > days. I just found the original message (somehow it had been tossed > into my spam folder) and Replied-All to it with Stefan Cc'ed. ACK. I agree to the change. It seems to be possible to release the lock earlier. Stefan

On 05/24/2016 03:22 AM, Maxim Nestratov wrote: > 20.04.2016 22:04, Stefan Berger пишет: > >> On 04/20/2016 12:06 PM, Laine Stump wrote: >>> On 04/19/2016 05:45 PM, Cole Robinson wrote: >>>> On 04/09/2016 12:14 PM, Maxim Nestratov wrote: >>>>> Below is backtraces of two deadlocked threads: >>>>> >>>>> thread #1: >>>>> virDomainConfVMNWFilterTeardown >>>>> virNWFilterTeardownFilter >>>>> lock updateMutex <------------ >>>>> _virNWFilterTeardownFilter >>>>> try to lock interface <---------- >>>>> >>>>> thread #2: >>>>> learnIPAddressThread >>>>> lock interface <------- >>>>> virNWFilterInstantiateFilterLate >>>>> try to lock updateMutex <---------- >>>>> >>>>> The problem is fixed by unlocking interface before calling >>>>> virNWFilterInstantiateFilterLate to avoid updateMutex and >>>>> interface ordering >>>>> deadlocks. Otherwise we are going to instantiate the filter while >>>>> holding >>>>> interface lock, which will try to lock updateMutex, and if some >>>>> other thread >>>>> instantiating a filter in parallel is holding updateMutex and is >>>>> trying to >>>>> lock interface, both will deadlock. >>>>> Also it is safe to unlock interface before >>>>> virNWFilterInstantiateFilterLate >>>>> because learnIPAddressThread stopped capturing packets and >>>>> applied necessary >>>>> rules on the interface, while instantiating a new filter doesn't >>>>> require a >>>>> locked interface. >>>>> >>>>> Signed-off-by: Maxim Nestratov <mnestratov(a)virtuozzo.com&gt; >>>>> --- >>>>> src/nwfilter/nwfilter_learnipaddr.c | 12 +++++++++++- >>>>> 1 file changed, 11 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/src/nwfilter/nwfilter_learnipaddr.c >>>>> b/src/nwfilter/nwfilter_learnipaddr.c >>>>> index 1adbadb..cfd92d9 100644 >>>>> --- a/src/nwfilter/nwfilter_learnipaddr.c >>>>> +++ b/src/nwfilter/nwfilter_learnipaddr.c >>>>> @@ -611,6 +611,16 @@ learnIPAddressThread(void *arg) >>>>> sa.data.inet4.sin_addr.s_addr = vmaddr; >>>>> char *inetaddr; >>>>> + /* It is necessary to unlock interface here to avoid >>>>> updateMutex and >>>>> + * interface ordering deadlocks. Otherwise we are going to >>>>> + * instantiate the filter, which will try to lock >>>>> updateMutex, and >>>>> + * some other thread instantiating a filter in parallel >>>>> is holding >>>>> + * updateMutex and is trying to lock interface, both >>>>> will deadlock. >>>>> + * Also it is safe to unlock interface here because we >>>>> stopped >>>>> + * capturing and applied necessary rules on the >>>>> interface, while >>>>> + * instantiating a new filter doesn't require a locked >>>>> interface.*/ >>>>> + virNWFilterUnlockIface(req->ifname); >>>>> + >>>>> if ((inetaddr = virSocketAddrFormat(&sa)) != NULL) { >>>>> if (virNWFilterIPAddrMapAddIPAddr(req->ifname, >>>>> inetaddr) < 0) { >>>>> VIR_ERROR(_("Failed to add IP address %s to IP >>>>> address " >>>>> @@ -636,11 +646,11 @@ learnIPAddressThread(void *arg) >>>>> req->ifname, req->ifindex); >>>>> techdriver->applyDropAllRules(req->ifname); >>>>> + virNWFilterUnlockIface(req->ifname); >>>>> } >>>>> VIR_DEBUG("pcap thread terminating for interface %s", >>>>> req->ifname); >>>>> - virNWFilterUnlockIface(req->ifname); >>>>> err_no_lock: >>>>> virNWFilterDeregisterLearnReq(req->ifindex); >>>>> >>>> This looks sensible to me... the virNWFilterInstantiateFilterLate >>>> call tree >>>> certainly expects the iface to be unlocked at a certain point. >>>> This patch just >>>> moves the unlock a bit earlier. >>>> >>>> ACK, but maybe wait another day to see if anyone else wants to >>>> jump in, I'm >>>> not really familiar with this code (then again I doubt anyone >>>> watching the >>>> list is deeply familiar with it either) >>> >>> Stefan Berger is, but I don't think he watches the list closely >>> these days. I just found the original message (somehow it had been >>> tossed into my spam folder) and Replied-All to it with Stefan Cc'ed. >> >> ACK. I agree to the change. It seems to be possible to release the >> lock earlier. >> >> Stefan >> >> > > Shall I push it then? Yes. Stefan