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;
From:   Long Li <longli@microsoft.com>
To:     Andrea Parri <parri.andrea@gmail.com>
CC:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        KY Srinivasan <kys@microsoft.com>,
        Haiyang Zhang <haiyangz@microsoft.com>,
        Stephen Hemminger <sthemmin@microsoft.com>,
        Wei Liu <wei.liu@kernel.org>,
        "linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>,
        Michael Kelley <mikelley@microsoft.com>,
        Dexuan Cui <decui@microsoft.com>,
        Boqun Feng <boqun.feng@gmail.com>,
        vkuznets <vkuznets@redhat.com>,
        "James E.J. Bottomley" <jejb@linux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@oracle.com>,
        "linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>
Subject: RE: [PATCH 11/11] scsi: storvsc: Re-init stor_chns when a channel
 interrupt is re-assigned
Thread-Topic: [PATCH 11/11] scsi: storvsc: Re-init stor_chns when a channel
 interrupt is re-assigned
Thread-Index: AQHWC6i+EppuRPIzmUWsHp6UWcZ3DKhscw0QgABenQCAAa+rcA==
Date:   Wed, 8 Apr 2020 02:25:52 +0000
Message-ID: <BN8PR21MB1155E335C1E390964C08B6E3CEC00@BN8PR21MB1155.namprd21.prod.outlook.com>
References: <20200406001514.19876-1-parri.andrea@gmail.com>
 <20200406001514.19876-12-parri.andrea@gmail.com>
 <BN8PR21MB1155C78AAC02D02EA7A7841DCEC20@BN8PR21MB1155.namprd21.prod.outlook.com>
 <20200407003459.GA7776@andrea>
In-Reply-To: <20200407003459.GA7776@andrea>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: microsoft.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 8cba394b-ff6e-44cd-050f-08d7db642913
X-MS-Exchange-CrossTenant-originalarrivaltime: 08 Apr 2020 02:25:52.6910
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 72f988bf-86f1-41af-91ab-2d7cd011db47
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: vxDfTEha5DaXXn6ql+xNfSVPjsk9Qa2529krq+2lZPCdWDrI2zWBRdAbSARUT0mwatoxP0XQyxYh5Q0acnDCEw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN8PR21MB1364
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

>Subject: Re: [PATCH 11/11] scsi: storvsc: Re-init stor_chns when a channel
>interrupt is re-assigned
>
>> >@@ -621,6 +621,63 @@ static inline struct storvsc_device
>> >*get_in_stor_device(
>> >
>> > }
>> >
>> >+void storvsc_change_target_cpu(struct vmbus_channel *channel, u32
>> >+old,
>> >+u32 new) {
>> >+	struct storvsc_device *stor_device;
>> >+	struct vmbus_channel *cur_chn;
>> >+	bool old_is_alloced =3D false;
>> >+	struct hv_device *device;
>> >+	unsigned long flags;
>> >+	int cpu;
>> >+
>> >+	device =3D channel->primary_channel ?
>> >+			channel->primary_channel->device_obj
>> >+				: channel->device_obj;
>> >+	stor_device =3D get_out_stor_device(device);
>> >+	if (!stor_device)
>> >+		return;
>> >+
>> >+	/* See storvsc_do_io() -> get_og_chn(). */
>> >+	spin_lock_irqsave(&device->channel->lock, flags);
>> >+
>> >+	/*
>> >+	 * Determines if the storvsc device has other channels assigned to
>> >+	 * the "old" CPU to update the alloced_cpus mask and the stor_chns
>> >+	 * array.
>> >+	 */
>> >+	if (device->channel !=3D channel && device->channel->target_cpu =3D=
=3D
>> >old) {
>> >+		cur_chn =3D device->channel;
>> >+		old_is_alloced =3D true;
>> >+		goto old_is_alloced;
>> >+	}
>> >+	list_for_each_entry(cur_chn, &device->channel->sc_list, sc_list) {
>> >+		if (cur_chn =3D=3D channel)
>> >+			continue;
>> >+		if (cur_chn->target_cpu =3D=3D old) {
>> >+			old_is_alloced =3D true;
>> >+			goto old_is_alloced;
>> >+		}
>> >+	}
>> >+
>> >+old_is_alloced:
>> >+	if (old_is_alloced)
>> >+		WRITE_ONCE(stor_device->stor_chns[old], cur_chn);
>> >+	else
>> >+		cpumask_clear_cpu(old, &stor_device->alloced_cpus);
>>
>> If the old cpu is not allocated, is it still necessary to do a cpumask_c=
lear_cpu?
>
>AFAICT, this really depends on how much we "believe" in the current heuris=
tic
>(as implemented by get_og_chn()):  ;-)
>
>The cpumask_clear_cpu() (and the below, dependent "flush" as well) are
>intended to re-initialize alloced_cpus and stor_chns in order for get_og_c=
hn()
>to re-process/update them.
>
>Also, notice that (both in the current code and after this series) alloced=
_cpus
>can't be offlined and get_og_chn() does rely on this property (cf., e.g., =
the
>loop/check over alloced_cpus/node_mask).
>
>I suspect that giving up on this invariant/property would require a certai=
n
>amount of re-design in the heuristic/code in question...
>
>
>> >@@ -1360,7 +1432,14 @@ static int storvsc_do_io(struct hv_device
>*device,
>> > 			}
>> > 		}
>> > 	} else {
>> >+		spin_lock_irqsave(&device->channel->lock, flags);
>> >+		outgoing_channel =3D stor_device->stor_chns[q_num];
>> >+		if (outgoing_channel !=3D NULL) {
>> >+			spin_unlock_irqrestore(&device->channel->lock,
>> >flags);
>>
>> Checking outgoing_channel again seems unnecessary. Why not just call
>get_og_chn()?
>
>target_cpu_store() might have changed stor_chns (and alloced_cpus) in the
>meantime (but before we've acquired the device's lock): the double check i=
s
>to make sure we have a "consistent"/an up-to-date view of stor_chns and
>alloced_cpus.
>
>
>>
>> >+			goto found_channel;
>> >+		}
>> > 		outgoing_channel =3D get_og_chn(stor_device, q_num);
>> >+		spin_unlock_irqrestore(&device->channel->lock, flags);
>> > 	}
>>
>> With device->channel->lock, now we have one more lock on the I/O issuing
>path. It doesn't seem optimal as you are trying to protect the code in
>storvsc_change_target_cpu(), this doesn't need to block concurrent I/O
>issuers. Maybe moving to RCU is a better approach?
>
>I don't see this as a problem (*and I've validated such conclusion in
>experiments, where the "patched kernel" was sometimes performing slighlty
>better than the "unpatched kernel" and sometimes slightly
>worse...):
>
>On the one hand, the stor_chns array "stabilizes" quite early after system
>initialization in "normal" (i.e., common) situations (i.e., no channel
>reassignments, no device hotplugs...); IOW, get_og_chn() really represents
>the "rare and slow" path here (but not that slow!
>after all...).  Furthermore, notice that even in those "rare cases"
>the number of "contending" channels is limited to at most 1 per 4 CPUs IIR=
C
>(alloced_cpus is "sparsely populated"...).

Yes I realized it is on the slow path. There is no need to optimize locks.

Reviewed-by; Long Li <longli@microsoft.com>

>
>The latencies of the RCU grace period (in the order of milliseconds) would=
 be a
>major concern for the adoption of RCU here (at least, if we continue to
>consider get_og_chn() as an "updater").  I'm afraid that this could be "to=
o
>slow" even for our slow path...  ;-/
>
>What am I missing?  ;-)
>
>Thanks,
>  Andrea