Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] KVM: LAPIC: Also cancel preemption timer when disarm
 LAPIC timer
To:     Wanpeng Li <kernellwp@gmail.com>
Cc:     LKML <linux-kernel@vger.kernel.org>, kvm <kvm@vger.kernel.org>,
        Sean Christopherson <sean.j.christopherson@intel.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Joerg Roedel <joro@8bytes.org>
References: <1585031530-19823-1-git-send-email-wanpengli@tencent.com>
 <708f1914-be5e-91a0-2fdf-8d34b78ca7da@redhat.com>
 <CANRm+CwGT4oU_CcpRcDhS992htXdP4rcO6QqkA1CyryUJbE6Ug@mail.gmail.com>
From:   Paolo Bonzini <pbonzini@redhat.com>
Message-ID: <5582f554-5ae9-4cb9-d8e9-f1ff57fca35c@redhat.com>
Date:   Thu, 26 Mar 2020 01:28:03 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.5.0
MIME-Version: 1.0
In-Reply-To: <CANRm+CwGT4oU_CcpRcDhS992htXdP4rcO6QqkA1CyryUJbE6Ug@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 26/03/20 01:20, Wanpeng Li wrote:
>> There are a few other occurrences of hrtimer_cancel, and all of them
>> probably have a similar issue.  What about adding a cancel_apic_timer
> Other places are a little different, here we just disarm the timer,
> other places we will restart the timer just after the disarm except
> the vCPU reset (fixed in commit 95c065400a1 (KVM: VMX: Stop the
> preemption timer during vCPU reset)), the restart will override
> vmx->hv_deadline_tsc. What do you think? I can do it if introduce
> cancel_apic_timer() is still better.

At least start_apic_timer() would benefit from adding hrtimer_cancel(),
removing it from kvm_set_lapic_tscdeadline_msr and kvm_lapic_reg_write.
 But you're right that it doesn't benefit from a cancel_apic_timer(),
because ultimately they either update the preemption timer or cancel it
in start_sw_timer.  So I'll apply your patch and send a cleanup myself
for start_apic_timer.

Thanks,

Paolo