diff mbox

unsafe locks seen with netperf on net-2.6.29 tree

Message ID 1230300535.9487.292.camel@twins
State Not Applicable, archived
Delegated to: David Miller
Headers show

Commit Message

Peter Zijlstra Dec. 26, 2008, 2:08 p.m. UTC 
On Thu, 2008-12-25 at 22:26 +1100, Herbert Xu wrote:
> On Thu, Dec 25, 2008 at 10:25:44AM +0000, Jeff Kirsher wrote:
> >
> > [ 1439.758437] ======================================================
> > [ 1439.758724] [ INFO: soft-safe -> soft-unsafe lock order detected ]
> > [ 1439.758868] 2.6.28-rc8-net-next-igb #13
> > [ 1439.759007] ------------------------------------------------------
> > [ 1439.759150] netperf/22302 [HC0[0]:SC0[1]:HE1:SE0] is trying to acquire:
> > [ 1439.759293]  (&fbc->lock){--..}, at: [<ffffffff803691a6>]
> > __percpu_counter_add+0x4a/0x6d
> > [ 1439.759581]
> > [ 1439.759582] and this task is already holding:
> > [ 1439.759853]  (slock-AF_INET){-+..}, at: [<ffffffff804fdca6>]
> > tcp_close+0x16c/0x2da
> > [ 1439.760137] which would create a new lock dependency:
> > [ 1439.762122]  (slock-AF_INET){-+..} -> (&fbc->lock){--..}
> 
> This is a false positive.  The lock slock is not a normal lock.
> It's an ancient creature that's a spinlock in interrupt context
> and a semaphore in process context.
> 
> In particular, holding slock in process context does not disable
> softirqs and you're still allowed to take the spinlock portion of
> slock on the same CPU through an interrupt.  What happens is that
> the softirq will notice that the slock is already taken by process
> context, and defer the work for later.

Which doesn't seem to be relevant to the report in question.

What happens is that two different percpu counters with different irq
semantics get put in the same class (I suspect Eric never tested this
stuff with lockdep enabled).

Does the below -- which isn't even compile tested solve the issue?

Jeff, would you, in future, take care not to word wrap splats like that,
it takes way too much effort to untangle that mess.

---
Subject: lockdep: annotate percpu_counter

Classify percpu_counter instances similar to regular lock objects --
that is, per instantiation site.

Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
 include/linux/percpu_counter.h |   14 ++++++++++----
 lib/percpu_counter.c           |   18 ++++--------------
 lib/proportions.c              |    6 +++---
 mm/backing-dev.c               |    2 +-
 4 files changed, 18 insertions(+), 22 deletions(-)



--
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

Comments

