Re: [PATCH RFC 01/14] x86/asm: add iosubmit_cmds512() based on movdir64b CPU instruction

From: Thomas Gleixner <tglx@linutronix.de>
To: "Luck, Tony" <tony.luck@intel.com>
Cc: Borislav Petkov <bp@alien8.de>, Dave Jiang <dave.jiang@intel.com>,
	dmaengine@vger.kernel.org, linux-kernel@vger.kernel.org,
	vkoul@kernel.org, dan.j.williams@intel.com, jing.lin@intel.com,
	ashok.raj@intel.com, sanjay.k.kumar@intel.com,
	megha.dey@intel.com, jacob.jun.pan@intel.com, yi.l.liu@intel.com,
	axboe@kernel.dk, akpm@linux-foundation.org, mingo@redhat.com,
	fenghua.yu@intel.com, hpa@zytor.com
Subject: Re: [PATCH RFC 01/14] x86/asm: add iosubmit_cmds512() based on movdir64b CPU instruction
Date: Thu, 21 Nov 2019 01:22:41 +0100 (CET)	[thread overview]
Message-ID: <alpine.DEB.2.21.1911210120410.29534@nanos.tec.linutronix.de> (raw)
In-Reply-To: <20191120231923.GA32680@agluck-desk2.amr.corp.intel.com>

On Wed, 20 Nov 2019, Luck, Tony wrote:
> On Wed, Nov 20, 2019 at 10:53:39PM +0100, Borislav Petkov wrote:
> > On Wed, Nov 20, 2019 at 02:23:49PM -0700, Dave Jiang wrote:
> > > +static inline void iosubmit_cmds512(void __iomem *dst, const void *src,
> > > +				    size_t count)
> > 
> > An iosubmit function which returns void and doesn't tell its callers
> > whether it succeeded or not? That looks non-optimal to say the least.
> 
> That's the underlying functionality of the MOVDIR64B instruction. A
> posted write so no way to know if it succeeded. When using dedicated
> queues the caller must keep count of how many operations are in flight
> and not send more than the depth of the queue.
> 
> > Why isn't there a fallback function which to call when the CPU doesn't
> > support movdir64b?
> 
> This particular driver has no option for fallback. Descriptors can
> only be submitted with MOVDIR64B (to dedicated queues ... in later
> patch series support for shared queues will be added, but those require
> ENQCMD or ENQCMDS to submit).
> 
> The driver bails out at the beginning of the probe routine if the
> necessary instructions are not supported:
> 
> +       /*
> +        * If the CPU does not support write512, there's no point in
> +        * enumerating the device. We can not utilize it.
> +        */
> +       if (!cpu_has_write512())
> +               return -ENXIO;
> 
> Though we should always get past that as this PCI device ID shouldn't
> every appear on a system that doesn't have the support. Device is on
> the die, not a plug-in card.

Then the condition in the iosubmit function is just prone to silently paper
over any bug in a driver:

> +       if (!cpu_has_write512())
> +               return;

This should at least issue a WARN_ON_ONCE()

Thanks,

	tglx