[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2] x86/vpt: Do not take pt_migrate rwlock in some cases


  • To: Jan Beulich <jbeulich@xxxxxxxx>, roger.pau@xxxxxxxxxx
  • From: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
  • Date: Mon, 29 Mar 2021 11:04:37 -0400
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=oracle.com; dmarc=pass action=none header.from=oracle.com; dkim=pass header.d=oracle.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=YcjmxpoYNxxuOCKR2CDKfVORzpyWwUflamuix4CeWVU=; b=OykIHJAy/tbzN2mzwxeGnrp4ft4RfUiNwOdZVgtKaVBnDT/IGjwJ0peCdviHFw3s/nhkP8FpEMi+aaWve5JjLIHlRY19Om4iCMFBjWKe7iYundEvaTMxVktq+zhqdE3NT2qg/sMHz1jYTOVk4JvmJ4BwLwp4k22AbIdzHk4pt8SYBmhms1MnBM9B0HyWmhgLAbK7JQgp3xIFRsbp1qGkEg6bOb+6+iZyMCxpyWs5VdizflpOiCKBCdxxomXNLBVIn62TJfcDqk1EfyqAAyzyiyoCqJLCcUj3+JX10OdCwNujFvVScZJNR+aJeyXYW9wjvyYq0cy2aPKl0Crtdbn8qQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=odsUTMPBaHNkkha8p0cBUpDJK2uDHxEwSjeeK+f2K9WoeNutvPuBaozaZ1QfhOKph3SL2AlXFR+E4H91ffFYVT4Sj8fM/4PAmmvyI63ZI7A5dkG9NP6YQrjbxHit41BcctxrTCF7zobATKNUA9TBWIf/7Cm6UaLry7+UvkBacsYQftkBLQGzRJ0605YpGR0O2+FDhlkzC3LsRF/HiOTXwH91RIiBbextyiytmez9tJMXoYT7068m7tGT3ZdHi5TbFvOTVtQ31ej97MPqwuDwp/t+5DSk15nj62PjTPOobRtUaX39dK1AZbOZcZudDgnjkY/fSh2uVPpT8E+BUJdRGA==
  • Authentication-results: lists.xenproject.org; dkim=none (message not signed) header.d=none;lists.xenproject.org; dmarc=none action=none header.from=oracle.com;
  • Cc: andrew.cooper3@xxxxxxxxxx, wl@xxxxxxx, stephen.s.brennan@xxxxxxxxxx, iwj@xxxxxxxxxxxxxx, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Mon, 29 Mar 2021 15:05:14 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 3/29/21 5:56 AM, Jan Beulich wrote:
> On 27.03.2021 02:51, Boris Ostrovsky wrote:
>
>> @@ -543,8 +554,10 @@ void create_periodic_time(
>>      pt->cb = cb;
>>      pt->priv = data;
>>  
>> +    pt_vcpu_lock(pt->vcpu);
>>      pt->on_list = 1;
>>      list_add(&pt->list, &v->arch.hvm.tm_list);
>> +    pt_vcpu_unlock(pt->vcpu);
> I think it would be better (not just code generation wise) to use v
> here.
>
>> @@ -580,13 +593,22 @@ static void pt_adjust_vcpu(struct periodic_time *pt, 
>> struct vcpu *v)
>>          return;
>>  
>>      write_lock(&pt->vcpu->domain->arch.hvm.pl_time->pt_migrate);
>> +
>> +    pt_vcpu_lock(pt->vcpu);
>> +    if ( pt->on_list )
>> +        list_del(&pt->list);
>> +    pt_vcpu_unlock(pt->vcpu);
> While these two obviously can't use v, ...
>
>>      pt->vcpu = v;
>> +
>> +    pt_vcpu_lock(pt->vcpu);
>>      if ( pt->on_list )
>>      {
>> -        list_del(&pt->list);
>>          list_add(&pt->list, &v->arch.hvm.tm_list);
>>          migrate_timer(&pt->timer, v->processor);
>>      }
>> +    pt_vcpu_unlock(pt->vcpu);
> ... these two again could (and imo should), and ...
>
>>      write_unlock(&pt->vcpu->domain->arch.hvm.pl_time->pt_migrate);
> ... really this and its counterpart better would do so, too (albeit
> perhaps in a separate patch).


Are you suggesting to replace pt->vcpu with v here? They are different at lock 
and unlock points (although they obviously point to the same domain).


>
> While I see that pt_adjust_global_vcpu_target() (the only caller of
> pt_adjust_vcpu()) already avoids calling here when the vcpu there
> doesn't really change, I also think that with all this extra locking
> the function now would better short-circuit the case of
> pt->vcpu == v upon entry (or more precisely once the write lock was
> acquired).


Sure. I'll add this (and address comment changes from you and Roger).


-boris






 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.