Do not deprecate binary semaphore or do allow mutex in software interrupt contexts

Do not deprecate binary semaphore or do allow mutex in software interrupt contexts

[Matti Linnanvuori]

I thought of a scenario where it seems appropriate to use a binary semaphore or a mutex in a software interrupt context. If a device cannot interrupt when some important variable changes, it can be polled occasionally to update e.g. LEDs to indicate status. Such polling can be done most efficiently in timers that are software interrupts. Timers are more efficient than works because they do not have so much context switching overhead. If the access to the variables of the device must be serialized, a binary semaphore or a mutex is a natural choice. If user-space writing to the device is likely to change the status, it can make sense not to poll the status of the device at the same time. The timer could therefore sensibly call mutex_trylock. Therefore, it seems wrong to me to deprecate binary semaphores and disallow the use of mutexes in software interrupt contexts.




      __________________________________  
Yahoo! Clever: Sie haben Fragen? Yahoo! Nutzer antworten Ihnen. www.yahoo.de/clever

Re: Do not deprecate binary semaphore or do allow mutex in software interrupt contexts

[Alan Cox]

> to be honest, the scenario describe really smells of broken locking, in
> fact it really sounds like it wants to use spinlocks instead 

For polling and timer based code its often simpler to do

	del_timer_sync(&my_timer);
	FrobStuff
	add_timer(&my_timer);

especially if "FrobStuff" is likely to change when you next need to poll.

And don't forget regular polling is a good way to really annoy laptop
users as it burns power

Alan

Re: Do not deprecate binary semaphore or do allow mutex in software interrupt contexts

[Arjan van de Ven]

On Tue, 11 Sep 2007 06:20:51 -0700 (PDT)
Matti Linnanvuori <mattilinnanvuori@yahoo.com> wrote:

Hi,

> I thought of a scenario where it seems appropriate to use a binary
> semaphore or a mutex in a software interrupt context. If a device
> cannot interrupt when some important variable changes, it can be
> polled occasionally to update e.g. LEDs to indicate status. Such
> polling can be done most efficiently in timers that are software
> interrupts. Timers are more efficient than works because they do not
> have so much context switching overhead. If the access to the
> variables of the device must be serialized, a binary semaphore or a
> mutex is a natural choice. If user-space writing to the device is
> likely to change the status, it can make sense not to poll the status
> of the device at the same time. The timer could therefore sensibly
> call mutex_trylock.

what do you do if the trylock fails?

> Therefore, it seems wrong to me to deprecate
> binary semaphores and disallow the use of mutexes in software
> interrupt contexts.

to be honest, the scenario describe really smells of broken locking, in
fact it really sounds like it wants to use spinlocks instead 

Greetings,
   Arjan van de Ven

Do not deprecate binary semaphore or do allow mutex in software interrupt contexts

[Matti Linnanvuori]

 Arjan van de Ven:
> what do you do if the trylock fails?

Just do not read the status variable now but modify the timer to run later.

> to be honest, the scenario describe really smells of broken locking, in
> fact it really sounds like it wants to use spinlocks instead 

No, I don't think it is broken.
Spinlocks can be used, but I don't see them being obviously better in all cases.
If access takes a long time, it is better to sleep during it.
And if you sleep, you might just end up creating a new mutex implementation with a spinlock.

Alan Cox:
> For polling and timer based code its often simpler to do
>
>    del_timer_sync(&my_timer);
>    FrobStuff
>    add_timer(&my_timer);
>
> especially if "FrobStuff" is likely to change when you next need to poll.

In the scenario I presented, the timer modifies itself to run later.
Therefore, simply calling del_timer_sync is not enough but you have to set an atomic variable to prevent the timer from adding itself again.
Again, you end up creating a new mutex implementation, which is not good.




       __________________________________ 
Yahoo! Clever - Der einfachste Weg, Fragen zu stellen und Wissenswertes mit Anderen zu teilen. www.yahoo.de/clever

Re: Do not deprecate binary semaphore or do allow mutex in software interrupt contexts

[Peter Zijlstra]

[re-adding CCs, please do not drop these]

On Tue, 2007-09-11 at 09:20 -0700, Matti Linnanvuori wrote:
> Arjan van de Ven:
> > what do you do if the trylock fails?
> 
> Just do not read the status variable now but modify the timer to run later.
> 
> > to be honest, the scenario describe really smells of broken locking, in
> > fact it really sounds like it wants to use spinlocks instead 
> 
> No, I don't think it is broken.

Yes it is.

> Spinlocks can be used, but I don't see them being obviously better in all cases.
> If access takes a long time, it is better to sleep during it.
> And if you sleep, you might just end up creating a new mutex
> implementation with a spinlock.

If you have to wait a long time in an atomic context you've done
something wrong. If you're only reading it from an atomic context you
might consider storing a copy that can be quickly updated and protect
that using a spinlock.

