* Re: [PATCH 1/4] KVM: x86: hyperv: Remove duplicate definitions of Reference TSC Page
From: Paolo Bonzini @ 2020-04-21 11:16 UTC (permalink / raw)
To: Wei Liu, Michael Kelley
Cc: kys, haiyangz, sthemmin, tglx, mingo, bp, x86, hpa,
sean.j.christopherson, vkuznets, wanpengli, jmattson, joro, kvm,
linux-kernel, linux-hyperv
In-Reply-To: <20200421092925.rxb72yep4paruvi6@debian>
On 21/04/20 11:29, Wei Liu wrote:
> On Mon, Apr 20, 2020 at 10:38:35AM -0700, Michael Kelley wrote:
>> The Hyper-V Reference TSC Page structure is defined twice. struct
>> ms_hyperv_tsc_page has padding out to a full 4 Kbyte page size. But
>> the padding is not needed because the declaration includes a union
>> with HV_HYP_PAGE_SIZE. KVM uses the second definition, which is
>> struct _HV_REFERENCE_TSC_PAGE, because it does not have the padding.
>>
>> Fix the duplication by removing the padding from ms_hyperv_tsc_page.
>> Fix up the KVM code to use it. Remove the no longer used struct
>> _HV_REFERENCE_TSC_PAGE.
>>
>> There is no functional change.
>>
>> Signed-off-by: Michael Kelley <mikelley@microsoft.com>
>> ---
>> arch/x86/include/asm/hyperv-tlfs.h | 8 --------
>> arch/x86/include/asm/kvm_host.h | 2 +-
>> arch/x86/kvm/hyperv.c | 4 ++--
>
> Paolo, this patch touches KVM code. Let me know how you would like to
> handle this.
Just include it, I don't expect conflicts.
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Paolo
^ permalink raw reply
* Re: [PATCH 1/4] KVM: x86: hyperv: Remove duplicate definitions of Reference TSC Page
From: Wei Liu @ 2020-04-21 11:18 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Wei Liu, Michael Kelley, kys, haiyangz, sthemmin, tglx, mingo, bp,
x86, hpa, sean.j.christopherson, vkuznets, wanpengli, jmattson,
joro, kvm, linux-kernel, linux-hyperv
In-Reply-To: <6c2bae31-14a8-39cf-6e6d-139d84146477@redhat.com>
On Tue, Apr 21, 2020 at 01:16:42PM +0200, Paolo Bonzini wrote:
> On 21/04/20 11:29, Wei Liu wrote:
> > On Mon, Apr 20, 2020 at 10:38:35AM -0700, Michael Kelley wrote:
> >> The Hyper-V Reference TSC Page structure is defined twice. struct
> >> ms_hyperv_tsc_page has padding out to a full 4 Kbyte page size. But
> >> the padding is not needed because the declaration includes a union
> >> with HV_HYP_PAGE_SIZE. KVM uses the second definition, which is
> >> struct _HV_REFERENCE_TSC_PAGE, because it does not have the padding.
> >>
> >> Fix the duplication by removing the padding from ms_hyperv_tsc_page.
> >> Fix up the KVM code to use it. Remove the no longer used struct
> >> _HV_REFERENCE_TSC_PAGE.
> >>
> >> There is no functional change.
> >>
> >> Signed-off-by: Michael Kelley <mikelley@microsoft.com>
> >> ---
> >> arch/x86/include/asm/hyperv-tlfs.h | 8 --------
> >> arch/x86/include/asm/kvm_host.h | 2 +-
> >> arch/x86/kvm/hyperv.c | 4 ++--
> >
> > Paolo, this patch touches KVM code. Let me know how you would like to
> > handle this.
>
> Just include it, I don't expect conflicts.
>
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Ack.
^ permalink raw reply
* Re: [PATCH 0/4] Split hyperv-tlfs.h into generic and arch specific files
From: Wei Liu @ 2020-04-21 11:24 UTC (permalink / raw)
To: Michael Kelley
Cc: kys, haiyangz, sthemmin, wei.liu, tglx, mingo, bp, x86, hpa,
pbonzini, sean.j.christopherson, vkuznets, wanpengli, jmattson,
joro, kvm, linux-kernel, linux-hyperv
In-Reply-To: <20200420173838.24672-1-mikelley@microsoft.com>
On Mon, Apr 20, 2020 at 10:38:34AM -0700, Michael Kelley wrote:
> Michael Kelley (4):
> KVM: x86: hyperv: Remove duplicate definitions of Reference TSC Page
> x86/hyperv: Remove HV_PROCESSOR_POWER_STATE #defines
> x86/hyperv: Split hyperv-tlfs.h into arch dependent and independent
> files
> asm-generic/hyperv: Add definitions for Get/SetVpRegister hypercalls
>
> arch/x86/include/asm/hyperv-tlfs.h | 472 +++--------------------------
> arch/x86/include/asm/kvm_host.h | 2 +-
> arch/x86/kvm/hyperv.c | 4 +-
> include/asm-generic/hyperv-tlfs.h | 470 ++++++++++++++++++++++++++++
> 4 files changed, 509 insertions(+), 439 deletions(-)
> create mode 100644 include/asm-generic/hyperv-tlfs.h
This series looks good to me.
I will wait for a few days before actually queueing it to hyperv-next so
that other people can have a chance to comment.
Wei.
^ permalink raw reply
* Re: [PATCH 1/4] KVM: x86: hyperv: Remove duplicate definitions of Reference TSC Page
From: Vitaly Kuznetsov @ 2020-04-21 12:37 UTC (permalink / raw)
To: Michael Kelley
Cc: mikelley, kys, haiyangz, sthemmin, wei.liu, tglx, mingo, bp, x86,
hpa, pbonzini, sean.j.christopherson, wanpengli, jmattson, joro,
kvm, linux-kernel, linux-hyperv
In-Reply-To: <20200420173838.24672-2-mikelley@microsoft.com>
Michael Kelley <mikelley@microsoft.com> writes:
> The Hyper-V Reference TSC Page structure is defined twice. struct
> ms_hyperv_tsc_page has padding out to a full 4 Kbyte page size. But
> the padding is not needed because the declaration includes a union
> with HV_HYP_PAGE_SIZE. KVM uses the second definition, which is
> struct _HV_REFERENCE_TSC_PAGE, because it does not have the padding.
>
> Fix the duplication by removing the padding from ms_hyperv_tsc_page.
> Fix up the KVM code to use it. Remove the no longer used struct
> _HV_REFERENCE_TSC_PAGE.
>
> There is no functional change.
>
> Signed-off-by: Michael Kelley <mikelley@microsoft.com>
> ---
> arch/x86/include/asm/hyperv-tlfs.h | 8 --------
> arch/x86/include/asm/kvm_host.h | 2 +-
> arch/x86/kvm/hyperv.c | 4 ++--
> 3 files changed, 3 insertions(+), 11 deletions(-)
>
> diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h
> index 29336574d0bc..0e4d76920957 100644
> --- a/arch/x86/include/asm/hyperv-tlfs.h
> +++ b/arch/x86/include/asm/hyperv-tlfs.h
> @@ -303,7 +303,6 @@ struct ms_hyperv_tsc_page {
> u32 reserved1;
> volatile u64 tsc_scale;
> volatile s64 tsc_offset;
> - u64 reserved2[509];
> } __packed;
>
> /*
> @@ -433,13 +432,6 @@ enum HV_GENERIC_SET_FORMAT {
> */
> #define HV_CLOCK_HZ (NSEC_PER_SEC/100)
>
> -typedef struct _HV_REFERENCE_TSC_PAGE {
> - __u32 tsc_sequence;
> - __u32 res1;
> - __u64 tsc_scale;
> - __s64 tsc_offset;
> -} __packed HV_REFERENCE_TSC_PAGE, *PHV_REFERENCE_TSC_PAGE;
> -
> /* Define the number of synthetic interrupt sources. */
> #define HV_SYNIC_SINT_COUNT (16)
> /* Define the expected SynIC version. */
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 42a2d0d3984a..4698343b9a05 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -865,7 +865,7 @@ struct kvm_hv {
> u64 hv_crash_param[HV_X64_MSR_CRASH_PARAMS];
> u64 hv_crash_ctl;
>
> - HV_REFERENCE_TSC_PAGE tsc_ref;
> + struct ms_hyperv_tsc_page tsc_ref;
>
> struct idr conn_to_evt;
>
> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> index bcefa9d4e57e..1f3c6fd3cdaa 100644
> --- a/arch/x86/kvm/hyperv.c
> +++ b/arch/x86/kvm/hyperv.c
> @@ -900,7 +900,7 @@ static int kvm_hv_msr_set_crash_data(struct kvm_vcpu *vcpu,
> * These two equivalencies are implemented in this function.
> */
> static bool compute_tsc_page_parameters(struct pvclock_vcpu_time_info *hv_clock,
> - HV_REFERENCE_TSC_PAGE *tsc_ref)
> + struct ms_hyperv_tsc_page *tsc_ref)
> {
> u64 max_mul;
>
> @@ -941,7 +941,7 @@ void kvm_hv_setup_tsc_page(struct kvm *kvm,
> u64 gfn;
>
> BUILD_BUG_ON(sizeof(tsc_seq) != sizeof(hv->tsc_ref.tsc_sequence));
> - BUILD_BUG_ON(offsetof(HV_REFERENCE_TSC_PAGE, tsc_sequence) != 0);
> + BUILD_BUG_ON(offsetof(struct ms_hyperv_tsc_page, tsc_sequence) != 0);
>
> if (!(hv->hv_tsc_page & HV_X64_MSR_TSC_REFERENCE_ENABLE))
> return;
Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
--
Vitaly
^ permalink raw reply
* Re: [PATCH 4/4] asm-generic/hyperv: Add definitions for Get/SetVpRegister hypercalls
From: Vitaly Kuznetsov @ 2020-04-21 13:02 UTC (permalink / raw)
To: Michael Kelley
Cc: mikelley, kys, haiyangz, sthemmin, wei.liu, tglx, mingo, bp, x86,
hpa, pbonzini, sean.j.christopherson, wanpengli, jmattson, joro,
kvm, linux-kernel, linux-hyperv
In-Reply-To: <20200420173838.24672-5-mikelley@microsoft.com>
Michael Kelley <mikelley@microsoft.com> writes:
> Add definitions for GetVpRegister and SetVpRegister hypercalls, which
> are implemented for both x86 and ARM64.
>
> Signed-off-by: Michael Kelley <mikelley@microsoft.com>
> ---
> include/asm-generic/hyperv-tlfs.h | 28 ++++++++++++++++++++++++++++
> 1 file changed, 28 insertions(+)
>
> diff --git a/include/asm-generic/hyperv-tlfs.h b/include/asm-generic/hyperv-tlfs.h
> index 1f92ef92eb56..29b60f5b6323 100644
> --- a/include/asm-generic/hyperv-tlfs.h
> +++ b/include/asm-generic/hyperv-tlfs.h
> @@ -141,6 +141,8 @@ struct ms_hyperv_tsc_page {
> #define HVCALL_FLUSH_VIRTUAL_ADDRESS_SPACE_EX 0x0013
> #define HVCALL_FLUSH_VIRTUAL_ADDRESS_LIST_EX 0x0014
> #define HVCALL_SEND_IPI_EX 0x0015
> +#define HVCALL_GET_VP_REGISTERS 0x0050
> +#define HVCALL_SET_VP_REGISTERS 0x0051
> #define HVCALL_POST_MESSAGE 0x005c
> #define HVCALL_SIGNAL_EVENT 0x005d
> #define HVCALL_RETARGET_INTERRUPT 0x007e
> @@ -439,4 +441,30 @@ struct hv_retarget_device_interrupt {
> struct hv_device_interrupt_target int_target;
> } __packed __aligned(8);
>
> +
> +/* HvGetVPRegister hypercall */
Nit: 'HvGetVpRegisters' in TLFS
> +struct hv_get_vp_register_input {
Nit: I would also to name it 'hv_get_vp_registers_input' (plural, like
the hypercall).
> + u64 partitionid;
> + u32 vpindex;
> + u8 inputvtl;
> + u8 padding[3];
> + u32 name0;
> + u32 name1;
> +} __packed;
Isn't it a REP hypercall where we can we can pass a list? In that case
this should look like
struct hv_get_vp_registers_input {
struct {
u64 partitionid;
u32 vpindex;
u8 inputvtl;
u8 padding[3];
} header;
struct {
u32 name0;
u32 name1;
} elem[];
} __packed;
> +
> +struct hv_get_vp_register_output {
Ditto.
> + union {
> + struct {
> + u32 a;
> + u32 b;
> + u32 c;
> + u32 d;
> + } as32 __packed;
> + struct {
> + u64 low;
> + u64 high;
> + } as64 __packed;
> + };
> +};
I'm wondering why you define both
HVCALL_GET_VP_REGISTERS/HVCALL_SET_VP_REGISTERS but only add 'struct
hv_get_vp_register_input' and not 'struct hv_set_vp_register_input'.
The later should look similar, AFAIU it is:
struct hv_set_vp_registers_input {
struct {
u64 partitionid;
u32 vpindex;
u8 inputvtl;
u8 padding[3];
} header;
struct {
u32 name;
u32 padding1;
u64 padding2; //not sure this is not a mistake in TLFS
u64 regvallow;
u64 regvalhigh;
} elem[];
} __packed;
> +
> #endif
--
Vitaly
^ permalink raw reply
* RE: [PATCH 4/4] asm-generic/hyperv: Add definitions for Get/SetVpRegister hypercalls
From: Michael Kelley @ 2020-04-21 15:50 UTC (permalink / raw)
To: vkuznets
Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
wei.liu@kernel.org, tglx@linutronix.de, mingo@redhat.com,
bp@alien8.de, x86@kernel.org, hpa@zytor.com, pbonzini@redhat.com,
sean.j.christopherson@intel.com, wanpengli@tencent.com,
jmattson@google.com, joro@8bytes.org, kvm@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org
In-Reply-To: <87y2qpq9e7.fsf@vitty.brq.redhat.com>
From: Vitaly Kuznetsov <vkuznets@redhat.com> Sent: Tuesday, April 21, 2020 6:03 AM
>
> Michael Kelley <mikelley@microsoft.com> writes:
>
> > Add definitions for GetVpRegister and SetVpRegister hypercalls, which
> > are implemented for both x86 and ARM64.
> >
> > Signed-off-by: Michael Kelley <mikelley@microsoft.com>
> > ---
> > include/asm-generic/hyperv-tlfs.h | 28 ++++++++++++++++++++++++++++
> > 1 file changed, 28 insertions(+)
> >
> > diff --git a/include/asm-generic/hyperv-tlfs.h b/include/asm-generic/hyperv-tlfs.h
> > index 1f92ef92eb56..29b60f5b6323 100644
> > --- a/include/asm-generic/hyperv-tlfs.h
> > +++ b/include/asm-generic/hyperv-tlfs.h
> > @@ -141,6 +141,8 @@ struct ms_hyperv_tsc_page {
> > #define HVCALL_FLUSH_VIRTUAL_ADDRESS_SPACE_EX 0x0013
> > #define HVCALL_FLUSH_VIRTUAL_ADDRESS_LIST_EX 0x0014
> > #define HVCALL_SEND_IPI_EX 0x0015
> > +#define HVCALL_GET_VP_REGISTERS 0x0050
> > +#define HVCALL_SET_VP_REGISTERS 0x0051
> > #define HVCALL_POST_MESSAGE 0x005c
> > #define HVCALL_SIGNAL_EVENT 0x005d
> > #define HVCALL_RETARGET_INTERRUPT 0x007e
> > @@ -439,4 +441,30 @@ struct hv_retarget_device_interrupt {
> > struct hv_device_interrupt_target int_target;
> > } __packed __aligned(8);
> >
> > +
> > +/* HvGetVPRegister hypercall */
>
> Nit: 'HvGetVpRegisters' in TLFS
>
> > +struct hv_get_vp_register_input {
>
> Nit: I would also to name it 'hv_get_vp_registers_input' (plural, like
> the hypercall).
>
> > + u64 partitionid;
> > + u32 vpindex;
> > + u8 inputvtl;
> > + u8 padding[3];
> > + u32 name0;
> > + u32 name1;
> > +} __packed;
>
> Isn't it a REP hypercall where we can we can pass a list? In that case
> this should look like
>
> struct hv_get_vp_registers_input {
> struct {
> u64 partitionid;
> u32 vpindex;
> u8 inputvtl;
> u8 padding[3];
> } header;
> struct {
> u32 name0;
> u32 name1;
> } elem[];
> } __packed;
>
> > +
> > +struct hv_get_vp_register_output {
>
> Ditto.
>
> > + union {
> > + struct {
> > + u32 a;
> > + u32 b;
> > + u32 c;
> > + u32 d;
> > + } as32 __packed;
> > + struct {
> > + u64 low;
> > + u64 high;
> > + } as64 __packed;
> > + };
> > +};
>
> I'm wondering why you define both
> HVCALL_GET_VP_REGISTERS/HVCALL_SET_VP_REGISTERS but only add 'struct
> hv_get_vp_register_input' and not 'struct hv_set_vp_register_input'.
>
> The later should look similar, AFAIU it is:
>
> struct hv_set_vp_registers_input {
> struct {
> u64 partitionid;
> u32 vpindex;
> u8 inputvtl;
> u8 padding[3];
> } header;
> struct {
> u32 name;
> u32 padding1;
> u64 padding2; //not sure this is not a mistake in TLFS
> u64 regvallow;
> u64 regvalhigh;
> } elem[];
> } __packed;
>
> > +
> > #endif
>
> --
> Vitaly
Thanks. I'll make these changes. I was being a bit lazy since
the ARM64 code only gets/sets a single register at a time. Also
HvSetVpRegisters works very nicely as a fast hypercall when setting
a single register, so I didn't define the in-memory data structure.
But for completeness, I shouldn't take these shortcuts, so I'll do
an update.
Michael
^ permalink raw reply
* [PATCH] scsi: storvsc: Fix a panic in the hibernation procedure
From: Dexuan Cui @ 2020-04-22 0:17 UTC (permalink / raw)
To: jejb, martin.petersen, linux-scsi, linux-kernel, hch, bvanassche,
hare, mikelley, longli, ming.lei
Cc: linux-hyperv, wei.liu, sthemmin, haiyangz, kys, Dexuan Cui
During hibernation, the sdevs are suspended automatically in
drivers/scsi/scsi_pm.c before storvsc_suspend(), so after
storvsc_suspend(), there is no disk I/O from the file systems, but there
can still be disk I/O from the kernel space, e.g. disk_check_events() ->
sr_block_check_events() -> cdrom_check_events() can still submit I/O
to the storvsc driver, which causes a paic of NULL pointer dereference,
since storvsc has closed the vmbus channel in storvsc_suspend(): refer
to the below links for more info:
https://lkml.org/lkml/2020/4/10/47
https://lkml.org/lkml/2020/4/17/1103
Fix the panic by blocking/unblocking all the I/O queues properly.
Note: this patch depends on another patch "scsi: core: Allow the state
change from SDEV_QUIESCE to SDEV_BLOCK" (refer to the second link above).
Fixes: 56fb10585934 ("scsi: storvsc: Add the support of hibernation")
Signed-off-by: Dexuan Cui <decui@microsoft.com>
---
drivers/scsi/storvsc_drv.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index fb41636519ee..fd51d2f03778 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -1948,6 +1948,11 @@ static int storvsc_suspend(struct hv_device *hv_dev)
struct storvsc_device *stor_device = hv_get_drvdata(hv_dev);
struct Scsi_Host *host = stor_device->host;
struct hv_host_device *host_dev = shost_priv(host);
+ int ret;
+
+ ret = scsi_host_block(host);
+ if (ret)
+ return ret;
storvsc_wait_to_drain(stor_device);
@@ -1968,10 +1973,15 @@ static int storvsc_suspend(struct hv_device *hv_dev)
static int storvsc_resume(struct hv_device *hv_dev)
{
+ struct storvsc_device *stor_device = hv_get_drvdata(hv_dev);
+ struct Scsi_Host *host = stor_device->host;
int ret;
ret = storvsc_connect_to_vsp(hv_dev, storvsc_ringbuffer_size,
hv_dev_is_fc(hv_dev));
+ if (!ret)
+ ret = scsi_host_unblock(host, SDEV_RUNNING);
+
return ret;
}
--
2.19.1
^ permalink raw reply related
* Re: [PATCH] scsi: storvsc: Fix a panic in the hibernation procedure
From: Ming Lei @ 2020-04-22 1:28 UTC (permalink / raw)
To: Dexuan Cui
Cc: jejb, martin.petersen, linux-scsi, linux-kernel, hch, bvanassche,
hare, mikelley, longli, linux-hyperv, wei.liu, sthemmin, haiyangz,
kys
In-Reply-To: <1587514644-47058-1-git-send-email-decui@microsoft.com>
On Tue, Apr 21, 2020 at 05:17:24PM -0700, Dexuan Cui wrote:
> During hibernation, the sdevs are suspended automatically in
> drivers/scsi/scsi_pm.c before storvsc_suspend(), so after
> storvsc_suspend(), there is no disk I/O from the file systems, but there
> can still be disk I/O from the kernel space, e.g. disk_check_events() ->
> sr_block_check_events() -> cdrom_check_events() can still submit I/O
> to the storvsc driver, which causes a paic of NULL pointer dereference,
> since storvsc has closed the vmbus channel in storvsc_suspend(): refer
> to the below links for more info:
> https://lkml.org/lkml/2020/4/10/47
> https://lkml.org/lkml/2020/4/17/1103
>
> Fix the panic by blocking/unblocking all the I/O queues properly.
>
> Note: this patch depends on another patch "scsi: core: Allow the state
> change from SDEV_QUIESCE to SDEV_BLOCK" (refer to the second link above).
>
> Fixes: 56fb10585934 ("scsi: storvsc: Add the support of hibernation")
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
> drivers/scsi/storvsc_drv.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index fb41636519ee..fd51d2f03778 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -1948,6 +1948,11 @@ static int storvsc_suspend(struct hv_device *hv_dev)
> struct storvsc_device *stor_device = hv_get_drvdata(hv_dev);
> struct Scsi_Host *host = stor_device->host;
> struct hv_host_device *host_dev = shost_priv(host);
> + int ret;
> +
> + ret = scsi_host_block(host);
> + if (ret)
> + return ret;
>
> storvsc_wait_to_drain(stor_device);
>
> @@ -1968,10 +1973,15 @@ static int storvsc_suspend(struct hv_device *hv_dev)
>
> static int storvsc_resume(struct hv_device *hv_dev)
> {
> + struct storvsc_device *stor_device = hv_get_drvdata(hv_dev);
> + struct Scsi_Host *host = stor_device->host;
> int ret;
>
> ret = storvsc_connect_to_vsp(hv_dev, storvsc_ringbuffer_size,
> hv_dev_is_fc(hv_dev));
> + if (!ret)
> + ret = scsi_host_unblock(host, SDEV_RUNNING);
> +
> return ret;
> }
scsi_host_block() is actually too heavy for just avoiding
scsi internal command, which can be done simply by one atomic
variable.
Not mention scsi_host_block() is implemented too clumsy because
nr_luns * synchronize_rcu() are required in scsi_host_block(),
which should have been optimized to just one.
Also scsi_device_quiesce() is heavy too, still takes 2
synchronize_rcu() for one LUN.
That is said SCSI suspend may take (3 * nr_luns) sysnchronize_rcu() in
case that the HBA's suspend handler needs scsi_host_block().
Thanks,
Ming
^ permalink raw reply
* RE: [PATCH] scsi: storvsc: Fix a panic in the hibernation procedure
From: Dexuan Cui @ 2020-04-22 1:48 UTC (permalink / raw)
To: Ming Lei
Cc: jejb@linux.ibm.com, martin.petersen@oracle.com,
linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org,
hch@lst.de, bvanassche@acm.org, hare@suse.de, Michael Kelley,
Long Li, linux-hyperv@vger.kernel.org, wei.liu@kernel.org,
Stephen Hemminger, Haiyang Zhang, KY Srinivasan
In-Reply-To: <20200422012814.GB299948@T590>
> From: Ming Lei <ming.lei@redhat.com>
> Sent: Tuesday, April 21, 2020 6:28 PM
> To: Dexuan Cui <decui@microsoft.com>
>
> On Tue, Apr 21, 2020 at 05:17:24PM -0700, Dexuan Cui wrote:
> > During hibernation, the sdevs are suspended automatically in
> > drivers/scsi/scsi_pm.c before storvsc_suspend(), so after
> > storvsc_suspend(), there is no disk I/O from the file systems, but there
> > can still be disk I/O from the kernel space, e.g. disk_check_events() ->
> > sr_block_check_events() -> cdrom_check_events() can still submit I/O
> > to the storvsc driver, which causes a paic of NULL pointer dereference,
> > since storvsc has closed the vmbus channel in storvsc_suspend(): refer
> > to the below links for more info:
> >
> > Fix the panic by blocking/unblocking all the I/O queues properly.
> >
> > Note: this patch depends on another patch "scsi: core: Allow the state
> > change from SDEV_QUIESCE to SDEV_BLOCK" (refer to the second link
> above).
> >
> > Fixes: 56fb10585934 ("scsi: storvsc: Add the support of hibernation")
> > Signed-off-by: Dexuan Cui <decui@microsoft.com>
> > ---
> > drivers/scsi/storvsc_drv.c | 10 ++++++++++
> > 1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> > index fb41636519ee..fd51d2f03778 100644
> > --- a/drivers/scsi/storvsc_drv.c
> > +++ b/drivers/scsi/storvsc_drv.c
> > @@ -1948,6 +1948,11 @@ static int storvsc_suspend(struct hv_device
> *hv_dev)
> > struct storvsc_device *stor_device = hv_get_drvdata(hv_dev);
> > struct Scsi_Host *host = stor_device->host;
> > struct hv_host_device *host_dev = shost_priv(host);
> > + int ret;
> > +
> > + ret = scsi_host_block(host);
> > + if (ret)
> > + return ret;
> >
> > storvsc_wait_to_drain(stor_device);
> >
> > @@ -1968,10 +1973,15 @@ static int storvsc_suspend(struct hv_device
> *hv_dev)
> >
> > static int storvsc_resume(struct hv_device *hv_dev)
> > {
> > + struct storvsc_device *stor_device = hv_get_drvdata(hv_dev);
> > + struct Scsi_Host *host = stor_device->host;
> > int ret;
> >
> > ret = storvsc_connect_to_vsp(hv_dev, storvsc_ringbuffer_size,
> > hv_dev_is_fc(hv_dev));
> > + if (!ret)
> > + ret = scsi_host_unblock(host, SDEV_RUNNING);
> > +
> > return ret;
> > }
>
> scsi_host_block() is actually too heavy for just avoiding
> scsi internal command, which can be done simply by one atomic
> variable.
>
> Not mention scsi_host_block() is implemented too clumsy because
> nr_luns * synchronize_rcu() are required in scsi_host_block(),
> which should have been optimized to just one.
>
> Also scsi_device_quiesce() is heavy too, still takes 2
> synchronize_rcu() for one LUN.
>
> That is said SCSI suspend may take (3 * nr_luns) sysnchronize_rcu() in
> case that the HBA's suspend handler needs scsi_host_block().
>
> Thanks,
> Ming
When we're in storvsc_suspend(), all the userspace processes have been
frozen and all the file systems have been flushed, and there should not
be too much I/O from the kernel space, so IMO scsi_host_block() should be
pretty fast here.
Of cource, any performance improvement to the scsi_host_block() API is
still great. :-)
Thanks,
-- Dexuan
^ permalink raw reply
* Re: [PATCH] scsi: storvsc: Fix a panic in the hibernation procedure
From: Ming Lei @ 2020-04-22 2:01 UTC (permalink / raw)
To: Dexuan Cui, Paul E. McKenney, Josh Triplett, Rafael J. Wysocki
Cc: jejb@linux.ibm.com, martin.petersen@oracle.com,
linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org,
hch@lst.de, bvanassche@acm.org, hare@suse.de, Michael Kelley,
Long Li, linux-hyperv@vger.kernel.org, wei.liu@kernel.org,
Stephen Hemminger, Haiyang Zhang, KY Srinivasan, linux-pm
In-Reply-To: <HK0P153MB0273B954294B331E20AACB41BFD20@HK0P153MB0273.APCP153.PROD.OUTLOOK.COM>
On Wed, Apr 22, 2020 at 01:48:25AM +0000, Dexuan Cui wrote:
> > From: Ming Lei <ming.lei@redhat.com>
> > Sent: Tuesday, April 21, 2020 6:28 PM
> > To: Dexuan Cui <decui@microsoft.com>
> >
> > On Tue, Apr 21, 2020 at 05:17:24PM -0700, Dexuan Cui wrote:
> > > During hibernation, the sdevs are suspended automatically in
> > > drivers/scsi/scsi_pm.c before storvsc_suspend(), so after
> > > storvsc_suspend(), there is no disk I/O from the file systems, but there
> > > can still be disk I/O from the kernel space, e.g. disk_check_events() ->
> > > sr_block_check_events() -> cdrom_check_events() can still submit I/O
> > > to the storvsc driver, which causes a paic of NULL pointer dereference,
> > > since storvsc has closed the vmbus channel in storvsc_suspend(): refer
> > > to the below links for more info:
> > >
> > > Fix the panic by blocking/unblocking all the I/O queues properly.
> > >
> > > Note: this patch depends on another patch "scsi: core: Allow the state
> > > change from SDEV_QUIESCE to SDEV_BLOCK" (refer to the second link
> > above).
> > >
> > > Fixes: 56fb10585934 ("scsi: storvsc: Add the support of hibernation")
> > > Signed-off-by: Dexuan Cui <decui@microsoft.com>
> > > ---
> > > drivers/scsi/storvsc_drv.c | 10 ++++++++++
> > > 1 file changed, 10 insertions(+)
> > >
> > > diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> > > index fb41636519ee..fd51d2f03778 100644
> > > --- a/drivers/scsi/storvsc_drv.c
> > > +++ b/drivers/scsi/storvsc_drv.c
> > > @@ -1948,6 +1948,11 @@ static int storvsc_suspend(struct hv_device
> > *hv_dev)
> > > struct storvsc_device *stor_device = hv_get_drvdata(hv_dev);
> > > struct Scsi_Host *host = stor_device->host;
> > > struct hv_host_device *host_dev = shost_priv(host);
> > > + int ret;
> > > +
> > > + ret = scsi_host_block(host);
> > > + if (ret)
> > > + return ret;
> > >
> > > storvsc_wait_to_drain(stor_device);
> > >
> > > @@ -1968,10 +1973,15 @@ static int storvsc_suspend(struct hv_device
> > *hv_dev)
> > >
> > > static int storvsc_resume(struct hv_device *hv_dev)
> > > {
> > > + struct storvsc_device *stor_device = hv_get_drvdata(hv_dev);
> > > + struct Scsi_Host *host = stor_device->host;
> > > int ret;
> > >
> > > ret = storvsc_connect_to_vsp(hv_dev, storvsc_ringbuffer_size,
> > > hv_dev_is_fc(hv_dev));
> > > + if (!ret)
> > > + ret = scsi_host_unblock(host, SDEV_RUNNING);
> > > +
> > > return ret;
> > > }
> >
> > scsi_host_block() is actually too heavy for just avoiding
> > scsi internal command, which can be done simply by one atomic
> > variable.
> >
> > Not mention scsi_host_block() is implemented too clumsy because
> > nr_luns * synchronize_rcu() are required in scsi_host_block(),
> > which should have been optimized to just one.
> >
> > Also scsi_device_quiesce() is heavy too, still takes 2
> > synchronize_rcu() for one LUN.
> >
> > That is said SCSI suspend may take (3 * nr_luns) sysnchronize_rcu() in
> > case that the HBA's suspend handler needs scsi_host_block().
> >
> > Thanks,
> > Ming
>
> When we're in storvsc_suspend(), all the userspace processes have been
> frozen and all the file systems have been flushed, and there should not
> be too much I/O from the kernel space, so IMO scsi_host_block() should be
> pretty fast here.
I guess it depends on RCU's implementation, so CC RCU guys.
Hello Paul & Josh,
Could you clarify that if sysnchronize_rcu becomes quickly during
system suspend?
Thanks,
Ming
^ permalink raw reply
* Re: [PATCH] scsi: storvsc: Fix a panic in the hibernation procedure
From: Ming Lei @ 2020-04-22 2:06 UTC (permalink / raw)
To: Dexuan Cui
Cc: jejb@linux.ibm.com, martin.petersen@oracle.com,
linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org,
hch@lst.de, bvanassche@acm.org, hare@suse.de, Michael Kelley,
Long Li, linux-hyperv@vger.kernel.org, wei.liu@kernel.org,
Stephen Hemminger, Haiyang Zhang, KY Srinivasan
In-Reply-To: <HK0P153MB0273B954294B331E20AACB41BFD20@HK0P153MB0273.APCP153.PROD.OUTLOOK.COM>
On Wed, Apr 22, 2020 at 01:48:25AM +0000, Dexuan Cui wrote:
> > From: Ming Lei <ming.lei@redhat.com>
> > Sent: Tuesday, April 21, 2020 6:28 PM
> > To: Dexuan Cui <decui@microsoft.com>
> >
> > On Tue, Apr 21, 2020 at 05:17:24PM -0700, Dexuan Cui wrote:
> > > During hibernation, the sdevs are suspended automatically in
> > > drivers/scsi/scsi_pm.c before storvsc_suspend(), so after
> > > storvsc_suspend(), there is no disk I/O from the file systems, but there
> > > can still be disk I/O from the kernel space, e.g. disk_check_events() ->
> > > sr_block_check_events() -> cdrom_check_events() can still submit I/O
> > > to the storvsc driver, which causes a paic of NULL pointer dereference,
> > > since storvsc has closed the vmbus channel in storvsc_suspend(): refer
> > > to the below links for more info:
> > >
> > > Fix the panic by blocking/unblocking all the I/O queues properly.
> > >
> > > Note: this patch depends on another patch "scsi: core: Allow the state
> > > change from SDEV_QUIESCE to SDEV_BLOCK" (refer to the second link
> > above).
> > >
> > > Fixes: 56fb10585934 ("scsi: storvsc: Add the support of hibernation")
> > > Signed-off-by: Dexuan Cui <decui@microsoft.com>
> > > ---
> > > drivers/scsi/storvsc_drv.c | 10 ++++++++++
> > > 1 file changed, 10 insertions(+)
> > >
> > > diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> > > index fb41636519ee..fd51d2f03778 100644
> > > --- a/drivers/scsi/storvsc_drv.c
> > > +++ b/drivers/scsi/storvsc_drv.c
> > > @@ -1948,6 +1948,11 @@ static int storvsc_suspend(struct hv_device
> > *hv_dev)
> > > struct storvsc_device *stor_device = hv_get_drvdata(hv_dev);
> > > struct Scsi_Host *host = stor_device->host;
> > > struct hv_host_device *host_dev = shost_priv(host);
> > > + int ret;
> > > +
> > > + ret = scsi_host_block(host);
> > > + if (ret)
> > > + return ret;
> > >
> > > storvsc_wait_to_drain(stor_device);
> > >
> > > @@ -1968,10 +1973,15 @@ static int storvsc_suspend(struct hv_device
> > *hv_dev)
> > >
> > > static int storvsc_resume(struct hv_device *hv_dev)
> > > {
> > > + struct storvsc_device *stor_device = hv_get_drvdata(hv_dev);
> > > + struct Scsi_Host *host = stor_device->host;
> > > int ret;
> > >
> > > ret = storvsc_connect_to_vsp(hv_dev, storvsc_ringbuffer_size,
> > > hv_dev_is_fc(hv_dev));
> > > + if (!ret)
> > > + ret = scsi_host_unblock(host, SDEV_RUNNING);
> > > +
> > > return ret;
> > > }
> >
> > scsi_host_block() is actually too heavy for just avoiding
> > scsi internal command, which can be done simply by one atomic
> > variable.
> >
> > Not mention scsi_host_block() is implemented too clumsy because
> > nr_luns * synchronize_rcu() are required in scsi_host_block(),
> > which should have been optimized to just one.
> >
> > Also scsi_device_quiesce() is heavy too, still takes 2
> > synchronize_rcu() for one LUN.
> >
> > That is said SCSI suspend may take (3 * nr_luns) sysnchronize_rcu() in
> > case that the HBA's suspend handler needs scsi_host_block().
> >
> > Thanks,
> > Ming
>
> When we're in storvsc_suspend(), all the userspace processes have been
> frozen and all the file systems have been flushed, and there should not
> be too much I/O from the kernel space, so IMO scsi_host_block() should be
> pretty fast here.
Forget to mention, I guess that storvsc_suspend() may be called from runtime
PM path, at that time, the whole system is running, and RCU can't be quick.
Thanks,
Ming
^ permalink raw reply
* Re: [PATCH] scsi: storvsc: Fix a panic in the hibernation procedure
From: Paul E. McKenney @ 2020-04-22 3:08 UTC (permalink / raw)
To: Ming Lei
Cc: Dexuan Cui, Josh Triplett, Rafael J. Wysocki, jejb@linux.ibm.com,
martin.petersen@oracle.com, linux-scsi@vger.kernel.org,
linux-kernel@vger.kernel.org, hch@lst.de, bvanassche@acm.org,
hare@suse.de, Michael Kelley, Long Li,
linux-hyperv@vger.kernel.org, wei.liu@kernel.org,
Stephen Hemminger, Haiyang Zhang, KY Srinivasan, linux-pm
In-Reply-To: <20200422020134.GC299948@T590>
On Wed, Apr 22, 2020 at 10:01:34AM +0800, Ming Lei wrote:
> On Wed, Apr 22, 2020 at 01:48:25AM +0000, Dexuan Cui wrote:
> > > From: Ming Lei <ming.lei@redhat.com>
> > > Sent: Tuesday, April 21, 2020 6:28 PM
> > > To: Dexuan Cui <decui@microsoft.com>
> > >
> > > On Tue, Apr 21, 2020 at 05:17:24PM -0700, Dexuan Cui wrote:
> > > > During hibernation, the sdevs are suspended automatically in
> > > > drivers/scsi/scsi_pm.c before storvsc_suspend(), so after
> > > > storvsc_suspend(), there is no disk I/O from the file systems, but there
> > > > can still be disk I/O from the kernel space, e.g. disk_check_events() ->
> > > > sr_block_check_events() -> cdrom_check_events() can still submit I/O
> > > > to the storvsc driver, which causes a paic of NULL pointer dereference,
> > > > since storvsc has closed the vmbus channel in storvsc_suspend(): refer
> > > > to the below links for more info:
> > > >
> > > > Fix the panic by blocking/unblocking all the I/O queues properly.
> > > >
> > > > Note: this patch depends on another patch "scsi: core: Allow the state
> > > > change from SDEV_QUIESCE to SDEV_BLOCK" (refer to the second link
> > > above).
> > > >
> > > > Fixes: 56fb10585934 ("scsi: storvsc: Add the support of hibernation")
> > > > Signed-off-by: Dexuan Cui <decui@microsoft.com>
> > > > ---
> > > > drivers/scsi/storvsc_drv.c | 10 ++++++++++
> > > > 1 file changed, 10 insertions(+)
> > > >
> > > > diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> > > > index fb41636519ee..fd51d2f03778 100644
> > > > --- a/drivers/scsi/storvsc_drv.c
> > > > +++ b/drivers/scsi/storvsc_drv.c
> > > > @@ -1948,6 +1948,11 @@ static int storvsc_suspend(struct hv_device
> > > *hv_dev)
> > > > struct storvsc_device *stor_device = hv_get_drvdata(hv_dev);
> > > > struct Scsi_Host *host = stor_device->host;
> > > > struct hv_host_device *host_dev = shost_priv(host);
> > > > + int ret;
> > > > +
> > > > + ret = scsi_host_block(host);
> > > > + if (ret)
> > > > + return ret;
> > > >
> > > > storvsc_wait_to_drain(stor_device);
> > > >
> > > > @@ -1968,10 +1973,15 @@ static int storvsc_suspend(struct hv_device
> > > *hv_dev)
> > > >
> > > > static int storvsc_resume(struct hv_device *hv_dev)
> > > > {
> > > > + struct storvsc_device *stor_device = hv_get_drvdata(hv_dev);
> > > > + struct Scsi_Host *host = stor_device->host;
> > > > int ret;
> > > >
> > > > ret = storvsc_connect_to_vsp(hv_dev, storvsc_ringbuffer_size,
> > > > hv_dev_is_fc(hv_dev));
> > > > + if (!ret)
> > > > + ret = scsi_host_unblock(host, SDEV_RUNNING);
> > > > +
> > > > return ret;
> > > > }
> > >
> > > scsi_host_block() is actually too heavy for just avoiding
> > > scsi internal command, which can be done simply by one atomic
> > > variable.
> > >
> > > Not mention scsi_host_block() is implemented too clumsy because
> > > nr_luns * synchronize_rcu() are required in scsi_host_block(),
> > > which should have been optimized to just one.
> > >
> > > Also scsi_device_quiesce() is heavy too, still takes 2
> > > synchronize_rcu() for one LUN.
> > >
> > > That is said SCSI suspend may take (3 * nr_luns) sysnchronize_rcu() in
> > > case that the HBA's suspend handler needs scsi_host_block().
> > >
> > > Thanks,
> > > Ming
> >
> > When we're in storvsc_suspend(), all the userspace processes have been
> > frozen and all the file systems have been flushed, and there should not
> > be too much I/O from the kernel space, so IMO scsi_host_block() should be
> > pretty fast here.
>
> I guess it depends on RCU's implementation, so CC RCU guys.
>
> Hello Paul & Josh,
>
> Could you clarify that if sysnchronize_rcu becomes quickly during
> system suspend?
Once you have all but one CPU offlined, it becomes extremely fast, as
in roughly a no-op (which is an idea of Josh's from back in the day).
But if there is more than one CPU online, then synchronize_rcu() still
takes on the order of several to several tens of jiffies.
So, yes, in some portions of system suspend, synchronize_rcu() becomes
very fast indeed.
Thanx, Paul
^ permalink raw reply
* Re: [PATCH] scsi: storvsc: Fix a panic in the hibernation procedure
From: Ming Lei @ 2020-04-22 4:16 UTC (permalink / raw)
To: Paul E. McKenney
Cc: Dexuan Cui, Josh Triplett, Rafael J. Wysocki, jejb@linux.ibm.com,
martin.petersen@oracle.com, linux-scsi@vger.kernel.org,
linux-kernel@vger.kernel.org, hch@lst.de, bvanassche@acm.org,
hare@suse.de, Michael Kelley, Long Li,
linux-hyperv@vger.kernel.org, wei.liu@kernel.org,
Stephen Hemminger, Haiyang Zhang, KY Srinivasan, linux-pm
In-Reply-To: <20200422030807.GK17661@paulmck-ThinkPad-P72>
On Tue, Apr 21, 2020 at 08:08:07PM -0700, Paul E. McKenney wrote:
> On Wed, Apr 22, 2020 at 10:01:34AM +0800, Ming Lei wrote:
> > On Wed, Apr 22, 2020 at 01:48:25AM +0000, Dexuan Cui wrote:
> > > > From: Ming Lei <ming.lei@redhat.com>
> > > > Sent: Tuesday, April 21, 2020 6:28 PM
> > > > To: Dexuan Cui <decui@microsoft.com>
> > > >
> > > > On Tue, Apr 21, 2020 at 05:17:24PM -0700, Dexuan Cui wrote:
> > > > > During hibernation, the sdevs are suspended automatically in
> > > > > drivers/scsi/scsi_pm.c before storvsc_suspend(), so after
> > > > > storvsc_suspend(), there is no disk I/O from the file systems, but there
> > > > > can still be disk I/O from the kernel space, e.g. disk_check_events() ->
> > > > > sr_block_check_events() -> cdrom_check_events() can still submit I/O
> > > > > to the storvsc driver, which causes a paic of NULL pointer dereference,
> > > > > since storvsc has closed the vmbus channel in storvsc_suspend(): refer
> > > > > to the below links for more info:
> > > > >
> > > > > Fix the panic by blocking/unblocking all the I/O queues properly.
> > > > >
> > > > > Note: this patch depends on another patch "scsi: core: Allow the state
> > > > > change from SDEV_QUIESCE to SDEV_BLOCK" (refer to the second link
> > > > above).
> > > > >
> > > > > Fixes: 56fb10585934 ("scsi: storvsc: Add the support of hibernation")
> > > > > Signed-off-by: Dexuan Cui <decui@microsoft.com>
> > > > > ---
> > > > > drivers/scsi/storvsc_drv.c | 10 ++++++++++
> > > > > 1 file changed, 10 insertions(+)
> > > > >
> > > > > diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> > > > > index fb41636519ee..fd51d2f03778 100644
> > > > > --- a/drivers/scsi/storvsc_drv.c
> > > > > +++ b/drivers/scsi/storvsc_drv.c
> > > > > @@ -1948,6 +1948,11 @@ static int storvsc_suspend(struct hv_device
> > > > *hv_dev)
> > > > > struct storvsc_device *stor_device = hv_get_drvdata(hv_dev);
> > > > > struct Scsi_Host *host = stor_device->host;
> > > > > struct hv_host_device *host_dev = shost_priv(host);
> > > > > + int ret;
> > > > > +
> > > > > + ret = scsi_host_block(host);
> > > > > + if (ret)
> > > > > + return ret;
> > > > >
> > > > > storvsc_wait_to_drain(stor_device);
> > > > >
> > > > > @@ -1968,10 +1973,15 @@ static int storvsc_suspend(struct hv_device
> > > > *hv_dev)
> > > > >
> > > > > static int storvsc_resume(struct hv_device *hv_dev)
> > > > > {
> > > > > + struct storvsc_device *stor_device = hv_get_drvdata(hv_dev);
> > > > > + struct Scsi_Host *host = stor_device->host;
> > > > > int ret;
> > > > >
> > > > > ret = storvsc_connect_to_vsp(hv_dev, storvsc_ringbuffer_size,
> > > > > hv_dev_is_fc(hv_dev));
> > > > > + if (!ret)
> > > > > + ret = scsi_host_unblock(host, SDEV_RUNNING);
> > > > > +
> > > > > return ret;
> > > > > }
> > > >
> > > > scsi_host_block() is actually too heavy for just avoiding
> > > > scsi internal command, which can be done simply by one atomic
> > > > variable.
> > > >
> > > > Not mention scsi_host_block() is implemented too clumsy because
> > > > nr_luns * synchronize_rcu() are required in scsi_host_block(),
> > > > which should have been optimized to just one.
> > > >
> > > > Also scsi_device_quiesce() is heavy too, still takes 2
> > > > synchronize_rcu() for one LUN.
> > > >
> > > > That is said SCSI suspend may take (3 * nr_luns) sysnchronize_rcu() in
> > > > case that the HBA's suspend handler needs scsi_host_block().
> > > >
> > > > Thanks,
> > > > Ming
> > >
> > > When we're in storvsc_suspend(), all the userspace processes have been
> > > frozen and all the file systems have been flushed, and there should not
> > > be too much I/O from the kernel space, so IMO scsi_host_block() should be
> > > pretty fast here.
> >
> > I guess it depends on RCU's implementation, so CC RCU guys.
> >
> > Hello Paul & Josh,
> >
> > Could you clarify that if sysnchronize_rcu becomes quickly during
> > system suspend?
>
> Once you have all but one CPU offlined, it becomes extremely fast, as
> in roughly a no-op (which is an idea of Josh's from back in the day).
> But if there is more than one CPU online, then synchronize_rcu() still
> takes on the order of several to several tens of jiffies.
>
> So, yes, in some portions of system suspend, synchronize_rcu() becomes
> very fast indeed.
Hi Paul,
Thanks for your clarification.
In system suspend path, device is suspended before suspend_disable_secondary_cpus(),
so I guess synchronize_rcu() is not quick enough even though user space
processes and some kernel threads are frozen.
Thanks,
Ming
^ permalink raw reply
* RE: [PATCH] scsi: storvsc: Fix a panic in the hibernation procedure
From: Dexuan Cui @ 2020-04-22 4:58 UTC (permalink / raw)
To: Ming Lei, Paul E. McKenney
Cc: Josh Triplett, Rafael J. Wysocki, jejb@linux.ibm.com,
martin.petersen@oracle.com, linux-scsi@vger.kernel.org,
linux-kernel@vger.kernel.org, hch@lst.de, bvanassche@acm.org,
hare@suse.de, Michael Kelley, Long Li,
linux-hyperv@vger.kernel.org, wei.liu@kernel.org,
Stephen Hemminger, Haiyang Zhang, KY Srinivasan,
linux-pm@vger.kernel.org
In-Reply-To: <20200422041629.GE299948@T590>
> From: Ming Lei <ming.lei@redhat.com>
> Sent: Tuesday, April 21, 2020 9:16 PM
> ...
> > > > When we're in storvsc_suspend(), all the userspace processes have been
> > > > frozen and all the file systems have been flushed, and there should not
> > > > be too much I/O from the kernel space, so IMO scsi_host_block() should
> be
> > > > pretty fast here.
> > >
> > > I guess it depends on RCU's implementation, so CC RCU guys.
> > >
> > > Hello Paul & Josh,
> > >
> > > Could you clarify that if sysnchronize_rcu becomes quickly during
> > > system suspend?
> >
> > Once you have all but one CPU offlined, it becomes extremely fast, as
> > in roughly a no-op (which is an idea of Josh's from back in the day).
> > But if there is more than one CPU online, then synchronize_rcu() still
> > takes on the order of several to several tens of jiffies.
> >
> > So, yes, in some portions of system suspend, synchronize_rcu() becomes
> > very fast indeed.
>
> Hi Paul,
>
> Thanks for your clarification.
>
> In system suspend path, device is suspended before
> suspend_disable_secondary_cpus(),
> so I guess synchronize_rcu() is not quick enough even though user space
> processes and some kernel threads are frozen.
>
> Thanks,
> Ming
storvsc_suspend() -> scsi_host_block() is only called in the hibernation
path, which is not a hot path at all, so IMHO we don't really care if it
takes 10ms or 100ms or even 1s. :-) BTW, in my test, typically the
scsi_host_block() here takes about 3ms in my 40-vCPU VM.
storvsc_suspend() is not called from the runtime PM path, because the
runtime_suspend/runtime_resume/runtime_idle ops are not defined
at all for the devices on the Hyper-V VMBus bus: these are pure
software-emulated devices, so runtime PM is unnecessary for them.
Thanks,
-- Dexuan
^ permalink raw reply
* Re: [PATCH] scsi: storvsc: Fix a panic in the hibernation procedure
From: Bart Van Assche @ 2020-04-22 5:02 UTC (permalink / raw)
To: decui, jejb, martin.petersen, linux-scsi, linux-kernel, hch, hare,
mikelley, longli, ming.lei
Cc: linux-hyperv, wei.liu, sthemmin, haiyangz, kys
In-Reply-To: <1587514644-47058-1-git-send-email-decui@microsoft.com>
On 4/21/20 5:17 PM, Dexuan Cui wrote:
> During hibernation, the sdevs are suspended automatically in
> drivers/scsi/scsi_pm.c before storvsc_suspend(), so after
> storvsc_suspend(), there is no disk I/O from the file systems, but there
> can still be disk I/O from the kernel space, e.g. disk_check_events() ->
> sr_block_check_events() -> cdrom_check_events() can still submit I/O
> to the storvsc driver, which causes a paic of NULL pointer dereference,
> since storvsc has closed the vmbus channel in storvsc_suspend(): refer
> to the below links for more info:
> https://lkml.org/lkml/2020/4/10/47
> https://lkml.org/lkml/2020/4/17/1103
>
> Fix the panic by blocking/unblocking all the I/O queues properly.
>
> Note: this patch depends on another patch "scsi: core: Allow the state
> change from SDEV_QUIESCE to SDEV_BLOCK" (refer to the second link above).
>
> Fixes: 56fb10585934 ("scsi: storvsc: Add the support of hibernation")
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
> drivers/scsi/storvsc_drv.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index fb41636519ee..fd51d2f03778 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -1948,6 +1948,11 @@ static int storvsc_suspend(struct hv_device *hv_dev)
> struct storvsc_device *stor_device = hv_get_drvdata(hv_dev);
> struct Scsi_Host *host = stor_device->host;
> struct hv_host_device *host_dev = shost_priv(host);
> + int ret;
> +
> + ret = scsi_host_block(host);
> + if (ret)
> + return ret;
>
> storvsc_wait_to_drain(stor_device);
>
> @@ -1968,10 +1973,15 @@ static int storvsc_suspend(struct hv_device *hv_dev)
>
> static int storvsc_resume(struct hv_device *hv_dev)
> {
> + struct storvsc_device *stor_device = hv_get_drvdata(hv_dev);
> + struct Scsi_Host *host = stor_device->host;
> int ret;
>
> ret = storvsc_connect_to_vsp(hv_dev, storvsc_ringbuffer_size,
> hv_dev_is_fc(hv_dev));
> + if (!ret)
> + ret = scsi_host_unblock(host, SDEV_RUNNING);
> +
> return ret;
> }
I don't like this patch. It makes the behavior of the storsvc driver
different from every other SCSI LLD. Other SCSI LLDs don't need this
change because these don't destroy I/O queues upon suspend.
Bart.
^ permalink raw reply
* RE: [PATCH] scsi: storvsc: Fix a panic in the hibernation procedure
From: Dexuan Cui @ 2020-04-22 6:24 UTC (permalink / raw)
To: Bart Van Assche, jejb@linux.ibm.com, martin.petersen@oracle.com,
linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org,
hch@lst.de, hare@suse.de, Michael Kelley, Long Li,
ming.lei@redhat.com, Hannes Reinecke, Balsundar P,
Christoph Hellwig
Cc: linux-hyperv@vger.kernel.org, wei.liu@kernel.org,
Stephen Hemminger, Haiyang Zhang, KY Srinivasan
In-Reply-To: <1b6de3b0-4e0c-4b46-df1a-db531bd2c888@acm.org>
> From: Bart Van Assche <bvanassche@acm.org>
> Sent: Tuesday, April 21, 2020 10:02 PM
> On 4/21/20 5:17 PM, Dexuan Cui wrote:
> > During hibernation, the sdevs are suspended automatically in
> > drivers/scsi/scsi_pm.c before storvsc_suspend(), so after
> > storvsc_suspend(), there is no disk I/O from the file systems, but there
> > can still be disk I/O from the kernel space, e.g. disk_check_events() ->
> > sr_block_check_events() -> cdrom_check_events() can still submit I/O
> > to the storvsc driver, which causes a panic of NULL pointer dereference,
> > since storvsc has closed the vmbus channel in storvsc_suspend(): refer
> > to the below links for more info:
> > ...
> > Fix the panic by blocking/unblocking all the I/O queues properly.
> >
> > Note: this patch depends on another patch "scsi: core: Allow the state
> > change from SDEV_QUIESCE to SDEV_BLOCK" (refer to the second link
> above).
>
> I don't like this patch. It makes the behavior of the storsvc driver
> different from every other SCSI LLD. Other SCSI LLDs don't need this
> change because these don't destroy I/O queues upon suspend.
>
> Bart.
Upon suspend, I suppose the other LLDs can not accept I/O either, then
what do they do when some kernel thread still tries to submit I/O? Do
they "block" the I/O until resume, or just return an error immediately?
I had a look at drivers/scsi/xen-scsifront.c. It looks this LLD implements
a mechanism of marking the device busy upon suspend, so any I/O will
immediately get an error SCSI_MLQUEUE_HOST_BUSY. IMO the
disadvantage is: the mechanism of marking the device busy looks
complex, and the hot path .queuecommand() has to take the
spinlock shost->host_lock, which should affect the performance.
It looks drivers/scsi/nsp32.c: nsp32_suspend() and
drivers/scsi/3w-9xxx.c: twa_suspend() do nothing to handle new I/O
after suspend. I doubt this is correct.
It looks drivers/scsi/hisi_sas/hisi_sas_v3_hw.c: hisi_sas_v3_suspend()
tries to use scsi_block_requests() as a means of blocking new I/O, but
I doubt it can work: scsi_block_requests() only sets a variable --
how could this prevent another concurrent kernel thread to submit new
I/O without a race condition issue?
So it looks to me there is no simple mechanism to handle the scenario
here, and I guess that's why the scsi_host_block/unblock APIs are
introduced, and actually there is already an user of the APIs:
3d3ca53b1639 ("scsi: aacraid: use scsi_host_(block,unblock) to block I/O").
The aacraid patch says: "This has the advantage that the block layer will
stop sending I/O to the adapter instead of having the SCSI midlayer
requeueing I/O internally". It looks this may imply that using the new
APIs is encouraged?
Sorry for my lack of knowledge in Linux SCSI system, but IMHO the new
APIs are simple and effective, and they really look like a good fit for the
hibernation scenario. BTW, storvsc_suspend() is not a hot path here,
so IMO I don't have any performance concern.
PS, here storvsc has to destroy and re-construct the I/O queues: the
I/O queues are shared memory ringbufers between the guest and the
host; in the resume path of the hibernation procedure, the memory
pages allocated by the 'new' kernel is different from that allocated by
the 'old' kernel, so before jumping to the 'old' kernel, the 'new' kernel
must destroy the mapping of the pages, and later after we jump to
the 'old' kernel, we'll re-create the mapping using the pages allocated
by the 'old' kernel. Here "create the mapping" means the guest tells
the host about the physical addresses of the pages.
Thanks,
-- Dexuan
^ permalink raw reply
* Re: [PATCH] scsi: storvsc: Fix a panic in the hibernation procedure
From: Ming Lei @ 2020-04-22 9:23 UTC (permalink / raw)
To: Dexuan Cui
Cc: Paul E. McKenney, Josh Triplett, Rafael J. Wysocki,
jejb@linux.ibm.com, martin.petersen@oracle.com,
linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org,
hch@lst.de, bvanassche@acm.org, hare@suse.de, Michael Kelley,
Long Li, linux-hyperv@vger.kernel.org, wei.liu@kernel.org,
Stephen Hemminger, Haiyang Zhang, KY Srinivasan,
linux-pm@vger.kernel.org
In-Reply-To: <HK0P153MB0273CF2901E193C03C934A47BFD20@HK0P153MB0273.APCP153.PROD.OUTLOOK.COM>
On Wed, Apr 22, 2020 at 04:58:14AM +0000, Dexuan Cui wrote:
> > From: Ming Lei <ming.lei@redhat.com>
> > Sent: Tuesday, April 21, 2020 9:16 PM
> > ...
> > > > > When we're in storvsc_suspend(), all the userspace processes have been
> > > > > frozen and all the file systems have been flushed, and there should not
> > > > > be too much I/O from the kernel space, so IMO scsi_host_block() should
> > be
> > > > > pretty fast here.
> > > >
> > > > I guess it depends on RCU's implementation, so CC RCU guys.
> > > >
> > > > Hello Paul & Josh,
> > > >
> > > > Could you clarify that if sysnchronize_rcu becomes quickly during
> > > > system suspend?
> > >
> > > Once you have all but one CPU offlined, it becomes extremely fast, as
> > > in roughly a no-op (which is an idea of Josh's from back in the day).
> > > But if there is more than one CPU online, then synchronize_rcu() still
> > > takes on the order of several to several tens of jiffies.
> > >
> > > So, yes, in some portions of system suspend, synchronize_rcu() becomes
> > > very fast indeed.
> >
> > Hi Paul,
> >
> > Thanks for your clarification.
> >
> > In system suspend path, device is suspended before
> > suspend_disable_secondary_cpus(),
> > so I guess synchronize_rcu() is not quick enough even though user space
> > processes and some kernel threads are frozen.
> >
> > Thanks,
> > Ming
>
> storvsc_suspend() -> scsi_host_block() is only called in the hibernation
> path, which is not a hot path at all, so IMHO we don't really care if it
> takes 10ms or 100ms or even 1s. :-) BTW, in my test, typically the
Are you sure the 'we' can cover all users?
> scsi_host_block() here takes about 3ms in my 40-vCPU VM.
If more LUNs are added, the time should be increased proportionallly,
that is why I think scsi_host_block() is bad.
Thanks,
Ming
^ permalink raw reply
* [PATCH v1] hyper-v: Use UUID API for exporting the GUID
From: Andy Shevchenko @ 2020-04-22 12:59 UTC (permalink / raw)
To: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
linux-hyperv
Cc: Andy Shevchenko
There is export_guid() function which exports guid_t to the u8 array.
Use it instead of open coding variant.
This allows to hide the uuid_t internals.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/hv/hv_trace.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hv/hv_trace.h b/drivers/hv/hv_trace.h
index a43bc76c2d5d0..579d19bdc0981 100644
--- a/drivers/hv/hv_trace.h
+++ b/drivers/hv/hv_trace.h
@@ -286,8 +286,8 @@ TRACE_EVENT(vmbus_send_tl_connect_request,
__field(int, ret)
),
TP_fast_assign(
- memcpy(__entry->guest_id, &msg->guest_endpoint_id.b, 16);
- memcpy(__entry->host_id, &msg->host_service_id.b, 16);
+ export_guid(__entry->guest_id, &msg->guest_endpoint_id);
+ export_guid(__entry->host_id, &msg->host_service_id);
__entry->ret = ret;
),
TP_printk("sending guest_endpoint_id %pUl, host_service_id %pUl, "
--
2.26.1
^ permalink raw reply related
* [PATCH v1] hyper-v: Remove internal types from UAPI header
From: Andy Shevchenko @ 2020-04-22 13:18 UTC (permalink / raw)
To: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
linux-hyperv
Cc: Andy Shevchenko
The uuid_le mistakenly comes to be an UAPI type. Since it's luckily not used by
Hyper-V APIs, we may replace with POD types, i.e. __u8 array.
Note, previously shared uuid_be had been removed from UAPI few releases ago.
This is a continuation of that process towards removing uuid_le one.
Note, there is no ABI change!
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
include/uapi/linux/hyperv.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/hyperv.h b/include/uapi/linux/hyperv.h
index 991b2b7ada7a3..8f24404ad04f1 100644
--- a/include/uapi/linux/hyperv.h
+++ b/include/uapi/linux/hyperv.h
@@ -119,8 +119,8 @@ enum hv_fcopy_op {
struct hv_fcopy_hdr {
__u32 operation;
- uuid_le service_id0; /* currently unused */
- uuid_le service_id1; /* currently unused */
+ __u8 service_id0[16]; /* currently unused */
+ __u8 service_id1[16]; /* currently unused */
} __attribute__((packed));
#define OVER_WRITE 0x1
--
2.26.1
^ permalink raw reply related
* Re: [PATCH v1] hyper-v: Use UUID API for exporting the GUID
From: Wei Liu @ 2020-04-22 13:28 UTC (permalink / raw)
To: Andy Shevchenko
Cc: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
linux-hyperv
In-Reply-To: <20200422125937.38355-1-andriy.shevchenko@linux.intel.com>
On Wed, Apr 22, 2020 at 03:59:37PM +0300, Andy Shevchenko wrote:
> There is export_guid() function which exports guid_t to the u8 array.
> Use it instead of open coding variant.
>
> This allows to hide the uuid_t internals.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> drivers/hv/hv_trace.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hv/hv_trace.h b/drivers/hv/hv_trace.h
> index a43bc76c2d5d0..579d19bdc0981 100644
> --- a/drivers/hv/hv_trace.h
> +++ b/drivers/hv/hv_trace.h
> @@ -286,8 +286,8 @@ TRACE_EVENT(vmbus_send_tl_connect_request,
> __field(int, ret)
> ),
> TP_fast_assign(
> - memcpy(__entry->guest_id, &msg->guest_endpoint_id.b, 16);
> - memcpy(__entry->host_id, &msg->host_service_id.b, 16);
> + export_guid(__entry->guest_id, &msg->guest_endpoint_id);
> + export_guid(__entry->host_id, &msg->host_service_id);
I was about to say I couldn't find this function but it seems to have
been introduced in d01cd62400b which is currently in Linus' master
branch. That's fine then.
I will queue this up to hyperv-fixes for 5.7.
Wei.
> __entry->ret = ret;
> ),
> TP_printk("sending guest_endpoint_id %pUl, host_service_id %pUl, "
> --
> 2.26.1
>
^ permalink raw reply
* Re: [PATCH v1] hyper-v: Remove internal types from UAPI header
From: Wei Liu @ 2020-04-22 13:41 UTC (permalink / raw)
To: Andy Shevchenko
Cc: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
linux-hyperv
In-Reply-To: <20200422131818.23088-1-andriy.shevchenko@linux.intel.com>
On Wed, Apr 22, 2020 at 04:18:18PM +0300, Andy Shevchenko wrote:
> The uuid_le mistakenly comes to be an UAPI type. Since it's luckily not used by
> Hyper-V APIs, we may replace with POD types, i.e. __u8 array.
>
> Note, previously shared uuid_be had been removed from UAPI few releases ago.
> This is a continuation of that process towards removing uuid_le one.
>
> Note, there is no ABI change!
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Can you clarify why guid_t is not used instead? Is the plan to remove it
from UAPI as well?
Wei.
> ---
> include/uapi/linux/hyperv.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/uapi/linux/hyperv.h b/include/uapi/linux/hyperv.h
> index 991b2b7ada7a3..8f24404ad04f1 100644
> --- a/include/uapi/linux/hyperv.h
> +++ b/include/uapi/linux/hyperv.h
> @@ -119,8 +119,8 @@ enum hv_fcopy_op {
>
> struct hv_fcopy_hdr {
> __u32 operation;
> - uuid_le service_id0; /* currently unused */
> - uuid_le service_id1; /* currently unused */
> + __u8 service_id0[16]; /* currently unused */
> + __u8 service_id1[16]; /* currently unused */
> } __attribute__((packed));
>
> #define OVER_WRITE 0x1
> --
> 2.26.1
>
^ permalink raw reply
* Re: [PATCH v1] hyper-v: Remove internal types from UAPI header
From: Andy Shevchenko @ 2020-04-22 14:15 UTC (permalink / raw)
To: Wei Liu; +Cc: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, linux-hyperv
In-Reply-To: <20200422134127.zgsympiwgvp7hdam@debian>
On Wed, Apr 22, 2020 at 02:41:27PM +0100, Wei Liu wrote:
> On Wed, Apr 22, 2020 at 04:18:18PM +0300, Andy Shevchenko wrote:
> > The uuid_le mistakenly comes to be an UAPI type. Since it's luckily not used by
> > Hyper-V APIs, we may replace with POD types, i.e. __u8 array.
> >
> > Note, previously shared uuid_be had been removed from UAPI few releases ago.
> > This is a continuation of that process towards removing uuid_le one.
> >
> > Note, there is no ABI change!
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> Can you clarify why guid_t is not used instead?
It's internal type to the kernel. The libuuid or so should provide a compatible
type(s) for user space.
> Is the plan to remove it
> from UAPI as well?
Yes.
> Wei.
>
> > ---
> > include/uapi/linux/hyperv.h | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/uapi/linux/hyperv.h b/include/uapi/linux/hyperv.h
> > index 991b2b7ada7a3..8f24404ad04f1 100644
> > --- a/include/uapi/linux/hyperv.h
> > +++ b/include/uapi/linux/hyperv.h
> > @@ -119,8 +119,8 @@ enum hv_fcopy_op {
> >
> > struct hv_fcopy_hdr {
> > __u32 operation;
> > - uuid_le service_id0; /* currently unused */
> > - uuid_le service_id1; /* currently unused */
> > + __u8 service_id0[16]; /* currently unused */
> > + __u8 service_id1[16]; /* currently unused */
> > } __attribute__((packed));
> >
> > #define OVER_WRITE 0x1
> > --
> > 2.26.1
> >
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH] scsi: storvsc: Fix a panic in the hibernation procedure
From: Paul E. McKenney @ 2020-04-22 16:19 UTC (permalink / raw)
To: Ming Lei
Cc: Dexuan Cui, Josh Triplett, Rafael J. Wysocki, jejb@linux.ibm.com,
martin.petersen@oracle.com, linux-scsi@vger.kernel.org,
linux-kernel@vger.kernel.org, hch@lst.de, bvanassche@acm.org,
hare@suse.de, Michael Kelley, Long Li,
linux-hyperv@vger.kernel.org, wei.liu@kernel.org,
Stephen Hemminger, Haiyang Zhang, KY Srinivasan,
linux-pm@vger.kernel.org
In-Reply-To: <20200422092351.GF299948@T590>
On Wed, Apr 22, 2020 at 05:23:51PM +0800, Ming Lei wrote:
> On Wed, Apr 22, 2020 at 04:58:14AM +0000, Dexuan Cui wrote:
> > > From: Ming Lei <ming.lei@redhat.com>
> > > Sent: Tuesday, April 21, 2020 9:16 PM
> > > ...
> > > > > > When we're in storvsc_suspend(), all the userspace processes have been
> > > > > > frozen and all the file systems have been flushed, and there should not
> > > > > > be too much I/O from the kernel space, so IMO scsi_host_block() should
> > > be
> > > > > > pretty fast here.
> > > > >
> > > > > I guess it depends on RCU's implementation, so CC RCU guys.
> > > > >
> > > > > Hello Paul & Josh,
> > > > >
> > > > > Could you clarify that if sysnchronize_rcu becomes quickly during
> > > > > system suspend?
> > > >
> > > > Once you have all but one CPU offlined, it becomes extremely fast, as
> > > > in roughly a no-op (which is an idea of Josh's from back in the day).
> > > > But if there is more than one CPU online, then synchronize_rcu() still
> > > > takes on the order of several to several tens of jiffies.
> > > >
> > > > So, yes, in some portions of system suspend, synchronize_rcu() becomes
> > > > very fast indeed.
> > >
> > > Hi Paul,
> > >
> > > Thanks for your clarification.
> > >
> > > In system suspend path, device is suspended before
> > > suspend_disable_secondary_cpus(),
> > > so I guess synchronize_rcu() is not quick enough even though user space
> > > processes and some kernel threads are frozen.
> > >
> > > Thanks,
> > > Ming
> >
> > storvsc_suspend() -> scsi_host_block() is only called in the hibernation
> > path, which is not a hot path at all, so IMHO we don't really care if it
> > takes 10ms or 100ms or even 1s. :-) BTW, in my test, typically the
>
> Are you sure the 'we' can cover all users?
>
> > scsi_host_block() here takes about 3ms in my 40-vCPU VM.
>
> If more LUNs are added, the time should be increased proportionallly,
> that is why I think scsi_host_block() is bad.
If the caller must wait until the grace period ends, then the traditional
approach is to use a single synchronize_rcu() to cover all LUNs. This of
course can require some reworking of the code.
If the caller does not need to wait, then either call_rcu() or kfree_rcu()
can work quite well.
Thanx, Paul
^ permalink raw reply
* RE: [PATCH] scsi: storvsc: Fix a panic in the hibernation procedure
From: Dexuan Cui @ 2020-04-22 18:01 UTC (permalink / raw)
To: Ming Lei
Cc: Paul E. McKenney, Josh Triplett, Rafael J. Wysocki,
jejb@linux.ibm.com, martin.petersen@oracle.com,
linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org,
hch@lst.de, bvanassche@acm.org, hare@suse.de, Michael Kelley,
Long Li, linux-hyperv@vger.kernel.org, wei.liu@kernel.org,
Stephen Hemminger, Haiyang Zhang, KY Srinivasan,
linux-pm@vger.kernel.org
In-Reply-To: <20200422092351.GF299948@T590>
> From: Ming Lei <ming.lei@redhat.com>
> Sent: Wednesday, April 22, 2020 2:24 AM
> > ...
> > storvsc_suspend() -> scsi_host_block() is only called in the hibernation
> > path, which is not a hot path at all, so IMHO we don't really care if it
> > takes 10ms or 100ms or even 1s. :-) BTW, in my test, typically the
>
> Are you sure the 'we' can cover all users?
I actully only meant I don't have any performance concern in this path
storvsc_suspend() -> scsi_host_block() :-)
Thanks for your effort to improve the scsi_host_block() API in the patch
[PATCH] scsi: avoid to run synchronize_rcu for each device in scsi_host_block.
> > scsi_host_block() here takes about 3ms in my 40-vCPU VM.
>
> If more LUNs are added, the time should be increased proportionallly,
> that is why I think scsi_host_block() is bad.
>
> Thanks,
> Ming
FWIW, if the time increases linearly against the number of the LUNs, it
looks typically it's still fast enough for storvsc_suspend().
Thanks,
-- Dexuan
^ permalink raw reply
* Re: [PATCH v1] hyper-v: Remove internal types from UAPI header
From: Wei Liu @ 2020-04-22 19:38 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Wei Liu, K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
linux-hyperv
In-Reply-To: <20200422141507.GI185537@smile.fi.intel.com>
On Wed, Apr 22, 2020 at 05:15:07PM +0300, Andy Shevchenko wrote:
> On Wed, Apr 22, 2020 at 02:41:27PM +0100, Wei Liu wrote:
> > On Wed, Apr 22, 2020 at 04:18:18PM +0300, Andy Shevchenko wrote:
> > > The uuid_le mistakenly comes to be an UAPI type. Since it's luckily not used by
> > > Hyper-V APIs, we may replace with POD types, i.e. __u8 array.
> > >
> > > Note, previously shared uuid_be had been removed from UAPI few releases ago.
> > > This is a continuation of that process towards removing uuid_le one.
> > >
> > > Note, there is no ABI change!
> > >
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >
> > Can you clarify why guid_t is not used instead?
>
> It's internal type to the kernel. The libuuid or so should provide a compatible
> type(s) for user space.
>
> > Is the plan to remove it
> > from UAPI as well?
>
> Yes.
>
OK. Thanks for explaining.
Wei.
> > Wei.
> >
> > > ---
> > > include/uapi/linux/hyperv.h | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/include/uapi/linux/hyperv.h b/include/uapi/linux/hyperv.h
> > > index 991b2b7ada7a3..8f24404ad04f1 100644
> > > --- a/include/uapi/linux/hyperv.h
> > > +++ b/include/uapi/linux/hyperv.h
> > > @@ -119,8 +119,8 @@ enum hv_fcopy_op {
> > >
> > > struct hv_fcopy_hdr {
> > > __u32 operation;
> > > - uuid_le service_id0; /* currently unused */
> > > - uuid_le service_id1; /* currently unused */
> > > + __u8 service_id0[16]; /* currently unused */
> > > + __u8 service_id1[16]; /* currently unused */
> > > } __attribute__((packed));
> > >
> > > #define OVER_WRITE 0x1
> > > --
> > > 2.26.1
> > >
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox