| Message ID | 1463382133.19947.10.camel@TP420 |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, May 16, 2016 at 03:02:13PM +0800, Li Zhong wrote: > It seems that we don't need to take the lock before evaluating irq's > resend flag. What needed is to make sure when we clear the ics's bit > in the icp's resend_map, we don't miss the resend flag of the irqs > that set the bit. > > And seems this could be ordered through the barrier in test_and_clear_bit(), > and an newly added wmb when setting irq's resend flag, and icp's resend_map. This looks fine to me. Is there a measurable performance improvement from this? I understand it could be hard to measure. Also, you could make the patch description more definite - just say that we don't need to take the lock, there's no need for "seems". Paul. -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Jun 20, 2016, at 13:27, Paul Mackerras <paulus@ozlabs.org> wrote: > > On Mon, May 16, 2016 at 03:02:13PM +0800, Li Zhong wrote: >> It seems that we don't need to take the lock before evaluating irq's >> resend flag. What needed is to make sure when we clear the ics's bit >> in the icp's resend_map, we don't miss the resend flag of the irqs >> that set the bit. >> >> And seems this could be ordered through the barrier in test_and_clear_bit(), >> and an newly added wmb when setting irq's resend flag, and icp's resend_map. > > This looks fine to me. Is there a measurable performance improvement > from this? I understand it could be hard to measure. > > Also, you could make the patch description more definite - just say > that we don't need to take the lock, there's no need for "seems”. OK :) However, we may need to ignore this one for now. To implement the P/Q stuff, we probably need make sure the resend irqs to be resent only once. It’s easier to make sure that with the lock here, and the resend flag can be cleared inside the lock. Thanks, Zhong > > Paul. > _______________________________________________ > Linuxppc-dev mailing list > Linuxppc-dev@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/linuxppc-dev -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/powerpc/kvm/book3s_hv_rm_xics.c b/arch/powerpc/kvm/book3s_hv_rm_xics.c index c3f604e..c3fa386 100644 --- a/arch/powerpc/kvm/book3s_hv_rm_xics.c +++ b/arch/powerpc/kvm/book3s_hv_rm_xics.c @@ -39,14 +39,9 @@ static void ics_rm_check_resend(struct kvmppc_xics *xics, for (i = 0; i < KVMPPC_XICS_IRQ_PER_ICS; i++) { struct ics_irq_state *state = &ics->irq_state[i]; - arch_spin_lock(&state->lock); - - if (!state->resend) { - arch_spin_unlock(&state->lock); + if (!state->resend) continue; - } - arch_spin_unlock(&state->lock); icp_rm_deliver_irq(xics, icp, state->number); } } @@ -374,8 +369,13 @@ static void icp_rm_deliver_irq(struct kvmppc_xics *xics, struct kvmppc_icp *icp, * We failed to deliver the interrupt we need to set the * resend map bit and mark the ICS state as needing a resend */ - set_bit(ics->icsid, icp->resend_map); state->resend = 1; + /* + * Make sure when checking resend, we don't miss the resend if + * resend_map bit is seen and cleared. + */ + smp_wmb(); + set_bit(ics->icsid, icp->resend_map); /* * If the need_resend flag got cleared in the ICP some time diff --git a/arch/powerpc/kvm/book3s_xics.c b/arch/powerpc/kvm/book3s_xics.c index b9dbf75..420c4fc 100644 --- a/arch/powerpc/kvm/book3s_xics.c +++ b/arch/powerpc/kvm/book3s_xics.c @@ -110,30 +110,17 @@ static void ics_check_resend(struct kvmppc_xics *xics, struct kvmppc_ics *ics, { int i; - unsigned long flags; - - local_irq_save(flags); - for (i = 0; i < KVMPPC_XICS_IRQ_PER_ICS; i++) { struct ics_irq_state *state = &ics->irq_state[i]; - arch_spin_lock(&state->lock); - - if (!state->resend) { - arch_spin_unlock(&state->lock); + if (!state->resend) continue; - } XICS_DBG("resend %#x prio %#x\n", state->number, state->priority); - arch_spin_unlock(&state->lock); - local_irq_restore(flags); icp_deliver_irq(xics, icp, state->number); - local_irq_save(flags); } - - local_irq_restore(flags); } static bool write_xive(struct kvmppc_xics *xics, struct kvmppc_ics *ics, @@ -474,8 +461,13 @@ static void icp_deliver_irq(struct kvmppc_xics *xics, struct kvmppc_icp *icp, * We failed to deliver the interrupt we need to set the * resend map bit and mark the ICS state as needing a resend */ - set_bit(ics->icsid, icp->resend_map); state->resend = 1; + /* + * Make sure when checking resend, we don't miss the resend if + * resend_map bit is seen and cleared. + */ + smp_wmb(); + set_bit(ics->icsid, icp->resend_map); /* * If the need_resend flag got cleared in the ICP some time
It seems that we don't need to take the lock before evaluating irq's resend flag. What needed is to make sure when we clear the ics's bit in the icp's resend_map, we don't miss the resend flag of the irqs that set the bit. And seems this could be ordered through the barrier in test_and_clear_bit(), and an newly added wmb when setting irq's resend flag, and icp's resend_map. Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com> --- arch/powerpc/kvm/book3s_hv_rm_xics.c | 14 +++++++------- arch/powerpc/kvm/book3s_xics.c | 22 +++++++--------------- 2 files changed, 14 insertions(+), 22 deletions(-)