do_update ()
{
  mutex_lock(&my_device_mutex);
  my_device_frob_state(&my_state); /* <-- this takes a _long_ while */
  spin_lock_irq(&my_shadow_state_lock);
  my_shadow_state = state; /* <-- this is a quick memcopy */
  spin_unlock_irq(&my_shadow_state_lock);
  mutex_unlock(&my_device_mutex);
}

do_read()
{
  spin_lock_irq(&my_shadow_state);
  do_something_with_shadow_state(&mt_shadow_state);
  spin_unlock_irq(&my_shadow_state);

  return foo;
}

> Alan Cox:
> > For polling and timer based code its often simpler to do
> >
> >    del_timer_sync(&my_timer);
> >    FrobStuff
> >    add_timer(&my_timer);
> >
> > especially if "FrobStuff" is likely to change when you next need to poll.
> 
> In the scenario I presented, the timer modifies itself to run later.
> Therefore, simply calling del_timer_sync is not enough but you have to
> set an atomic variable to prevent the timer from adding itself again.

Not being too familiar with the timer stuff, it smells wrong what you
say.

As for the whole polling method, consider what Alan said, don't do it if
you don't need to. You'll annoy people at no end. Try to push state
changes where possible.

Do not deprecate binary semaphore or do allow mutex in software interrupt contexts

[Matti Linnanvuori]

Peter Zijlstra <peterz@infradead.org>:
> Yes it is.

Why do you think it is broken?

> If you have to wait a long time in an atomic context you've done
> something wrong.

I saw an implementation where there were two atomic contexts, one to initiate reading and another to complete the reading. 
That way, there was no busy wait for a long time in an atomic context.

> If you're only reading it from an atomic context you
> might consider storing a copy that can be quickly updated and protect
> that using a spinlock.

You suggested that a user-space task read from the device. 
But that includes more context switching and therefore consumes more resources than reading just from an atomic context.

> Not being too familiar with the timer stuff, it smells wrong what you
> say.

Why?




      Wissenswertes für Bastler und Hobby Handwerker. BE A BETTER HEIMWERKER! www.yahoo.de/clever

Re: Do not deprecate binary semaphore or do allow mutex in software interrupt contexts

[Arjan van de Ven]

On Tue, 11 Sep 2007 09:20:23 -0700 (PDT)
Matti Linnanvuori <mattilinnanvuori@yahoo.com> wrote:

>  Arjan van de Ven:
> > what do you do if the trylock fails?
> 
> Just do not read the status variable now but modify the timer to run
> later.
> 
> > to be honest, the scenario describe really smells of broken
> > locking, in fact it really sounds like it wants to use spinlocks
> > instead 
> 
> No, I don't think it is broken.
> Spinlocks can be used, but I don't see them being obviously better in
> all cases. If access takes a long time, it is better to sleep during
> it. And if you sleep, you might just end up creating a new mutex
> implementation with a spinlock.


at this point the discussion has gone so theoretical that I think it's
better to go with a real example. What actual source code do you think
is a legit case for this?

I still think that whatever case you have in mind is better served with
something else, but until we see the actual complete drier we're both
talking air.

Re: Do not deprecate binary semaphore or do allow mutex in software interrupt contexts

[Arjan van de Ven]

On Tue, 11 Sep 2007 10:29:19 -0700 (PDT)
Matti Linnanvuori <mattilinnanvuori@yahoo.com> wrote:

> > Not being too familiar with the timer stuff, it smells wrong what
> > you say.
> 
> Why?

timers and sleeping locks really shouldn't be mixed.
It ends up in general as more complex, fragile and weird...

But feel free to prove us otherwise with real code...

Do not deprecate binary semaphore or do allow mutex in software interrupt contexts

[Matti Linnanvuori]

The following code seems to me to be a valid example of a binary semaphore (mutex) in a timer:

//timer called 10 times a second 
static void status_timer(unsigned long device)
{
    struct etp_device_private *dp = (struct etp_device_private *)device;
    if (unlikely(dp->status_interface == 0))
        dp->status_interface = INTERFACES_PER_DEVICE - 1;
    else
        dp->status_interface--;
    //DBG_PRINT ("%s: In status timer, interface:0x%x.\n",etp_NAME, dp->status_interface);
    idt_los_interrupt_1(dp, dp->status_interface);
    if (likely(!dp->reset))
        // reset the timer:
        mod_timer(&dp->status_timer, jiffies + HZ / 10);
}

static inline void read_idt_register_interrupt(struct etp_device_private *dp,
                           unsigned reg)
{
    DBG_PRINT("read_idt_register_interrupt to mutex_lock.\n");
    if (unlikely(down_trylock(&dp->semaphore)))
        return;        /* Do not read because failed to lock. */
    if (likely
        (!dp->status
         && !(inl((void *)(dp->ioaddr + REG_E1_CTRL)) & E1_ACCESS_ON))) {
        outl(((reg << E1_REGISTER_SHIFT) & E1_REGISTER_MASK)
             | E1_DIR_READ | E1_ACCESS_ON,
             (void *)(dp->ioaddr + REG_E1_CTRL));
        dp->status = 1;
        DBG_PRINT("read_idt_register_interrupt set status read.\n");
    } else
        DBG_PRINT
            ("read_idt_register_interrupt did not set status %u read.\n",
             dp->status);
    DBG_PRINT
        ("read_idt_register_interrupt do not wait for result here, read in tasklet.\n");
}