Tantilov, Emil S Dec. 27, 2008, 7:38 p.m. UTC | #1 
Peter Zijlstra wrote:
> On Thu, 2008-12-25 at 22:26 +1100, Herbert Xu wrote:
>> On Thu, Dec 25, 2008 at 10:25:44AM +0000, Jeff Kirsher wrote:
>>>
>>> [ 1439.758437]
>>> ====================================================== [
>>> 1439.758724] [ INFO: soft-safe -> soft-unsafe lock order detected ]
>>> [ 1439.758868] 2.6.28-rc8-net-next-igb #13 [ 1439.759007]
>>> ------------------------------------------------------ [
>>> 1439.759150] netperf/22302 [HC0[0]:SC0[1]:HE1:SE0] is trying to
>>> acquire: [ 1439.759293]  (&fbc->lock){--..}, at:
>>> [<ffffffff803691a6>] __percpu_counter_add+0x4a/0x6d [ 1439.759581]
>>> [ 1439.759582] and this task is already holding:
>>> [ 1439.759853]  (slock-AF_INET){-+..}, at: [<ffffffff804fdca6>]
>>> tcp_close+0x16c/0x2da [ 1439.760137] which would create a new lock
>>> dependency: [ 1439.762122]  (slock-AF_INET){-+..} ->
>>> (&fbc->lock){--..}
>>
>> This is a false positive.  The lock slock is not a normal lock.
>> It's an ancient creature that's a spinlock in interrupt context
>> and a semaphore in process context.
>>
>> In particular, holding slock in process context does not disable
>> softirqs and you're still allowed to take the spinlock portion of
>> slock on the same CPU through an interrupt.  What happens is that
>> the softirq will notice that the slock is already taken by process
>> context, and defer the work for later.
>
> Which doesn't seem to be relevant to the report in question.
>
> What happens is that two different percpu counters with different irq
> semantics get put in the same class (I suspect Eric never tested this
> stuff with lockdep enabled).
>
> Does the below -- which isn't even compile tested solve the issue?
>
> Jeff, would you, in future, take care not to word wrap splats like
> that,
> it takes way too much effort to untangle that mess.
>
> ---
> Subject: lockdep: annotate percpu_counter
>
> Classify percpu_counter instances similar to regular lock objects --
> that is, per instantiation site.
>
> Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> ---
>  include/linux/percpu_counter.h |   14 ++++++++++----
>  lib/percpu_counter.c           |   18 ++++--------------
>  lib/proportions.c              |    6 +++---
>  mm/backing-dev.c               |    2 +-
>  4 files changed, 18 insertions(+), 22 deletions(-)
>
> diff --git a/include/linux/percpu_counter.h
> b/include/linux/percpu_counter.h
> index 9007ccd..a074d77 100644
> --- a/include/linux/percpu_counter.h
> +++ b/include/linux/percpu_counter.h
> @@ -30,8 +30,16 @@ struct percpu_counter {
>  #define FBC_BATCH    (NR_CPUS*4)
>  #endif
>
> -int percpu_counter_init(struct percpu_counter *fbc, s64 amount);
> -int percpu_counter_init_irq(struct percpu_counter *fbc, s64 amount);
> +int __percpu_counter_init(struct percpu_counter *fbc, s64 amount,
> +                       struct lock_class_key *key);
> +
> +#define percpu_counter_init(fbc, value)                                      \
> +     do {                                                            \
> +             static struct lock_class_key __key;                     \
> +                                                                     \
> +             __percpu_counter_init(fbc, value, &__key);              \
> +     } while (0)
> +
>  void percpu_counter_destroy(struct percpu_counter *fbc);
>  void percpu_counter_set(struct percpu_counter *fbc, s64 amount);
>  void __percpu_counter_add(struct percpu_counter *fbc, s64 amount,
> s32 batch); @@ -85,8 +93,6 @@ static inline int
>       percpu_counter_init(struct percpu_counter *fbc, s64 amount) return
>  0; }
>
> -#define percpu_counter_init_irq percpu_counter_init
> -
>  static inline void percpu_counter_destroy(struct percpu_counter *fbc)
>  {
>  }
> diff --git a/lib/percpu_counter.c b/lib/percpu_counter.c
> index b255b93..4bb0ed3 100644
> --- a/lib/percpu_counter.c
> +++ b/lib/percpu_counter.c
> @@ -68,11 +68,11 @@ s64 __percpu_counter_sum(struct percpu_counter
>  *fbc) }
>  EXPORT_SYMBOL(__percpu_counter_sum);
>
> -static struct lock_class_key percpu_counter_irqsafe;
> -
> -int percpu_counter_init(struct percpu_counter *fbc, s64 amount)
> +int __percpu_counter_init(struct percpu_counter *fbc, s64 amount,
> +                       struct lock_class_key *key)
>  {
>       spin_lock_init(&fbc->lock);
> +     lockdep_set_class(&fbc->lock, key);
>       fbc->count = amount;
>       fbc->counters = alloc_percpu(s32);
>       if (!fbc->counters)
> @@ -84,17 +84,7 @@ int percpu_counter_init(struct percpu_counter
>  *fbc, s64 amount) #endif
>       return 0;
>  }
> -EXPORT_SYMBOL(percpu_counter_init);
> -
> -int percpu_counter_init_irq(struct percpu_counter *fbc, s64 amount)
> -{
> -     int err;
> -
> -     err = percpu_counter_init(fbc, amount);
> -     if (!err)
> -             lockdep_set_class(&fbc->lock, &percpu_counter_irqsafe);
> -     return err;
> -}
> +EXPORT_SYMBOL(__percpu_counter_init);
>
>  void percpu_counter_destroy(struct percpu_counter *fbc)
>  {
> diff --git a/lib/proportions.c b/lib/proportions.c
> index 4f387a6..7367f2b 100644
> --- a/lib/proportions.c
> +++ b/lib/proportions.c
> @@ -83,11 +83,11 @@ int prop_descriptor_init(struct prop_descriptor
>       *pd, int shift) pd->index = 0;
>       pd->pg[0].shift = shift;
>       mutex_init(&pd->mutex);
> -     err = percpu_counter_init_irq(&pd->pg[0].events, 0);
> +     err = percpu_counter_init(&pd->pg[0].events, 0);
>       if (err)
>               goto out;
>
> -     err = percpu_counter_init_irq(&pd->pg[1].events, 0);
> +     err = percpu_counter_init(&pd->pg[1].events, 0);
>       if (err)
>               percpu_counter_destroy(&pd->pg[0].events);
>
> @@ -191,7 +191,7 @@ int prop_local_init_percpu(struct
>       prop_local_percpu *pl) spin_lock_init(&pl->lock);
>       pl->shift = 0;
>       pl->period = 0;
> -     return percpu_counter_init_irq(&pl->events, 0);
> +     return percpu_counter_init(&pl->events, 0);
>  }
>
>  void prop_local_destroy_percpu(struct prop_local_percpu *pl)
> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> index 801c08b..a7c6c56 100644
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -223,7 +223,7 @@ int bdi_init(struct backing_dev_info *bdi)
>       bdi->max_prop_frac = PROP_FRAC_BASE;
>
>       for (i = 0; i < NR_BDI_STAT_ITEMS; i++) {
> -             err = percpu_counter_init_irq(&bdi->bdi_stat[i], 0);
> +             err = percpu_counter_init(&bdi->bdi_stat[i], 0);
>               if (err)
>                       goto err;
>       }

