From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cumulusnetworks.com; s=google; h=subject:to:references:cc:from:message-id:date:user-agent :mime-version:in-reply-to:content-type:content-transfer-encoding; bh=wkzzh0xwJriNzzy5KMC+txC8oKvSTQURJAN7PK6ZKD8=; b=R564/BniiSAbUGYt0uTa0oj4dibOGWypUPco1QPJBVsTtHKVMDZeSl8Uy4EmAjuME2 xgqJQ8LBB5CONrZvGvDZDnvDx56heWR9M29PCXTuQFLVlNFeHflGMYAEpsfFlB/sv+AP erPKDOyNYVlGQ8O6xRjmcRa3NwGVVcPzTGlP0= References: <1437667657-16954-1-git-send-email-nikolay@cumulusnetworks.com> <20150723095945.2b4dd59e@urahara> <55B11EF4.8050501@cumulusnetworks.com> <20150723101311.5e15b970@urahara> From: Nikolay Aleksandrov Message-ID: <55B12501.7070709@cumulusnetworks.com> Date: Thu, 23 Jul 2015 19:31:45 +0200 MIME-Version: 1.0 In-Reply-To: <20150723101311.5e15b970@urahara> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Bridge] [PATCH net] bridge: stp: when using userspace stp stop kernel hello and hold timers List-Id: Linux Ethernet Bridging List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stephen Hemminger Cc: netdev@vger.kernel.org, bridge@lists.linux-foundation.org, davem@davemloft.net On 07/23/2015 07:13 PM, Stephen Hemminger wrote: > On Thu, 23 Jul 2015 19:05:56 +0200 > Nikolay Aleksandrov wrote: > >> On 07/23/2015 06:59 PM, Stephen Hemminger wrote: >>> On Thu, 23 Jul 2015 09:07:37 -0700 >>> Nikolay Aleksandrov wrote: >>> >>>> + /* Stop hello and hold timer */ >>>> + spin_lock_bh(&br->lock); >>>> + del_timer(&br->hello_timer); >>>> + list_for_each_entry(p, &br->port_list, list) >>>> + del_timer(&p->hold_timer); >>>> + spin_unlock_bh(&br->lock); >>> >>> Wouldn't it be easier to use del_timer_sync here? >>> >> I think it should work. Also I have an error in the commit message >> about the kernel BPDU sending which I need to correct. I'll prepare >> a v2 with your suggestion and fixed commit message. > > The one thing to watch out for with del_timer_sync is that > the timer routine and the caller can't be using the same lock > otherwise timer will be spinning waiting to get lock that > is held by caller who is waiting for timer. > Actually I just noticed the locking was wrong in the patch, also we should use del_timer() only because a spin_lock is acquired before that and cannot use the blocking del_timer_sync() inside. I'll fix it all up in the second version. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nikolay Aleksandrov Subject: Re: [PATCH net] bridge: stp: when using userspace stp stop kernel hello and hold timers Date: Thu, 23 Jul 2015 19:31:45 +0200 Message-ID: <55B12501.7070709@cumulusnetworks.com> References: <1437667657-16954-1-git-send-email-nikolay@cumulusnetworks.com> <20150723095945.2b4dd59e@urahara> <55B11EF4.8050501@cumulusnetworks.com> <20150723101311.5e15b970@urahara> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org, bridge@lists.linux-foundation.org, davem@davemloft.net To: Stephen Hemminger Return-path: In-Reply-To: <20150723101311.5e15b970@urahara> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: bridge-bounces@lists.linux-foundation.org Errors-To: bridge-bounces@lists.linux-foundation.org List-Id: netdev.vger.kernel.org On 07/23/2015 07:13 PM, Stephen Hemminger wrote: > On Thu, 23 Jul 2015 19:05:56 +0200 > Nikolay Aleksandrov wrote: > >> On 07/23/2015 06:59 PM, Stephen Hemminger wrote: >>> On Thu, 23 Jul 2015 09:07:37 -0700 >>> Nikolay Aleksandrov wrote: >>> >>>> + /* Stop hello and hold timer */ >>>> + spin_lock_bh(&br->lock); >>>> + del_timer(&br->hello_timer); >>>> + list_for_each_entry(p, &br->port_list, list) >>>> + del_timer(&p->hold_timer); >>>> + spin_unlock_bh(&br->lock); >>> >>> Wouldn't it be easier to use del_timer_sync here? >>> >> I think it should work. Also I have an error in the commit message >> about the kernel BPDU sending which I need to correct. I'll prepare >> a v2 with your suggestion and fixed commit message. > > The one thing to watch out for with del_timer_sync is that > the timer routine and the caller can't be using the same lock > otherwise timer will be spinning waiting to get lock that > is held by caller who is waiting for timer. > Actually I just noticed the locking was wrong in the patch, also we should use del_timer() only because a spin_lock is acquired before that and cannot use the blocking del_timer_sync() inside. I'll fix it all up in the second version.