//for getting los information with interrupt:

void idt_los_interrupt_1(struct etp_device_private *dp, unsigned interface)

{

    read_idt_register_if_interrupt(dp, E1_TRNCVR_LINE_STATUS0_REG,

                       interface);

}


static void e1_access_task(unsigned long data)    //called after e1_access_interrupt
{
    struct etp_device_private *dp = (struct etp_device_private *)data;
    struct etp_interface_private *ip;
    unsigned int interface, error;
    bool los;

    //check if los status was read:
    if (unlikely(!dp->status)) {
        DBG_PRINT("e1_access_task wakes up user.\n");
        wake_up(&dp->e1_access_q);
        return;
    }
    error =
        idt_los_interrupt_2(dp->ioaddr, &interface, &los,
                dp->pci_dev->device);
    //DBG_PRINT ("%s: In e1 task, error:0x%x, interface:0x%x, los:0x%x.\n",
    //         etp_NAME, error, interface, los);
    dp->status = 0;
    up(&dp->semaphore);
    DBG_PRINT("e1_access_task got error %u.\n", error);
    if (unlikely(error))
        return;
    //update los status:
    ip = &dp->interface_privates[interface];
    ip->los = los;
    //update status:
    if ((ip->if_mode == IF_MODE_CLOSED) ||    //interface closed or
        (ip->los)) {    //link down 
        set_led(LED_CTRL_OFF, ip);
        if (netif_carrier_ok(ip->ch_priv.this_netdev))
            netif_carrier_off(ip->ch_priv.this_netdev);
    } else {        //link up and interface opened
        if (!netif_carrier_ok(ip->ch_priv.this_netdev))
            netif_carrier_on(ip->ch_priv.this_netdev);
        if (ip->if_mode == IF_MODE_HDLC) {
            set_led(LED_CTRL_TRAFFIC, ip);
        } else {    //ip->if_mode == IF_MODE_TIMESLOT
            set_led(LED_CTRL_ON, ip);
        }
    }
}

int idt_los_interrupt_2(u8 * ioaddr, unsigned *interface, bool * los,
            unsigned pci_device_id)
{                //returns 0 in success
    unsigned int value = inl((void *)(ioaddr + REG_E1_CTRL));
    //if access not ended:
    if (value & E1_ACCESS_ON) {
        return 1;
    }
    //if access not to los status register 
    if ((value & E1_REGISTER_MASK_NO_IF) !=
        (E1_TRNCVR_LINE_STATUS0_REG << E1_REGISTER_SHIFT)) {
        return 1;
    }
    //get interface
    *interface =
        idt_if_to_if((value & E1_REGISTER_MASK_IF) >>
             E1_REGISTER_SHIFT_IF, pci_device_id);
    *los = value & 0x1;
    return 0;
}

int write_idt_register_lock(unsigned device, unsigned reg, u32 value)
{
    struct etp_device_private *etp = get_dev_priv(device);
    unsigned ctrl;
    DBG_PRINT("write_idt_register_lock to mutex lock device %u.\n", device);
    down(&etp->semaphore);
    if (unlikely(etp->reset)) {
        up(&etp->semaphore);
        DBG_PRINT
            ("write_idt_register_lock device %u unusable.\n", device);
        return -ENXIO;
    }
    DBG_PRINT("write_idt_register_lock mutex locked device %u.\n", device);
    do {
        DBG_PRINT
            ("write_idt_register_lock to wait_event_timeout device %u.\n",
             device);
        wait_event_timeout(etp->e1_access_q,
                   !((ctrl =
                      inl((void *)(etp->ioaddr + REG_E1_CTRL)))
                     & E1_ACCESS_ON), HZ / 500);
    }
    while (ctrl & E1_ACCESS_ON);
    DBG_PRINT("write_idt_register_lock to outl device %u.\n", device);
    outl(((reg << E1_REGISTER_SHIFT) & E1_REGISTER_MASK) |
         E1_DIR_WRITE | E1_ACCESS_ON | (value & E1_DATA_MASK),
         (void *)(etp->ioaddr + REG_E1_CTRL));
    up(&etp->semaphore);
    DBG_PRINT("write_idt_register_lock mutex unlocked device %u.\n",
          device);
    return 0;
}
        Copyright (C) 2006 Jouni Kujala, Flexibilis Oy.




      __________________________________  
Alles was der Gesundheit und Entspannung dient. BE A BETTER MEDIZINMANN! www.yahoo.de/clever