This patch fails to compile:

mm/backing-dev.c: In function 'bdi_init':
mm/backing-dev.c:226: error: expected expression bedore 'do'

Thakns,
Emil
--
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
Peter Zijlstra Dec. 27, 2008, 8:38 p.m. UTC | #2 
On Sat, 2008-12-27 at 12:38 -0700, Tantilov, Emil S wrote:
> Peter Zijlstra wrote:

> > index 9007ccd..a074d77 100644
> > --- a/include/linux/percpu_counter.h
> > +++ b/include/linux/percpu_counter.h
> > @@ -30,8 +30,16 @@ struct percpu_counter {
> >  #define FBC_BATCH    (NR_CPUS*4)
> >  #endif
> >
> > -int percpu_counter_init(struct percpu_counter *fbc, s64 amount);
> > -int percpu_counter_init_irq(struct percpu_counter *fbc, s64 amount);
> > +int __percpu_counter_init(struct percpu_counter *fbc, s64 amount,
> > +                       struct lock_class_key *key);
> > +
> > +#define percpu_counter_init(fbc, value)                                      \
> > +     do {                                                            \
> > +             static struct lock_class_key __key;                     \
> > +                                                                     \
> > +             __percpu_counter_init(fbc, value, &__key);              \
> > +     } while (0)
> > +
> >  void percpu_counter_destroy(struct percpu_counter *fbc);
> >  void percpu_counter_set(struct percpu_counter *fbc, s64 amount);
> >  void __percpu_counter_add(struct percpu_counter *fbc, s64 amount,

> This patch fails to compile:
> 
> mm/backing-dev.c: In function 'bdi_init':
> mm/backing-dev.c:226: error: expected expression bedore 'do'

Ah indeed, stupid me...

Please try something like this instead of the above hunk:

@@ -30,8 +30,16 @@ struct percpu_counter {
 #define FBC_BATCH      (NR_CPUS*4)
 #endif

-int percpu_counter_init(struct percpu_counter *fbc, s64 amount);
-int percpu_counter_init_irq(struct percpu_counter *fbc, s64 amount);
+int __percpu_counter_init(struct percpu_counter *fbc, s64 amount,
+                         struct lock_class_key *key);
+
+#define percpu_counter_init(fbc, value)                                        \
+       ({                                                              \
+               static struct lock_class_key __key;                     \
+                                                                       \
+               __percpu_counter_init(fbc, value, &__key);              \
+       })
+
 void percpu_counter_destroy(struct percpu_counter *fbc);
 void percpu_counter_set(struct percpu_counter *fbc, s64 amount);
 void __percpu_counter_add(struct percpu_counter *fbc, s64 amount, s32 batch)

--
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
Tantilov, Emil S Dec. 28, 2008, 12:54 a.m. UTC | #3 
Peter Zijlstra wrote:
> On Sat, 2008-12-27 at 12:38 -0700, Tantilov, Emil S wrote:
>> Peter Zijlstra wrote:
>
>>> index 9007ccd..a074d77 100644
>>> --- a/include/linux/percpu_counter.h
>>> +++ b/include/linux/percpu_counter.h
>>> @@ -30,8 +30,16 @@ struct percpu_counter {
>>>  #define FBC_BATCH    (NR_CPUS*4)
>>>  #endif
>>>
>>> -int percpu_counter_init(struct percpu_counter *fbc, s64 amount);
>>> -int percpu_counter_init_irq(struct percpu_counter *fbc, s64
>>> amount); +int __percpu_counter_init(struct percpu_counter *fbc, s64
>>> amount, +                       struct lock_class_key *key); +
>>> +#define percpu_counter_init(fbc, value)
>>> \ +     do {
>>> \ +             static struct lock_class_key __key;
>>> \ +
>>> \ +             __percpu_counter_init(fbc, value, &__key);
>>> \ +     } while (0) +
>>>  void percpu_counter_destroy(struct percpu_counter *fbc);
>>>  void percpu_counter_set(struct percpu_counter *fbc, s64 amount);
>>>  void __percpu_counter_add(struct percpu_counter *fbc, s64 amount,
>
>> This patch fails to compile:
>>
>> mm/backing-dev.c: In function 'bdi_init':
>> mm/backing-dev.c:226: error: expected expression bedore 'do'
>
> Ah indeed, stupid me...
>
> Please try something like this instead of the above hunk:
>
> @@ -30,8 +30,16 @@ struct percpu_counter {
>  #define FBC_BATCH      (NR_CPUS*4)
>  #endif
>
> -int percpu_counter_init(struct percpu_counter *fbc, s64 amount);
> -int percpu_counter_init_irq(struct percpu_counter *fbc, s64 amount);
> +int __percpu_counter_init(struct percpu_counter *fbc, s64 amount,
> +                         struct lock_class_key *key);
> +
> +#define percpu_counter_init(fbc, value)
> \ +       ({
> \ +               static struct lock_class_key __key;
> \ +
> \ +               __percpu_counter_init(fbc, value, &__key);
> \ +       })
> +
>  void percpu_counter_destroy(struct percpu_counter *fbc);
>  void percpu_counter_set(struct percpu_counter *fbc, s64 amount);
>  void __percpu_counter_add(struct percpu_counter *fbc, s64 amount,
> s32 batch)

With this compiled, but I still get the following:

[  435.632627] =================================
[  435.633030] [ INFO: inconsistent lock state ]
[  435.633037] 2.6.28-rc8-net-next-igbL #14
[  435.633040] ---------------------------------
[  435.633044] inconsistent {in-softirq-W} -> {softirq-on-W} usage.
[  435.633049] netperf/12669 [HC0[0]:SC0[0]:HE1:SE1] takes:
[  435.633053]  (key#8){-+..}, at: [<ffffffff803691ac>] __percpu_counter_add+0x4a/0x6d
[  435.633068] {in-softirq-W} state was registered at:
[  435.633070]   [<ffffffffffffffff>] 0xffffffffffffffff
[  435.633078] irq event stamp: 988533
[  435.633080] hardirqs last  enabled at (988533): [<ffffffff80243712>] _local_bh_enable_ip+0xc8/0xcd
[  435.633088] hardirqs last disabled at (988531): [<ffffffff8024369e>] _local_bh_enable_ip+0x54/0xcd
[  435.633093] softirqs last  enabled at (988532): [<ffffffff804fc814>] sock_orphan+0x3f/0x44
[  435.633100] softirqs last disabled at (988530): [<ffffffff8056454d>] _write_lock_bh+0x11/0x3d
[  435.633107]
[  435.633108] other info that might help us debug this:
[  435.633110] 1 lock held by netperf/12669:
[  435.633112]  #0:  (sk_lock-AF_INET6){--..}, at: [<ffffffff804fc544>] lock_sock+0xb/0xd
[  435.633119]
[  435.633120] stack backtrace:
[  435.633124] Pid: 12669, comm: netperf Not tainted 2.6.28-rc8-net-next-igbL #14
[  435.633127] Call Trace:
[  435.633134]  [<ffffffff8025ffb8>] print_usage_bug+0x159/0x16a
[  435.633139]  [<ffffffff8026000e>] valid_state+0x45/0x52
[  435.633143]  [<ffffffff802601cf>] mark_lock_irq+0x1b4/0x27b
[  435.633148]  [<ffffffff80260339>] mark_lock+0xa3/0x110
[  435.633152]  [<ffffffff80260480>] mark_irqflags+0xda/0xf2
[  435.633157]  [<ffffffff8026122e>] __lock_acquire+0x1c3/0x2ee
[  435.633161]  [<ffffffff80261d93>] lock_acquire+0x55/0x71
[  435.633166]  [<ffffffff803691ac>] ? __percpu_counter_add+0x4a/0x6d
[  435.633170]  [<ffffffff80564434>] _spin_lock+0x2c/0x38
[  435.633175]  [<ffffffff803691ac>] ? __percpu_counter_add+0x4a/0x6d
[  435.633179]  [<ffffffff803691ac>] __percpu_counter_add+0x4a/0x6d
[  435.633184]  [<ffffffff804fc827>] percpu_counter_add+0xe/0x10
[  435.633188]  [<ffffffff804fc837>] percpu_counter_inc+0xe/0x10
[  435.633193]  [<ffffffff804fdc91>] tcp_close+0x157/0x2da
[  435.633197]  [<ffffffff8051907e>] inet_release+0x58/0x5f
[  435.633204]  [<ffffffff80527c48>] inet6_release+0x30/0x35
[  435.633213]  [<ffffffff804c9354>] sock_release+0x1a/0x76
[  435.633221]  [<ffffffff804c9804>] sock_close+0x22/0x26
[  435.633229]  [<ffffffff802a345a>] __fput+0x82/0x110
[  435.633234]  [<ffffffff802a381a>] fput+0x15/0x17
[  435.633239]  [<ffffffff802a09c5>] filp_close+0x67/0x72
[  435.633246]  [<ffffffff80240ae3>] close_files+0x66/0x8d
[  435.633251]  [<ffffffff80240b39>] put_files_struct+0x19/0x42
[  435.633256]  [<ffffffff80240b98>] exit_files+0x36/0x3b
[  435.633260]  [<ffffffff80241eec>] do_exit+0x1b7/0x2b1
[  435.633265]  [<ffffffff80242087>] sys_exit_group+0x0/0x14
[  435.633269]  [<ffffffff80242099>] sys_exit_group+0x12/0x14
[  435.633275]  [<ffffffff8020b9cb>] system_call_fastpath+0x16/0x1b

Thanks,
Emil
--
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
diff mbox

Patch

diff --git a/include/linux/percpu_counter.h b/include/linux/percpu_counter.h
index 9007ccd..a074d77 100644
--- a/include/linux/percpu_counter.h
+++ b/include/linux/percpu_counter.h
@@ -30,8 +30,16 @@  struct percpu_counter {
 #define FBC_BATCH	(NR_CPUS*4)
 #endif
 
-int percpu_counter_init(struct percpu_counter *fbc, s64 amount);
-int percpu_counter_init_irq(struct percpu_counter *fbc, s64 amount);
+int __percpu_counter_init(struct percpu_counter *fbc, s64 amount,
+			  struct lock_class_key *key);
+
+#define percpu_counter_init(fbc, value)					\
+	do {								\
+		static struct lock_class_key __key;			\
+									\
+		__percpu_counter_init(fbc, value, &__key);		\
+	} while (0)
+
 void percpu_counter_destroy(struct percpu_counter *fbc);
 void percpu_counter_set(struct percpu_counter *fbc, s64 amount);
 void __percpu_counter_add(struct percpu_counter *fbc, s64 amount, s32 batch);
@@ -85,8 +93,6 @@  static inline int percpu_counter_init(struct percpu_counter *fbc, s64 amount)
 	return 0;
 }
 
-#define percpu_counter_init_irq percpu_counter_init
-
 static inline void percpu_counter_destroy(struct percpu_counter *fbc)
 {
 }
diff --git a/lib/percpu_counter.c b/lib/percpu_counter.c
index b255b93..4bb0ed3 100644
--- a/lib/percpu_counter.c
+++ b/lib/percpu_counter.c
@@ -68,11 +68,11 @@  s64 __percpu_counter_sum(struct percpu_counter *fbc)
 }
 EXPORT_SYMBOL(__percpu_counter_sum);
 
-static struct lock_class_key percpu_counter_irqsafe;
-
-int percpu_counter_init(struct percpu_counter *fbc, s64 amount)
+int __percpu_counter_init(struct percpu_counter *fbc, s64 amount,
+			  struct lock_class_key *key)
 {
 	spin_lock_init(&fbc->lock);
+	lockdep_set_class(&fbc->lock, key);
 	fbc->count = amount;
 	fbc->counters = alloc_percpu(s32);
 	if (!fbc->counters)
@@ -84,17 +84,7 @@  int percpu_counter_init(struct percpu_counter *fbc, s64 amount)
 #endif
 	return 0;
 }
-EXPORT_SYMBOL(percpu_counter_init);
-
-int percpu_counter_init_irq(struct percpu_counter *fbc, s64 amount)
-{
-	int err;
-
-	err = percpu_counter_init(fbc, amount);
-	if (!err)
-		lockdep_set_class(&fbc->lock, &percpu_counter_irqsafe);
-	return err;
-}
+EXPORT_SYMBOL(__percpu_counter_init);
 
 void percpu_counter_destroy(struct percpu_counter *fbc)
 {
diff --git a/lib/proportions.c b/lib/proportions.c
index 4f387a6..7367f2b 100644
--- a/lib/proportions.c
+++ b/lib/proportions.c
@@ -83,11 +83,11 @@  int prop_descriptor_init(struct prop_descriptor *pd, int shift)
 	pd->index = 0;
 	pd->pg[0].shift = shift;
 	mutex_init(&pd->mutex);
-	err = percpu_counter_init_irq(&pd->pg[0].events, 0);
+	err = percpu_counter_init(&pd->pg[0].events, 0);
 	if (err)
 		goto out;
 
-	err = percpu_counter_init_irq(&pd->pg[1].events, 0);
+	err = percpu_counter_init(&pd->pg[1].events, 0);
 	if (err)
 		percpu_counter_destroy(&pd->pg[0].events);
 
@@ -191,7 +191,7 @@  int prop_local_init_percpu(struct prop_local_percpu *pl)
 	spin_lock_init(&pl->lock);
 	pl->shift = 0;
 	pl->period = 0;
-	return percpu_counter_init_irq(&pl->events, 0);
+	return percpu_counter_init(&pl->events, 0);
 }
 
 void prop_local_destroy_percpu(struct prop_local_percpu *pl)
diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index 801c08b..a7c6c56 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -223,7 +223,7 @@  int bdi_init(struct backing_dev_info *bdi)
 	bdi->max_prop_frac = PROP_FRAC_BASE;
 
 	for (i = 0; i < NR_BDI_STAT_ITEMS; i++) {
-		err = percpu_counter_init_irq(&bdi->bdi_stat[i], 0);
+		err = percpu_counter_init(&bdi->bdi_stat[i], 0);
 		if (err)
 			goto err;
 	}