Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1030321AbVJ1Rgv (ORCPT ); Fri, 28 Oct 2005 13:36:51 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1030336AbVJ1Rgv (ORCPT ); Fri, 28 Oct 2005 13:36:51 -0400 Received: from e35.co.us.ibm.com ([32.97.110.153]:33697 "EHLO e35.co.us.ibm.com") by vger.kernel.org with ESMTP id S1030321AbVJ1Rgu (ORCPT ); Fri, 28 Oct 2005 13:36:50 -0400 Date: Fri, 28 Oct 2005 10:37:16 -0700 From: "Paul E. McKenney" To: Corey Minyard Cc: linux-kernel@vger.kernel.org, akpm@osdl.org Subject: Re: [PATCH] ipmi: use rcu lock for using command receivers Message-ID: <20051028173716.GG25375@us.ibm.com> Reply-To: paulmck@us.ibm.com References: <20051028131944.GA10285@i2.minyard.local> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20051028131944.GA10285@i2.minyard.local> User-Agent: Mutt/1.4.1i Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5518 Lines: 160 On Fri, Oct 28, 2005 at 08:19:44AM -0500, Corey Minyard wrote: > I finally got some time to think this through and test it. > This patch is in addition to the ipmi-use-refcount-in-message-handler > patch. > Thanks again, Paul, for seeing this. > > Use rcu_read_lock for the cmd_rcvrs list, since that was what > what intended, anyway. This means that all the users of the > cmd_rcvrs_lock are tasks, so the irq disables are no longer > required for that lock and it can become a semaphore. Looks good from an RCU viewpoint! I presume that find_cmd_rcvr() still includes an rcu_dereference(). Thanx, Paul Acked-by: > Signed-off-by: Corey Minyard > > Index: linux-2.6.14-rc4/drivers/char/ipmi/ipmi_msghandler.c > =================================================================== > --- linux-2.6.14-rc4.orig/drivers/char/ipmi/ipmi_msghandler.c > +++ linux-2.6.14-rc4/drivers/char/ipmi/ipmi_msghandler.c > @@ -209,7 +209,7 @@ struct ipmi_smi > > /* The list of command receivers that are registered for commands > on this interface. */ > - spinlock_t cmd_rcvrs_lock; > + struct semaphore cmd_rcvrs_lock; > struct list_head cmd_rcvrs; > > /* Events that were queues because no one was there to receive > @@ -345,7 +345,6 @@ static void clean_up_interface_data(ipmi > { > int i; > struct cmd_rcvr *rcvr, *rcvr2; > - unsigned long flags; > struct list_head list; > > free_recv_msg_list(&intf->waiting_msgs); > @@ -353,10 +352,10 @@ static void clean_up_interface_data(ipmi > > /* Wholesale remove all the entries from the list in the > * interface and wait for RCU to know that none are in use. */ > - spin_lock_irqsave(&intf->cmd_rcvrs_lock, flags); > + down(&intf->cmd_rcvrs_lock); > list_add_rcu(&list, &intf->cmd_rcvrs); > list_del_rcu(&intf->cmd_rcvrs); > - spin_unlock_irqrestore(&intf->cmd_rcvrs_lock, flags); > + up(&intf->cmd_rcvrs_lock); > synchronize_rcu(); > > list_for_each_entry_safe(rcvr, rcvr2, &list, link) > @@ -812,7 +811,7 @@ int ipmi_destroy_user(ipmi_user_t user) > * since other things may be using it till we do > * synchronize_rcu()) then free everything in that list. > */ > - spin_lock_irqsave(&intf->cmd_rcvrs_lock, flags); > + down(&intf->cmd_rcvrs_lock); > list_for_each_safe_rcu(entry1, entry2, &intf->cmd_rcvrs) { > rcvr = list_entry(entry1, struct cmd_rcvr, link); > if (rcvr->user == user) { > @@ -821,7 +820,7 @@ int ipmi_destroy_user(ipmi_user_t user) > rcvrs = rcvr; > } > } > - spin_unlock_irqrestore(&intf->cmd_rcvrs_lock, flags); > + up(&intf->cmd_rcvrs_lock); > synchronize_rcu(); > while (rcvrs) { > rcvr = rcvrs; > @@ -950,7 +949,7 @@ int ipmi_register_for_cmd(ipmi_user_t > rcvr->netfn = netfn; > rcvr->user = user; > > - spin_lock_irq(&intf->cmd_rcvrs_lock); > + down(&intf->cmd_rcvrs_lock); > /* Make sure the command/netfn is not already registered. */ > entry = find_cmd_rcvr(intf, netfn, cmd); > if (entry) { > @@ -961,7 +960,7 @@ int ipmi_register_for_cmd(ipmi_user_t > list_add_rcu(&rcvr->link, &intf->cmd_rcvrs); > > out_unlock: > - spin_unlock_irq(&intf->cmd_rcvrs_lock); > + up(&intf->cmd_rcvrs_lock); > if (rv) > kfree(rcvr); > > @@ -975,17 +974,17 @@ int ipmi_unregister_for_cmd(ipmi_user_t > ipmi_smi_t intf = user->intf; > struct cmd_rcvr *rcvr; > > - spin_lock_irq(&intf->cmd_rcvrs_lock); > + down(&intf->cmd_rcvrs_lock); > /* Make sure the command/netfn is not already registered. */ > rcvr = find_cmd_rcvr(intf, netfn, cmd); > if ((rcvr) && (rcvr->user == user)) { > list_del_rcu(&rcvr->link); > - spin_unlock_irq(&intf->cmd_rcvrs_lock); > + up(&intf->cmd_rcvrs_lock); > synchronize_rcu(); > kfree(rcvr); > return 0; > } else { > - spin_unlock_irq(&intf->cmd_rcvrs_lock); > + up(&intf->cmd_rcvrs_lock); > return -ENOENT; > } > } > @@ -1858,7 +1857,7 @@ int ipmi_register_smi(struct ipmi_smi_ha > spin_lock_init(&intf->events_lock); > INIT_LIST_HEAD(&intf->waiting_events); > intf->waiting_events_count = 0; > - spin_lock_init(&intf->cmd_rcvrs_lock); > + init_MUTEX(&intf->cmd_rcvrs_lock); > INIT_LIST_HEAD(&intf->cmd_rcvrs); > init_waitqueue_head(&intf->waitq); > > @@ -2058,14 +2057,14 @@ static int handle_ipmb_get_msg_cmd(ipmi_ > netfn = msg->rsp[4] >> 2; > cmd = msg->rsp[8]; > > - spin_lock_irqsave(&intf->cmd_rcvrs_lock, flags); > + rcu_read_lock(); > rcvr = find_cmd_rcvr(intf, netfn, cmd); > if (rcvr) { > user = rcvr->user; > kref_get(&user->refcount); > } else > user = NULL; > - spin_unlock_irqrestore(&intf->cmd_rcvrs_lock, flags); > + rcu_read_unlock(); > > if (user == NULL) { > /* We didn't find a user, deliver an error response. */ > @@ -2238,14 +2237,14 @@ static int handle_lan_get_msg_cmd(ipmi_s > netfn = msg->rsp[6] >> 2; > cmd = msg->rsp[10]; > > - spin_lock_irqsave(&intf->cmd_rcvrs_lock, flags); > + rcu_read_lock(); > rcvr = find_cmd_rcvr(intf, netfn, cmd); > if (rcvr) { > user = rcvr->user; > kref_get(&user->refcount); > } else > user = NULL; > - spin_unlock_irqrestore(&intf->cmd_rcvrs_lock, flags); > + rcu_read_unlock(); > > if (user == NULL) { > /* We didn't find a user, just give up. */ > - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/