Messages in this thread | | | Date | Thu, 01 Sep 2011 16:56:49 +0800 | From | Huang Ying <> | Subject | Re: [PATCH -mm 1/2] irq_work, Use llist in irq_work |
| |
On 09/01/2011 03:58 PM, Peter Zijlstra wrote: > On Thu, 2011-09-01 at 11:20 +0800, Huang Ying wrote: >> On 09/01/2011 09:46 AM, Huang Ying wrote: >>>>> -static void __irq_work_queue(struct irq_work *entry) >>>>> +static void __irq_work_queue(struct irq_work *work) >>>>> { >>>>> - struct irq_work *next; >>>>> + struct irq_work_list *irq_work_list; >>>>> >>>>> - preempt_disable(); >>>>> + irq_work_list = &get_cpu_var(irq_work_lists); >>>>> >>>>> - do { >>>>> - next = __this_cpu_read(irq_work_list); >>>>> - /* Can assign non-atomic because we keep the flags set. */ >>>>> - entry->next = next_flags(next, IRQ_WORK_FLAGS); >>>>> - } while (this_cpu_cmpxchg(irq_work_list, next, entry) != next); >>>>> + llist_add(&work->llnode, &irq_work_list->llist); >>>>> >>>>> /* The list was empty, raise self-interrupt to start processing. */ >>>>> - if (!irq_work_next(entry)) >>>>> + if (!test_and_set_bit(LIST_NONEMPTY_BIT, &irq_work_list->flags)) >>>>> arch_irq_work_raise(); >>>> >>>> So why can't you simply test work->llnode->next? >>> >>> Yes. That is better. Even if there may be a small race window, it is >>> not a big issue to raise one extra self interrupt seldom. >> >> Remember something about this. I didn't test work->llnode->next here >> because I didn't want expose the implementation details like that here. >> How about make llist_add() return whether list is empty before adding? >> Because it will be an inline function, that should be optimized out if >> the caller do not need the information. > > You could also use llist_empty() although that brings me to that > ACCESS_ONCE thing in there, what's the point?
Something as follow with llist_empty() seems not work.
empty = llist_empty(irq_work_list); llist_add(&work->llnode, irq_work_list); if (empty) arch_irq_work_raise();
Because irq_work IRQ handler or timer IRQ handler may be executed just before "llist_add(&work->llnode, irq_work_list)", so that, although "empty == false", arch_irq_work_raise() still should be executed.
Can you tell me how to that with llist_empty()?
For ACCESS_ONCE, Mathiew suggest me to add it,
Hi, Mathiew,
Can you explain why ACCESS_ONCE should be used here?
Best Regards, Huang Ying
| |