On Thu, Dec 20, 2018 at 06:46:32PM -0600, Corey Minyard wrote:
> On 12/20/18 5:59 PM, Paul E. McKenney wrote:
> >On Thu, Dec 20, 2018 at 05:06:19PM -0600, [email protected] wrote:
> >>From: Corey Minyard <[email protected]>
> >>
> >>The IPMI driver was recently modified to use SRCU, but it turns out
> >>this uses a chunk of percpu memory, even if IPMI is never used.
> >>
> >>So modify thing to on initialize on the first use. There was already
> >>code to sort of handle this for handling init races, so piggy back
> >>on top of that, and simplify it in the process.
> >>
> >>Signed-off-by: Corey Minyard <[email protected]>
> >>Reported-by: Tejun Heo <[email protected]>
> >>Cc: Paul E. McKenney <[email protected]>
> >The initialization portion looks good, but don't you need a
> >cleanup_srcu_struct() somewhere in cleanup_ipmi()?
> >
> >Or is it not possible to repeatedly load and unload this as a module?
>
> Thanks for the review. I really meant to put that there :).
Been there, done that! ;-)
Thanx, Paul
> -corey
>
> > Thanx, Paul
> >
> >>---
> >> drivers/char/ipmi/ipmi_msghandler.c | 133 +++++++++++++++-------------
> >> 1 file changed, 70 insertions(+), 63 deletions(-)
> >>
> >>Tejun, here's a patch to fix the issue. Does this look ok?
> >>
> >>Maybe it would be best to not use SRCU, but IMHO it's easier to use
> >>than locking. That's why I used it.
> >>
> >>diff --git a/drivers/char/ipmi/ipmi_msghandler.c
> >>b/drivers/char/ipmi/ipmi_msghandler.c
> >>index 9650fe109f67..7490b9ae8508 100644
> >>--- a/drivers/char/ipmi/ipmi_msghandler.c
> >>+++ b/drivers/char/ipmi/ipmi_msghandler.c
> >>@@ -62,7 +62,8 @@ static void ipmi_debug_msg(const char *title, unsigned
> >>char *data,
> >> { }
> >> #endif
> >>
> >>-static int initialized;
> >>+static bool initialized;
> >>+static bool drvregistered;
> >>
> >> enum ipmi_panic_event_op {
> >> IPMI_SEND_PANIC_EVENT_NONE,
> >>@@ -630,7 +631,7 @@ static DEFINE_MUTEX(ipmidriver_mutex);
> >>
> >> static LIST_HEAD(ipmi_interfaces);
> >> static DEFINE_MUTEX(ipmi_interfaces_mutex);
> >>-DEFINE_STATIC_SRCU(ipmi_interfaces_srcu);
> >>+struct srcu_struct ipmi_interfaces_srcu;
> >>
> >> /*
> >> * List of watchers that want to know when smi's are added and deleted.
> >>@@ -1154,7 +1155,7 @@ int ipmi_create_user(unsigned int if_num,
> >> {
> >> unsigned long flags;
> >> struct ipmi_user *new_user;
> >>- int rv = 0, index;
> >>+ int rv, index;
> >> struct ipmi_smi *intf;
> >>
> >> /*
> >>@@ -1172,18 +1173,9 @@ int ipmi_create_user(unsigned int if_num,
> >> * Make sure the driver is actually initialized, this handles
> >> * problems with initialization order.
> >> */
> >>- if (!initialized) {
> >>- rv = ipmi_init_msghandler();
> >>- if (rv)
> >>- return rv;
> >>-
> >>- /*
> >>- * The init code doesn't return an error if it was turned
> >>- * off, but it won't initialize. Check that.
> >>- */
> >>- if (!initialized)
> >>- return -ENODEV;
> >>- }
> >>+ rv = ipmi_init_msghandler();
> >>+ if (rv)
> >>+ return rv;
> >>
> >> new_user = kmalloc(sizeof(*new_user), GFP_KERNEL);
> >> if (!new_user)
> >>@@ -3364,17 +3356,9 @@ int ipmi_register_smi(const struct ipmi_smi_handlers
> >>*handlers,
> >> * Make sure the driver is actually initialized, this handles
> >> * problems with initialization order.
> >> */
> >>- if (!initialized) {
> >>- rv = ipmi_init_msghandler();
> >>- if (rv)
> >>- return rv;
> >>- /*
> >>- * The init code doesn't return an error if it was turned
> >>- * off, but it won't initialize. Check that.
> >>- */
> >>- if (!initialized)
> >>- return -ENODEV;
> >>- }
> >>+ rv = ipmi_init_msghandler();
> >>+ if (rv)
> >>+ return rv;
> >>
> >> intf = kzalloc(sizeof(*intf), GFP_KERNEL);
> >> if (!intf)
> >>@@ -5084,6 +5068,22 @@ static int panic_event(struct notifier_block *this,
> >> return NOTIFY_DONE;
> >> }
> >>
> >>+/* Must be called with ipmi_interfaces_mutex held. */
> >>+static int ipmi_register_driver(void)
> >>+{
> >>+ int rv;
> >>+
> >>+ if (drvregistered)
> >>+ return 0;
> >>+
> >>+ rv = driver_register(&ipmidriver.driver);
> >>+ if (rv)
> >>+ pr_err("Could not register IPMI driver\n");
> >>+ else
> >>+ drvregistered = true;
> >>+ return rv;
> >>+}
> >>+
> >> static struct notifier_block panic_block = {
> >> .notifier_call = panic_event,
> >> .next = NULL,
> >>@@ -5094,66 +5094,73 @@ static int ipmi_init_msghandler(void)
> >> {
> >> int rv;
> >>
> >>+ mutex_lock(&ipmi_interfaces_mutex);
> >>+ rv = ipmi_register_driver();
> >>+ if (rv)
> >>+ goto out;
> >> if (initialized)
> >>- return 0;
> >>-
> >>- rv = driver_register(&ipmidriver.driver);
> >>- if (rv) {
> >>- pr_err("Could not register IPMI driver\n");
> >>- return rv;
> >>- }
> >>+ goto out;
> >>
> >>- pr_info("version " IPMI_DRIVER_VERSION "\n");
> >>+ init_srcu_struct(&ipmi_interfaces_srcu);
> >>
> >> timer_setup(&ipmi_timer, ipmi_timeout, 0);
> >> mod_timer(&ipmi_timer, jiffies + IPMI_TIMEOUT_JIFFIES);
> >>
> >> atomic_notifier_chain_register(&panic_notifier_list, &panic_block);
> >>
> >>- initialized = 1;
> >>+ initialized = true;
> >>
> >>- return 0;
> >>+out:
> >>+ mutex_unlock(&ipmi_interfaces_mutex);
> >>+ return rv;
> >> }
> >>
> >> static int __init ipmi_init_msghandler_mod(void)
> >> {
> >>- ipmi_init_msghandler();
> >>- return 0;
> >>+ int rv;
> >>+
> >>+ pr_info("version " IPMI_DRIVER_VERSION "\n");
> >>+
> >>+ mutex_lock(&ipmi_interfaces_mutex);
> >>+ rv = ipmi_register_driver();
> >>+ mutex_unlock(&ipmi_interfaces_mutex);
> >>+
> >>+ return rv;
> >> }
> >>
> >> static void __exit cleanup_ipmi(void)
> >> {
> >> int count;
> >>
> >>- if (!initialized)
> >>- return;
> >>-
> >>- atomic_notifier_chain_unregister(&panic_notifier_list, &panic_block);
> >>-
> >>- /*
> >>- * This can't be called if any interfaces exist, so no worry
> >>- * about shutting down the interfaces.
> >>- */
> >>+ if (initialized) {
> >>+ atomic_notifier_chain_unregister(&panic_notifier_list,
> >>+ &panic_block);
> >>
> >>- /*
> >>- * Tell the timer to stop, then wait for it to stop. This
> >>- * avoids problems with race conditions removing the timer
> >>- * here.
> >>- */
> >>- atomic_inc(&stop_operation);
> >>- del_timer_sync(&ipmi_timer);
> >>+ /*
> >>+ * This can't be called if any interfaces exist, so no worry
> >>+ * about shutting down the interfaces.
> >>+ */
> >>
> >>- driver_unregister(&ipmidriver.driver);
> >>+ /*
> >>+ * Tell the timer to stop, then wait for it to stop. This
> >>+ * avoids problems with race conditions removing the timer
> >>+ * here.
> >>+ */
> >>+ atomic_inc(&stop_operation);
> >>+ del_timer_sync(&ipmi_timer);
> >>
> >>- initialized = 0;
> >>+ initialized = false;
> >>
> >>- /* Check for buffer leaks. */
> >>- count = atomic_read(&smi_msg_inuse_count);
> >>- if (count != 0)
> >>- pr_warn("SMI message count %d at exit\n", count);
> >>- count = atomic_read(&recv_msg_inuse_count);
> >>- if (count != 0)
> >>- pr_warn("recv message count %d at exit\n", count);
> >>+ /* Check for buffer leaks. */
> >>+ count = atomic_read(&smi_msg_inuse_count);
> >>+ if (count != 0)
> >>+ pr_warn("SMI message count %d at exit\n", count);
> >>+ count = atomic_read(&recv_msg_inuse_count);
> >>+ if (count != 0)
> >>+ pr_warn("recv message count %d at exit\n", count);
> >>+ }
> >>+ if (drvregistered)
> >>+ driver_unregister(&ipmidriver.driver);
> >> }
> >> module_exit(cleanup_ipmi);
> >>
> >>--
> >>2.17.1
> >>
>
_______________________________________________
Openipmi-developer mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
