On Mon, Oct 22, 2007 at 10:02:59PM +0400, Oleg Nesterov wrote:
...
> If this work doesn't rearm itself - yes. (otherwise, the same ->func
> can run twice _at the same time_)
>
> But again, in this case wait_on_work() after try_to_grab_pending() == 1
> doesn't block, so we can just do
>
> if
On Mon, Oct 22, 2007 at 10:02:59PM +0400, Oleg Nesterov wrote:
> On 10/22, Jarek Poplawski wrote:
...
> > OK, I know I'm dumber and dumber everyday,
>
> You are not alone. I have the same feeling about myself!
Feeling is not the same, only true knowledge counts!
>
> > these all flushes are
> >
On 10/22, Jarek Poplawski wrote:
>
> On Fri, Oct 19, 2007 at 09:50:14AM +0200, Jarek Poplawski wrote:
> > On Thu, Oct 18, 2007 at 07:48:19PM +0400, Oleg Nesterov wrote:
> > > On 10/18, Jarek Poplawski wrote:
> > > >
> > > > +/**
> > > > + * flush_work_sync - block until a work_struct's callback has
On Fri, Oct 19, 2007 at 09:50:14AM +0200, Jarek Poplawski wrote:
> On Thu, Oct 18, 2007 at 07:48:19PM +0400, Oleg Nesterov wrote:
> > On 10/18, Jarek Poplawski wrote:
> > >
> > > +/**
> > > + * flush_work_sync - block until a work_struct's callback has terminated
> > ^^^
> Actually I'm not convinced with this explanation. It seems to me that
> since there are such serious locking problems (especially with rntl),
> there could be once more considered a private workqueue. You've
> written earlier about being a lonely user of this code. But, since
> Benjamin offered
On Fri, 19 Oct 2007, Jarek Poplawski wrote:
> Actually I'm not convinced with this explanation. It seems to me that
> since there are such serious locking problems (especially with rntl),
> there could be once more considered a private workqueue. You've
> written earlier about being a lonely user
On Fri, Oct 19, 2007 at 12:38:29PM +0100, Maciej W. Rozycki wrote:
> On Thu, 18 Oct 2007, Maciej W. Rozycki wrote:
>
> > > 1) phy_change() checks PHY_HALTED flag without lock; I think it's
> > > racy: eg. if it's done during phy_stop() it can check just before
> > > the flag is set and reenable in
On Fri, 19 Oct 2007, Jarek Poplawski wrote:
> But then... your patch seems to make it possible, because it enables
> irq to the initial state of the counter. Of course, this could happen
> on closing only.
That's because free_irq() does not disable the interrupt in the correct
manner. The scen
On Thu, 18 Oct 2007, Maciej W. Rozycki wrote:
> > 1) phy_change() checks PHY_HALTED flag without lock; I think it's
> > racy: eg. if it's done during phy_stop() it can check just before
> > the flag is set and reenable interrupts just after phy_stop() ends.
>
> I remember having a look into it,
On Thu, Oct 18, 2007 at 12:30:35PM +0100, Maciej W. Rozycki wrote:
> On Wed, 17 Oct 2007, Jarek Poplawski wrote:
...
> > 2) phy_change() doesn't reenable irq line after it sees returns
> > with errors; IMHO it should at least write some warning, but maybe
> > try some safety plan, so enable_irq() a
On Thu, 2007-10-18 at 19:48 +0400, Oleg Nesterov wrote:
> > +void flush_work_sync(struct work_struct *work)
> If we really the new helper, perhaps we can make it a bit better?
>
> 1. Modify insert_work() to take the "struct list_head *at" parameter instead
>of "int tail". I think this patch
On Fri, Oct 19, 2007 at 09:50:14AM +0200, Jarek Poplawski wrote:
...
> sched_work_sync() with rtnl_lock(). It's only less probable to lockup
> with this than with flush_schedule_work().
...But, not much less...
Jarek P.
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the bo
On Thu, Oct 18, 2007 at 07:48:19PM +0400, Oleg Nesterov wrote:
> On 10/18, Jarek Poplawski wrote:
> >
> > +/**
> > + * flush_work_sync - block until a work_struct's callback has terminated
> ^^^
> Hmm...
>
> > + * Similar to c
On Thu, 18 Oct 2007, Oleg Nesterov wrote:
> If we can't just cancel the work, can't we do something like
>
> if (cancel_work_sync(w))
> w->func(w);
>
> instead?
We do an equivalent of this -- all that we care about that w->func(w)
would do is enable_irq() and the rest we w
On 10/18, Jarek Poplawski wrote:
>
> +/**
> + * flush_work_sync - block until a work_struct's callback has terminated
^^^
Hmm...
> + * Similar to cancel_work_sync() but will only busy wait (without cancel)
> + * if the work is
On Thu, 18 Oct 2007, Jarek Poplawski wrote:
> Technically until free_irq returns a handler should respond to calls
> and with proper hardware it should have no problem with checking if
> it's its interrupt, even after disabling this hardware, because of
> possible races.
Well, the hardirq handle
On Thu, Oct 18, 2007 at 12:30:35PM +0100, Maciej W. Rozycki wrote:
> On Wed, 17 Oct 2007, Jarek Poplawski wrote:
>
> > I'm not sure free_irq() should maintain the depth count - rather warn
> > on not zero. But, IMHO, any activity on freed irq seems suspicious to
> > me (and doesn't look like very
On Thu, 18 Oct 2007, Jarek Poplawski wrote:
> After rethinking, it looks like this last cancel should be useless.
> So, if phy_interrupt() schedules only if !PHY_HALTED and phy_change()
> does enable_irq() with no exeptions, it seems phy_interrupt() even
> without lock must see PHY_HALTED state be
On Wed, 17 Oct 2007, Jarek Poplawski wrote:
> I'm not sure free_irq() should maintain the depth count - rather warn
> on not zero. But, IMHO, any activity on freed irq seems suspicious to
> me (and doesn't look like very common), even if it's safe with current
> implementation.
No way to avoid i
After reading this and earlier threads about phylib's way of using
workqueue I think such a lighter and safer wrt. locking alternative
for flush_scheduled_work should be useful, but maybe it's only my
imagination.
So, let's ask Oleg Nesterov, whose solutions are here only
copy-cut-pasted & possibl
On Wed, Oct 17, 2007 at 10:58:09AM +0200, Jarek Poplawski wrote:
...
> 8) phy_stop_interrupts(): I'm not sure this additional call from
> DEBUG_SHIRQ should be so dangerous, eg.:
>
> /*
>* status == PHY_HALTED &&
>* interrupts are stopped after phy_stop()
>*/
>
> Btw., since, because of this patch, I've had a one more look at phy.c
> and there are a few more doubts, so this time I'll try to bother you
> for real:
.../...
While there... is somebody interested in making the whole PHY lib
operate a task level and use mutexes instead of spinlock ? I need
On Wed, Oct 17, 2007 at 10:58:09AM +0200, Jarek Poplawski wrote:
...
> 5) phy_stop_interrupts(): maybe I miss something, but it seems
> phy_stop() is required before this, so maybe there should be a
> comment on this?
>
> 6) phy_stop_interrupts(): if I'm not wrong with #3 calling
Should be:
6)
On Tue, Oct 16, 2007 at 06:19:32PM +0100, Maciej W. Rozycki wrote:
...
> Well, enable_irq() and disable_irq() themselves are nesting, so they are
> not a problem. OTOH, free_irq() does not seem to maintain the depth count
> correctly, which looks like a bug to me and which could trigger regardl
On Tue, 16 Oct 2007, Jarek Poplawski wrote:
> Yes, it's all right here. Sorry for bothering - I should've found this
> by myself.
Ah, no problem -- even with the right keys you may sometimes get lost in
the results.
> I've still some doubts about this possible enable_irq() after
> free_irq().
On Mon, Oct 15, 2007 at 06:03:20PM +0100, Maciej W. Rozycki wrote:
> On Mon, 15 Oct 2007, Jarek Poplawski wrote:
>
> > Could you explain why cancel_work_sync() is better here than
> > flush_scheduled_work() wrt. rtnl_lock()?
>
> Well, this is actually the bit that made cancel_work_sync() be writ
On Mon, 15 Oct 2007, Jarek Poplawski wrote:
> Could you explain why cancel_work_sync() is better here than
> flush_scheduled_work() wrt. rtnl_lock()?
Well, this is actually the bit that made cancel_work_sync() be written in
the first place. The short story is the netlink lock is most probably
On 19-09-2007 16:38, Maciej W. Rozycki wrote:
...
> @@ -661,13 +664,22 @@ int phy_stop_interrupts(struct phy_devic
> if (err)
> phy_error(phydev);
>
> + free_irq(phydev->irq, phydev);
> +
> /*
> - * Finish any pending work; we might have been scheduled to be cal
On Fri, 21 Sep 2007 13:51:12 +0100 (BST)
"Maciej W. Rozycki" <[EMAIL PROTECTED]> wrote:
> On Thu, 20 Sep 2007, Andrew Morton wrote:
>
> > You always put boring, crappy, insufficient text in the for-the-changelog
> > section and interesting, useful, sufficient text in the
> > not-for-the-changelo
On Thu, 20 Sep 2007, Andrew Morton wrote:
> You always put boring, crappy, insufficient text in the for-the-changelog
> section and interesting, useful, sufficient text in the not-for-the-changelog
> section.
I'll swap the sections in the future then. ;-) Frankly I was not sure
whether the cha
On Wed, 19 Sep 2007 15:38:19 +0100 (BST)
"Maciej W. Rozycki" <[EMAIL PROTECTED]> wrote:
> Keep track of disable_irq_nosync() invocations and call enable_irq() the
> right number of times if work has been cancelled that would include them.
>
> Signed-off-by: Maciej W. Rozycki <[EMAIL PROTECTED]>
Keep track of disable_irq_nosync() invocations and call enable_irq() the
right number of times if work has been cancelled that would include them.
Signed-off-by: Maciej W. Rozycki <[EMAIL PROTECTED]>
---
Now that the call to flush_work_keventd() (problematic because of
rtnl_mutex being held) h
32 matches
Mail list logo
