Patchwork [repost,for-3.9] pci: avoid work_on_cpu for nested SRIOV probes

login
register
mail settings
Submitter Tejun Heo
Date April 11, 2013, 8:41 p.m.
Message ID <20130411204104.GC11956@mtj.dyndns.org>
Download mbox | patch
Permalink /patch/235923/
State Not Applicable
Headers show

Comments

Tejun Heo - April 11, 2013, 8:41 p.m.
Hello,

On Thu, Apr 11, 2013 at 11:30:53PM +0300, Michael S. Tsirkin wrote:
> Okay, so you are saying it's a false-positive?

Yeah, I think so.  It didn't actually lock up, right?  It it did,
our analysis upto this point is likely to be completely wrong.

> Want to send a patch so Or can try it out?

Hmmm... something like the following on the workqueue side (completely
untested).

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Or Gerlitz - April 11, 2013, 9:52 p.m.
On Thu, Apr 11, 2013 at 11:41 PM, Tejun Heo <tj@kernel.org> wrote:
> Hello,
>
> On Thu, Apr 11, 2013 at 11:30:53PM +0300, Michael S. Tsirkin wrote:
>> Okay, so you are saying it's a false-positive?
>
> Yeah, I think so.  It didn't actually lock up, right?  It it did,
> our analysis upto this point is likely to be completely wrong.
>
>> Want to send a patch so Or can try it out?


I can test that Sunday
>
> Hmmm... something like the following on the workqueue side (completely
> untested).
>
> diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
> index 8afab27..899d470 100644
> --- a/include/linux/workqueue.h
> +++ b/include/linux/workqueue.h
> @@ -466,14 +466,21 @@ static inline bool __deprecated flush_delayed_work_sync(struct delayed_work *dwo
>  }
>
>  #ifndef CONFIG_SMP
> -static inline long work_on_cpu(unsigned int cpu, long (*fn)(void *), void *arg)
> +static inline long work_on_cpu_nested(unsigned int cpu, long (*fn)(void *),
> +                                     void *arg, int subclass)
>  {
>         return fn(arg);
>  }
>  #else
> -long work_on_cpu(unsigned int cpu, long (*fn)(void *), void *arg);
> +long work_on_cpu_nested(unsigned int cpu, long (*fn)(void *), void *arg,
> +                       int subclass);
>  #endif /* CONFIG_SMP */
>
> +static inline long work_on_cpu(unsigned int cpu, long (*fn)(void *), void *arg)
> +{
> +       return work_on_cpu_nested(cpu, fn, arg, 0);
> +}
> +
>  #ifdef CONFIG_FREEZER
>  extern void freeze_workqueues_begin(void);
>  extern bool freeze_workqueues_busy(void);
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 81f2457..c2be670 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -3555,25 +3555,30 @@ static void work_for_cpu_fn(struct work_struct *work)
>  }
>
>  /**
> - * work_on_cpu - run a function in user context on a particular cpu
> + * work_on_cpu_nested - run a function in user context on a particular cpu
>   * @cpu: the cpu to run on
>   * @fn: the function to run
>   * @arg: the function arg
> + * @subclass: lockdep subclass
>   *
>   * This will return the value @fn returns.
>   * It is up to the caller to ensure that the cpu doesn't go offline.
>   * The caller must not hold any locks which would prevent @fn from completing.
> + *
> + * XXX: explain @subclass
>   */
> -long work_on_cpu(unsigned int cpu, long (*fn)(void *), void *arg)
> +long work_on_cpu_nested(unsigned int cpu, long (*fn)(void *), void *arg,
> +                       int subclass)
>  {
>         struct work_for_cpu wfc = { .fn = fn, .arg = arg };
>
>         INIT_WORK_ONSTACK(&wfc.work, work_for_cpu_fn);
> +       lock_set_subclass(&wfc.work.lockdep_map, subclass, _RET_IP_);
>         schedule_work_on(cpu, &wfc.work);
>         flush_work(&wfc.work);
>         return wfc.ret;
>  }
> -EXPORT_SYMBOL_GPL(work_on_cpu);
> +EXPORT_SYMBOL_GPL(work_on_cpu_nested);
>  #endif /* CONFIG_SMP */
>
>  #ifdef CONFIG_FREEZER
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo - April 14, 2013, 1:43 p.m.
On Sun, Apr 14, 2013 at 03:58:55PM +0300, Or Gerlitz wrote:
> So the patch eliminated the lockdep warning for mlx4 nested probing
> sequence, but introduced lockdep warning for
> 00:13.0 PIC: Intel Corporation 7500/5520/5500/X58 I/O Hub I/OxAPIC
> Interrupt Controller (rev 22)

Oops, the patch in itself doesn't really change anything.  The caller
should use a different subclass for the nested invocation, just like
spin_lock_nested() and friends.  Sorry about not being clear.
Michael, can you please help?

Thanks.

Patch

diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index 8afab27..899d470 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -466,14 +466,21 @@  static inline bool __deprecated flush_delayed_work_sync(struct delayed_work *dwo
 }
 
 #ifndef CONFIG_SMP
-static inline long work_on_cpu(unsigned int cpu, long (*fn)(void *), void *arg)
+static inline long work_on_cpu_nested(unsigned int cpu, long (*fn)(void *),
+				      void *arg, int subclass)
 {
 	return fn(arg);
 }
 #else
-long work_on_cpu(unsigned int cpu, long (*fn)(void *), void *arg);
+long work_on_cpu_nested(unsigned int cpu, long (*fn)(void *), void *arg,
+			int subclass);
 #endif /* CONFIG_SMP */
 
+static inline long work_on_cpu(unsigned int cpu, long (*fn)(void *), void *arg)
+{
+	return work_on_cpu_nested(cpu, fn, arg, 0);
+}
+
 #ifdef CONFIG_FREEZER
 extern void freeze_workqueues_begin(void);
 extern bool freeze_workqueues_busy(void);
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 81f2457..c2be670 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -3555,25 +3555,30 @@  static void work_for_cpu_fn(struct work_struct *work)
 }
 
 /**
- * work_on_cpu - run a function in user context on a particular cpu
+ * work_on_cpu_nested - run a function in user context on a particular cpu
  * @cpu: the cpu to run on
  * @fn: the function to run
  * @arg: the function arg
+ * @subclass: lockdep subclass
  *
  * This will return the value @fn returns.
  * It is up to the caller to ensure that the cpu doesn't go offline.
  * The caller must not hold any locks which would prevent @fn from completing.
+ *
+ * XXX: explain @subclass
  */
-long work_on_cpu(unsigned int cpu, long (*fn)(void *), void *arg)
+long work_on_cpu_nested(unsigned int cpu, long (*fn)(void *), void *arg,
+			int subclass)
 {
 	struct work_for_cpu wfc = { .fn = fn, .arg = arg };
 
 	INIT_WORK_ONSTACK(&wfc.work, work_for_cpu_fn);
+	lock_set_subclass(&wfc.work.lockdep_map, subclass, _RET_IP_);
 	schedule_work_on(cpu, &wfc.work);
 	flush_work(&wfc.work);
 	return wfc.ret;
 }
-EXPORT_SYMBOL_GPL(work_on_cpu);
+EXPORT_SYMBOL_GPL(work_on_cpu_nested);
 #endif /* CONFIG_SMP */
 
 #ifdef CONFIG_FREEZER