by John W. Linville

[permalink] [raw]

Hello,

> > > usb 2-1: USB disconnect, address 2
> > > BUG: atomic counter underflow at:
> > > [<c010456a>] show_trace_log_lvl+0x1a/0x30
> > > [<c010508d>] show_trace+0x12/0x14
> > > [<c01051e0>] dump_stack+0x15/0x17
> > > [<c01418cf>] __free_pages+0x50/0x52
> > > [<c01418f0>] free_pages+0x1f/0x21
> > > [<c010783d>] dma_free_coherent+0x43/0x9c
> > > [<c0315067>] hcd_buffer_free+0x43/0x6a
> > > [<c030b2b4>] usb_buffer_free+0x23/0x29
> > > [<c0346db4>] hid_free_buffers+0x23/0x71
> > > [<c0346eb2>] hid_disconnect+0xb0/0xc8
> > > [<c0313676>] usb_unbind_interface+0x30/0x72
> > > [<c02c6df0>] __device_release_driver+0x6a/0x92
> > > [<c02c71c3>] device_release_driver+0x20/0x36
> > > [<c02c6736>] bus_remove_device+0x62/0x85
> > > [<c02c49f8>] device_del+0x16d/0x27c
> > > [<c0310f25>] usb_disable_device+0x7a/0xe2
> > > [<c030d0bc>] usb_disconnect+0x94/0xde
> > > [<c030e030>] hub_thread+0x2fe/0xc1b
> > > [<c0128aee>] kthread+0x36/0x58
> > > [<c0104233>] kernel_thread_helper+0x7/0x14
> > > =======================
> > > uhci_hcd 0000:00:0c.0: dma_pool_free buffer-32, 6b6b6b6b/6b6b6b6b (bad dma)
>
> Mariusz,
>
> I guess the patch below (which I have just added to my tree) fixes that,
> right? Thanks.

Yes - that's correct. This patch fixes the bug. Thanks.

Mariusz

2007-08-02 10:15:23

On Fri, 2007-08-03 at 21:21 +0400, Oleg Nesterov wrote:

> To avoid a possible confusion: it is still OK if work->func() flushes
> its own workqueue, so strictly speaking this trace is false positive,
> but it would be very nice if we can get rid of this practice.

I just had a thought: we could get rid of this warning by using a
read-lock here. That way, flushing from within a work function (which
would be seen as read-after-read recursive lock) won't trigger this
warning. Patch below. This would, however, also get rid of any warnings
for run_workqueue recursion. Which again we may or may not want, the
code inidicates that it should be allowed up to a depth of three.

However, the question whether we should allow flush_workqueue from
within a struct work is mainly an API policy issue; it doesn't hurt to
flush a workqueue from within a work, but it is probably nearer the
intent to use targeted cancel_work_sync() or such. OTOH, one could
imagine situations where multiple different work structs are on that
workqueue belonging to the same subsystem and then the general
flush_scheduled_work() call is the only way to guarantee nothing is on
scheduled at a given point... I don't feel qualified to make the
decision for or against allowing this use of the API at this point.

Marc, do you have an easy way to trigger this warning? Could you verify
that it goes away with the patch below applied?

johannes

---
kernel/workqueue.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

