[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [patch v4 12/16] e1000: apply fine lock on e1000
From: |
liu ping fan |
Subject: |
Re: [Qemu-devel] [patch v4 12/16] e1000: apply fine lock on e1000 |
Date: |
Wed, 24 Oct 2012 15:29:44 +0800 |
On Tue, Oct 23, 2012 at 5:04 PM, Jan Kiszka <address@hidden> wrote:
> On 2012-10-22 11:23, Liu Ping Fan wrote:
>> Use local lock to protect e1000. When calling the system function,
>> dropping the fine lock before acquiring the big lock. This will
>> introduce broken device state, which need extra effort to fix.
>>
>> Signed-off-by: Liu Ping Fan <address@hidden>
>> ---
>> hw/e1000.c | 24 +++++++++++++++++++++++-
>> 1 files changed, 23 insertions(+), 1 deletions(-)
>>
>> diff --git a/hw/e1000.c b/hw/e1000.c
>> index ae8a6c5..5eddab5 100644
>> --- a/hw/e1000.c
>> +++ b/hw/e1000.c
>> @@ -85,6 +85,7 @@ typedef struct E1000State_st {
>> NICConf conf;
>> MemoryRegion mmio;
>> MemoryRegion io;
>> + QemuMutex e1000_lock;
>>
>> uint32_t mac_reg[0x8000];
>> uint16_t phy_reg[0x20];
>> @@ -223,13 +224,27 @@ static const uint32_t mac_reg_init[] = {
>> static void
>> set_interrupt_cause(E1000State *s, int index, uint32_t val)
>> {
>> + QemuThread *t;
>> +
>> if (val && (E1000_DEVID >= E1000_DEV_ID_82547EI_MOBILE)) {
>> /* Only for 8257x */
>> val |= E1000_ICR_INT_ASSERTED;
>> }
>> s->mac_reg[ICR] = val;
>> s->mac_reg[ICS] = val;
>> - qemu_set_irq(s->dev.irq[0], (s->mac_reg[IMS] & s->mac_reg[ICR]) != 0);
>> +
>> + t = pthread_getspecific(qemu_thread_key);
>> + if (t->context_type == 1) {
>> + qemu_mutex_unlock(&s->e1000_lock);
>> + qemu_mutex_lock_iothread();
>> + }
>> + if (DEVICE(s)->state < DEV_STATE_STOPPING) {
>> + qemu_set_irq(s->dev.irq[0], (s->mac_reg[IMS] & s->mac_reg[ICR]) !=
>> 0);
>> + }
>> + if (t->context_type == 1) {
>> + qemu_mutex_unlock_iothread();
>> + qemu_mutex_lock(&s->e1000_lock);
>> + }
>
> This is ugly for many reasons. First of all, it is racy as the register
> content may change while dropping the device lock, no? Then you would
> raise or clear an IRQ spuriously.
>
> Second, it clearly shows that we need to address lock-less IRQ delivery.
> Almost nothing is won if we have to take the global lock again to push
> an IRQ event to the guest. I'm repeating myself, but the problem to be
> solved here is almost identical to fast IRQ delivery for assigned
> devices (which we only address pretty ad-hoc for PCI so far).
>
Interesting, could you show me more detail about it, so I can google...
Thanks,
pingfan
> And third: too much boilerplate code... :-/
>
>> }
>>
>> static void
>> @@ -268,6 +283,7 @@ static void e1000_reset(void *opaque)
>> E1000State *d = opaque;
>>
>> qemu_del_timer(d->autoneg_timer);
>> +
>> memset(d->phy_reg, 0, sizeof d->phy_reg);
>> memmove(d->phy_reg, phy_reg_init, sizeof phy_reg_init);
>> memset(d->mac_reg, 0, sizeof d->mac_reg);
>> @@ -448,7 +464,11 @@ e1000_send_packet(E1000State *s, const uint8_t *buf,
>> int size)
>> if (s->phy_reg[PHY_CTRL] & MII_CR_LOOPBACK) {
>> s->nic->nc.info->receive(&s->nic->nc, buf, size);
>> } else {
>> + qemu_mutex_unlock(&s->e1000_lock);
>> + qemu_mutex_lock_iothread();
>> qemu_send_packet(&s->nic->nc, buf, size);
>> + qemu_mutex_unlock_iothread();
>> + qemu_mutex_lock(&s->e1000_lock);
>
> And that is the also a problem to be discussed next: How to handle
> locking of backends? Do we want separate locks for backend and frontend?
> Although they are typically in a 1:1 relationship? Oh, I'm revealing the
> content of my talk... ;)
>
>> }
>> }
>>
>> @@ -1221,6 +1241,8 @@ static int pci_e1000_init(PCIDevice *pci_dev)
>> int i;
>> uint8_t *macaddr;
>>
>> + qemu_mutex_init(&d->e1000_lock);
>> +
>> pci_conf = d->dev.config;
>>
>> /* TODO: RST# value should be 0, PCI spec 6.2.4 */
>>
>
> Jan
>
> --
> Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
> Corporate Competence Center Embedded Linux
>
- Re: [Qemu-devel] [patch v4 12/16] e1000: apply fine lock on e1000, (continued)
- Re: [Qemu-devel] [patch v4 12/16] e1000: apply fine lock on e1000, Jan Kiszka, 2012/10/23
- Re: [Qemu-devel] [patch v4 12/16] e1000: apply fine lock on e1000, liu ping fan, 2012/10/24
- Re: [Qemu-devel] [patch v4 12/16] e1000: apply fine lock on e1000, Jan Kiszka, 2012/10/24
- Re: [Qemu-devel] [patch v4 12/16] e1000: apply fine lock on e1000, Avi Kivity, 2012/10/25
- Re: [Qemu-devel] [patch v4 12/16] e1000: apply fine lock on e1000, Jan Kiszka, 2012/10/25
- Re: [Qemu-devel] [patch v4 12/16] e1000: apply fine lock on e1000, Avi Kivity, 2012/10/25
- Re: [Qemu-devel] [patch v4 12/16] e1000: apply fine lock on e1000, Jan Kiszka, 2012/10/25
- Re: [Qemu-devel] [patch v4 12/16] e1000: apply fine lock on e1000, Avi Kivity, 2012/10/25
- Re: [Qemu-devel] [patch v4 12/16] e1000: apply fine lock on e1000, Jan Kiszka, 2012/10/25
- Re: [Qemu-devel] [patch v4 12/16] e1000: apply fine lock on e1000, liu ping fan, 2012/10/29
- Re: [Qemu-devel] [patch v4 12/16] e1000: apply fine lock on e1000,
liu ping fan <=
- Re: [Qemu-devel] [patch v4 12/16] e1000: apply fine lock on e1000, Jan Kiszka, 2012/10/25
- Re: [Qemu-devel] [patch v4 12/16] e1000: apply fine lock on e1000, Avi Kivity, 2012/10/25
- Re: [Qemu-devel] [patch v4 12/16] e1000: apply fine lock on e1000, Jan Kiszka, 2012/10/25
- Re: [Qemu-devel] [patch v4 12/16] e1000: apply fine lock on e1000, Avi Kivity, 2012/10/25
- Re: [Qemu-devel] [patch v4 12/16] e1000: apply fine lock on e1000, liu ping fan, 2012/10/29
- Re: [Qemu-devel] [patch v4 12/16] e1000: apply fine lock on e1000, Jan Kiszka, 2012/10/31
[Qemu-devel] [patch v4 03/16] hotplug: introduce qdev_unplug_complete() to remove device from views, Liu Ping Fan, 2012/10/22
[Qemu-devel] [patch v4 01/16] atomic: introduce atomic operations, Liu Ping Fan, 2012/10/22
[Qemu-devel] [patch v4 13/16] e1000: add busy flag to anti broken device state, Liu Ping Fan, 2012/10/22