--- wireless-dev.orig/kernel/workqueue.c 2007-08-06 08:11:23.297846657 +0200
+++ wireless-dev/kernel/workqueue.c 2007-08-06 08:19:54.727846657 +0200
@@ -272,7 +272,7 @@ static void run_workqueue(struct cpu_wor

BUG_ON(get_wq_data(work) != cwq);
work_clear_pending(work);
- lock_acquire(&cwq->wq->lockdep_map, 0, 0, 0, 2, _THIS_IP_);
+ lock_acquire(&cwq->wq->lockdep_map, 0, 0, 1, 2, _THIS_IP_);
lock_acquire(&lockdep_map, 0, 0, 0, 2, _THIS_IP_);
f(work);
lock_release(&lockdep_map, 1, _THIS_IP_);
@@ -395,7 +395,7 @@ void fastcall flush_workqueue(struct wor
int cpu;

might_sleep();
- lock_acquire(&wq->lockdep_map, 0, 0, 0, 2, _THIS_IP_);
+ lock_acquire(&wq->lockdep_map, 0, 0, 1, 2, _THIS_IP_);
lock_release(&wq->lockdep_map, 1, _THIS_IP_);
for_each_cpu_mask(cpu, *cpu_map)
flush_cpu_workqueue(per_cpu_ptr(wq->cpu_wq, cpu));
@@ -779,7 +779,7 @@ static void cleanup_workqueue_thread(str
if (cwq->thread == NULL)
return;

- lock_acquire(&cwq->wq->lockdep_map, 0, 0, 0, 2, _THIS_IP_);
+ lock_acquire(&cwq->wq->lockdep_map, 0, 0, 1, 2, _THIS_IP_);
lock_release(&cwq->wq->lockdep_map, 1, _THIS_IP_);

flush_cpu_workqueue(cwq);

Attachments:

signature.asc (190.00 B)
This is a digitally signed message part

2007-08-06 10:53:50

by Oleg Nesterov

[permalink] [raw]

Subject: Re: [NFS] 2.6.23-rc1-mm2

On 08/06, Johannes Berg wrote:
>
> On Fri, 2007-08-03 at 21:21 +0400, Oleg Nesterov wrote:
>
> > To avoid a possible confusion: it is still OK if work->func() flushes
> > its own workqueue, so strictly speaking this trace is false positive,
> > but it would be very nice if we can get rid of this practice.
>
> However, the question whether we should allow flush_workqueue from
> within a struct work is mainly an API policy issue; it doesn't hurt to
> flush a workqueue from within a work,

I am not sure, but currently I hope we can forbid this eventually, so I
personally think it is good that your patch complains.

> --- wireless-dev.orig/kernel/workqueue.c 2007-08-06 08:11:23.297846657 +0200
> +++ wireless-dev/kernel/workqueue.c 2007-08-06 08:19:54.727846657 +0200
> @@ -272,7 +272,7 @@ static void run_workqueue(struct cpu_wor
>
> BUG_ON(get_wq_data(work) != cwq);
> work_clear_pending(work);
> - lock_acquire(&cwq->wq->lockdep_map, 0, 0, 0, 2, _THIS_IP_);
> + lock_acquire(&cwq->wq->lockdep_map, 0, 0, 1, 2, _THIS_IP_);
> lock_acquire(&lockdep_map, 0, 0, 0, 2, _THIS_IP_);
> f(work);
> lock_release(&lockdep_map, 1, _THIS_IP_);
> @@ -395,7 +395,7 @@ void fastcall flush_workqueue(struct wor
> int cpu;
>
> might_sleep();
> - lock_acquire(&wq->lockdep_map, 0, 0, 0, 2, _THIS_IP_);
> + lock_acquire(&wq->lockdep_map, 0, 0, 1, 2, _THIS_IP_);
> lock_release(&wq->lockdep_map, 1, _THIS_IP_);
> for_each_cpu_mask(cpu, *cpu_map)
> flush_cpu_workqueue(per_cpu_ptr(wq->cpu_wq, cpu));
> @@ -779,7 +779,7 @@ static void cleanup_workqueue_thread(str
> if (cwq->thread == NULL)
> return;
>
> - lock_acquire(&cwq->wq->lockdep_map, 0, 0, 0, 2, _THIS_IP_);
> + lock_acquire(&cwq->wq->lockdep_map, 0, 0, 1, 2, _THIS_IP_);
> lock_release(&cwq->wq->lockdep_map, 1, _THIS_IP_);
>
> flush_cpu_workqueue(cwq);

But this makes ->lockdep_map meaningless? We always take wq->lockdep_map
for reading, now we can't detect deadlocks.

read_lock(A);
lock(B);

vs
lock(B);
read_lock(A);

is valid, kernel/lockdep.c should not complain.

No?

Oleg.

2007-08-06 10:57:28

On Mon, 2007-08-06 at 13:05 +0200, Marc Dietrich wrote:

> I applied the patch, but now I get:
>
> =================================
> [ INFO: inconsistent lock state ]
> 2.6.23-rc1-mm2 #4
> ---------------------------------
> inconsistent {softirq-on-W} -> {in-softirq-W} usage.
> swapper/0 [HC0[0]:SC1[1]:HE1:SE0] takes:
> (rpc_credcache_lock){-+..}, at: [<c01dc487>] _atomic_dec_and_lock+0x17/0x60

Interesting, but doesn't seem related to this at all. As Oleg just
pointed out this basically disabled checking for workqueue stuff so this
should be looked into by somebody familiar with the NFS code.

johannes

Attachments:

signature.asc (190.00 B)
This is a digitally signed message part

2007-08-06 16:24:47

On Tue, Jul 31, 2007 at 11:09:32PM -0700, Andrew Morton wrote:
>...
> Changes since 2.6.23-rc1-mm1:
>...
> +reiser4-cryptcompress-misc-fixups.patch
>...
> reiser4 updates
>...

This patch makes the following needlessly global functions static:
- file/cryptcompress.c: __put_page_cluster()
- file/cryptcompress.c: put_hint_cluster()
- item/ctail.c: ctail_read_disk_cluster()

Signed-off-by: Adrian Bunk <[email protected]>

---

fs/reiser4/plugin/cluster.h | 4 ----
fs/reiser4/plugin/file/cryptcompress.c | 8 ++++----
fs/reiser4/plugin/file/cryptcompress.h | 2 --
fs/reiser4/plugin/item/ctail.c | 5 +++--
4 files changed, 7 insertions(+), 12 deletions(-)

dcbd29fcda0d6143ab0ec5f91e347d6e540f27bd
diff --git a/fs/reiser4/plugin/cluster.h b/fs/reiser4/plugin/cluster.h
index 7856074..af7a305 100644
--- a/fs/reiser4/plugin/cluster.h
+++ b/fs/reiser4/plugin/cluster.h
@@ -326,8 +326,6 @@ int reiser4_deflate_cluster(struct cluster_handle *, struct inode *);
void truncate_complete_page_cluster(struct inode *inode, cloff_t start,
int even_cows);
void invalidate_hint_cluster(struct cluster_handle * clust);
-void put_hint_cluster(struct cluster_handle * clust, struct inode *inode,
- znode_lock_mode mode);
int get_disk_cluster_locked(struct cluster_handle * clust, struct inode * inode,
znode_lock_mode lock_mode);
void reset_cluster_params(struct cluster_handle * clust);
@@ -335,8 +333,6 @@ int set_cluster_by_page(struct cluster_handle * clust, struct page * page,
int count);
int prepare_page_cluster(struct inode *inode, struct cluster_handle * clust,
rw_op rw);
-void __put_page_cluster(int from, int to, struct page ** pages,
- struct inode * inode);
void put_page_cluster(struct cluster_handle * clust,
struct inode * inode, rw_op rw);
void put_cluster_handle(struct cluster_handle * clust);
diff --git a/fs/reiser4/plugin/file/cryptcompress.c b/fs/reiser4/plugin/file/cryptcompress.c
index f0f0bee..9724d64 100644
--- a/fs/reiser4/plugin/file/cryptcompress.c
+++ b/fs/reiser4/plugin/file/cryptcompress.c
@@ -1378,8 +1378,8 @@ static void truncate_page_cluster_range(struct inode * inode,
}

/* Put @count pages starting from @from offset */
-void __put_page_cluster(int from, int count,
- struct page ** pages, struct inode * inode)
+static void __put_page_cluster(int from, int count,
+ struct page ** pages, struct inode * inode)
{
int i;
assert("edward-1468", pages != NULL);
@@ -1975,8 +1975,8 @@ void invalidate_hint_cluster(struct cluster_handle * clust)
hint_clr_valid(clust->hint);
}

-void put_hint_cluster(struct cluster_handle * clust, struct inode *inode,
- znode_lock_mode mode)
+static void put_hint_cluster(struct cluster_handle * clust,
+ struct inode *inode, znode_lock_mode mode)
{
assert("edward-1286", clust != NULL);
assert("edward-1287", clust->hint != NULL);
diff --git a/fs/reiser4/plugin/file/cryptcompress.h b/fs/reiser4/plugin/file/cryptcompress.h
index 5984e3c..117d377 100644
--- a/fs/reiser4/plugin/file/cryptcompress.h
+++ b/fs/reiser4/plugin/file/cryptcompress.h
@@ -530,8 +530,6 @@ int equal_to_rdk(znode *, const reiser4_key *);
int goto_right_neighbor(coord_t *, lock_handle *);
int cryptcompress_inode_ok(struct inode *inode);
int coord_is_unprepped_ctail(const coord_t * coord);
-extern int ctail_read_disk_cluster (struct cluster_handle *, struct inode *,
- struct page *, znode_lock_mode mode);
extern int do_readpage_ctail(struct inode *, struct cluster_handle *,
struct page * page, znode_lock_mode mode);
extern int ctail_insert_unprepped_cluster(struct cluster_handle * clust,
diff --git a/fs/reiser4/plugin/item/ctail.c b/fs/reiser4/plugin/item/ctail.c
index a931392..33ab44a 100644
--- a/fs/reiser4/plugin/item/ctail.c
+++ b/fs/reiser4/plugin/item/ctail.c
@@ -551,8 +551,9 @@ int read_ctail(struct file *file UNUSED_ARG, flow_t * f, hint_t * hint)
* Prepare transform stream with plain text for page
* @page taking into account synchronization issues.
*/
-int ctail_read_disk_cluster(struct cluster_handle * clust, struct inode * inode,
- struct page * page, znode_lock_mode mode)
+static int ctail_read_disk_cluster(struct cluster_handle * clust,
+ struct inode * inode, struct page * page,
+ znode_lock_mode mode)
{
int result;