Linux Documentation
 help / color / mirror / Atom feed
* Re: [PATCH 4.19 72/98] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
From: Borislav Petkov @ 2019-08-28 12:00 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Thomas Gleixner, Greg Kroah-Hartman, linux-kernel, stable,
	Tom Lendacky, Andrew Cooper, Andrew Morton, Chen Yu,
	H. Peter Anvin, Ingo Molnar, Jonathan Corbet, Josh Poimboeuf,
	Juergen Gross, Kees Cook, linux-doc@vger.kernel.org,
	linux-pm@vger.kernel.org, Nathan Chancellor, Paolo Bonzini,
	Rafael J. Wysocki, x86@kernel.org
In-Reply-To: <20190828114947.GC8052@amd>

On Wed, Aug 28, 2019 at 01:49:47PM +0200, Pavel Machek wrote:
> AMD screwed this up,

Except that it wasn't AMD who screwed up but BIOS on *some* laptops.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

^ permalink raw reply

* Re: [PATCH v2] vsprintf: introduce %dE for error constants
From: Jani Nikula @ 2019-08-28 11:54 UTC (permalink / raw)
  To: Petr Mladek, Uwe Kleine-König
  Cc: Sergey Senozhatsky, Steven Rostedt, Enrico, Weigelt,
	Andrew Morton, metux IT consult, Jonathan Corbet, linux-doc,
	linux-kernel
In-Reply-To: <20190828113216.p2yiha4xyupkbcbs@pathway.suse.cz>

On Wed, 28 Aug 2019, Petr Mladek <pmladek@suse.com> wrote:
> On Tue 2019-08-27 23:12:44, Uwe Kleine-König  wrote:
>> Petr Mladek had some concerns:
>> > The array is long, created by cpu&paste, the index of each code
>> > is not obvious.
>> 
>> Yeah right, the array is long. This cannot really be changed because we
>> have that many error codes. I don't understand your concern about the
>> index not being obvious. The array was just a list of (number, string)
>> pairs where the position in the array didn't have any semantic.
>
> I missed that the number was stored in the array as well. I somehow
> expected that it was array of strings.
>
>
>> > There are ideas to make the code even more tricky to reduce
>> > the size, keep it fast.
>> 
>> I think Enrico Weigelt's suggestion to use a case is the best
>> performance-wise so that's what I picked up. Also I hope that
>> performance isn't that important because the need to print an error
>> should not be so common that it really hurts in production.
>
> I personally do not like switch/case. It is a lot of code.
> I wonder if it even saved some space.
>
> If you want to safe space, I would use u16 to store the numbers.
> Or I would use array of strings. There will be only few holes.
>
> You might also consider handling only the most commonly
> used codes from errno.h and errno-base.h (1..133). There will
> be no holes and the codes are stable.
>
>
>> > Both, %dE modifier and the output format (ECODE) is non-standard.
>> 
>> Yeah, obviously right. The problem is that the new modifier does
>> something that wasn't implemented before, so it cannot match any
>> standard. %pI is only known on Linux either, so I think being
>> non-standard is a weak argument.
>
> I am not completely sure that %p modifiers were a good idea.
> They came before I started maintaining printk(). They add more
> complex algorithms into paths where we could not report problems
> easily (printk recursion). Also they are causing problems with
> unit testing that might be done in userspace. These non-standard
> formats cause that printk() can't be simply substituted by printf().
>
> I am not keen to spread these problems over more formats.
> Also %d format is more complicated. It is often used with
> already existing modifiers.
>
>
>> > Upper letters gain a lot of attention. But the error code is
>> > only helper information. Also many error codes are misleading because
>> > they are used either wrongly or there was no better available.
>> 
>> This isn't really an argument against the patch I think. Sure, if a
>> function returned (say) EIO while ETIMEOUT would be better, my patch
>> doesn't improve that detail. Still
>>
>>         mydev: Failed to initialize blablub: EIO
>>
>> is more expressive than
>> 
>>         mydev: Failed to initialize blablub: -5
>
> OK, upper letters probably are not a problem.
>
> But what about EWOULDBLOCK and EDEADLOCK? They have the same
> error codes as EAGAIN and EDEADLK. It might cause a lot of confusion.
> People might spend a lot of time searching for EAGAIN before they
> notice that EWOULDBLOCK was used in the code instead.
>
> Also you still did not answer the question where the idea came from.
> Did it just look nice? Anyone asked for it? Who? Why?
>
>
>> > There is no proof that this approach would be widely acceptable for
>> > subsystem maintainers. Some might not like mass and "blind" code
>> > changes. Some might not like the output at all.
>> 
>> I don't intend to mass convert existing code. I would restrict myself to
>> updating the documentation and then maybe send a patch per subsystem as an
>> example to let maintainers know and judge for themselves if they like it or
>> not. And if it doesn't get picked up, we can just remove the feature again next
>> year (or so).
>
> It looks like a lot of potentially useless work.
>
>
>> I dropped the example conversion, I think the idea should be clear now
>> even without an explicit example.
>
> Please, do the opposite. Add conversion of few subsystems into the
> patchset and add more people into CC. We will see immediately whether
> it makes sense to spend time on this.
>
> I personally think that this feature is not worth the code, data,
> and bikeshedding.

The obvious alternative, I think already mentioned, is to just add
strerror() or similar as a function. I doubt there'd be much opposition
to that. Folks could use %s and strerr(ret). And a follow-up could add
the special format specifier if needed.

BR,
Jani.



-- 
Jani Nikula, Intel Open Source Graphics Center

^ permalink raw reply

* Re: [PATCH 4.19 72/98] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
From: Pavel Machek @ 2019-08-28 11:49 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Pavel Machek, Greg Kroah-Hartman, linux-kernel, stable,
	Tom Lendacky, Borislav Petkov, Andrew Cooper, Andrew Morton,
	Chen Yu, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
	Josh Poimboeuf, Juergen Gross, Kees Cook,
	linux-doc@vger.kernel.org, linux-pm@vger.kernel.org,
	Nathan Chancellor, Paolo Bonzini, Rafael J. Wysocki,
	x86@kernel.org
In-Reply-To: <alpine.DEB.2.21.1908281231480.1869@nanos.tec.linutronix.de>

[-- Attachment #1: Type: text/plain, Size: 1580 bytes --]

Hi!

> > > There is no way to reinitialize RDRAND from the kernel otherwise we would
> > > have exactly done that. If you know how to do that please tell.
> > 
> > Would they? AMD is not exactly doing good job with communication
> 
> Yes they would. Stop making up weird conspiracy theories.

> > here. If BIOS can do it, kernel can do it, too...
> 
> May I recommend to read up on SMM and BIOS being able to lock down access
> to certain facilities?
> 
> > or do you have information saying otherwise?
> 
> Yes. It was clearly stated by Tom that it can only be done in the
> BIOS.

Do you have a link for that? Because I don't think I seen that one.

> > > Also disabling it for every BIOS is the only way which can be done because
> > > there is no way to know whether the BIOS is fixed or not at cold boot
> > > time. And it has to be known there because applications cache the
> > 
> > I'm pretty sure DMI-based whitelist would help here. It should be
> > reasonably to fill it with the common machines at least.
> 
> Send patches to that effect.

Why should it be my job? AMD screwed this up, they should fix it
properly. And you should insist on proper fix.

> > Plus, where is the CVE, and does AMD do anything to make BIOS vendors
> > fix them?
> 
> May I redirect you to: https://www.amd.com/en/corporate/contact

That will certainly make communication easier, right.

								Pavel
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply

* Re: [PATCH v2] vsprintf: introduce %dE for error constants
From: Petr Mladek @ 2019-08-28 11:32 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Sergey Senozhatsky, Steven Rostedt, Enrico, Weigelt,
	Andrew Morton, Jani Nikula, metux IT consult, Jonathan Corbet,
	linux-doc, linux-kernel
In-Reply-To: <20190827211244.7210-1-uwe@kleine-koenig.org>

On Tue 2019-08-27 23:12:44, Uwe Kleine-König  wrote:
> Petr Mladek had some concerns:
> > The array is long, created by cpu&paste, the index of each code
> > is not obvious.
> 
> Yeah right, the array is long. This cannot really be changed because we
> have that many error codes. I don't understand your concern about the
> index not being obvious. The array was just a list of (number, string)
> pairs where the position in the array didn't have any semantic.

I missed that the number was stored in the array as well. I somehow
expected that it was array of strings.


> > There are ideas to make the code even more tricky to reduce
> > the size, keep it fast.
> 
> I think Enrico Weigelt's suggestion to use a case is the best
> performance-wise so that's what I picked up. Also I hope that
> performance isn't that important because the need to print an error
> should not be so common that it really hurts in production.

I personally do not like switch/case. It is a lot of code.
I wonder if it even saved some space.

If you want to safe space, I would use u16 to store the numbers.
Or I would use array of strings. There will be only few holes.

You might also consider handling only the most commonly
used codes from errno.h and errno-base.h (1..133). There will
be no holes and the codes are stable.


> > Both, %dE modifier and the output format (ECODE) is non-standard.
> 
> Yeah, obviously right. The problem is that the new modifier does
> something that wasn't implemented before, so it cannot match any
> standard. %pI is only known on Linux either, so I think being
> non-standard is a weak argument.

I am not completely sure that %p modifiers were a good idea.
They came before I started maintaining printk(). They add more
complex algorithms into paths where we could not report problems
easily (printk recursion). Also they are causing problems with
unit testing that might be done in userspace. These non-standard
formats cause that printk() can't be simply substituted by printf().

I am not keen to spread these problems over more formats.
Also %d format is more complicated. It is often used with
already existing modifiers.


> > Upper letters gain a lot of attention. But the error code is
> > only helper information. Also many error codes are misleading because
> > they are used either wrongly or there was no better available.
> 
> This isn't really an argument against the patch I think. Sure, if a
> function returned (say) EIO while ETIMEOUT would be better, my patch
> doesn't improve that detail. Still
>
>         mydev: Failed to initialize blablub: EIO
>
> is more expressive than
> 
>         mydev: Failed to initialize blablub: -5

OK, upper letters probably are not a problem.

But what about EWOULDBLOCK and EDEADLOCK? They have the same
error codes as EAGAIN and EDEADLK. It might cause a lot of confusion.
People might spend a lot of time searching for EAGAIN before they
notice that EWOULDBLOCK was used in the code instead.

Also you still did not answer the question where the idea came from.
Did it just look nice? Anyone asked for it? Who? Why?


> > There is no proof that this approach would be widely acceptable for
> > subsystem maintainers. Some might not like mass and "blind" code
> > changes. Some might not like the output at all.
> 
> I don't intend to mass convert existing code. I would restrict myself to
> updating the documentation and then maybe send a patch per subsystem as an
> example to let maintainers know and judge for themselves if they like it or
> not. And if it doesn't get picked up, we can just remove the feature again next
> year (or so).

It looks like a lot of potentially useless work.


> I dropped the example conversion, I think the idea should be clear now
> even without an explicit example.

Please, do the opposite. Add conversion of few subsystems into the
patchset and add more people into CC. We will see immediately whether
it makes sense to spend time on this.

I personally think that this feature is not worth the code, data,
and bikeshedding.

Best Regards,
Petr

^ permalink raw reply

* Re: [PATCH v3 01/10] KVM: arm64: Document PV-time interface
From: Steven Price @ 2019-08-28 11:23 UTC (permalink / raw)
  To: Christoffer Dall
  Cc: Marc Zyngier, Will Deacon, linux-arm-kernel, kvmarm, linux-kernel,
	kvm, Catalin Marinas, linux-doc, Russell King, Paolo Bonzini
In-Reply-To: <20190827084407.GA6541@e113682-lin.lund.arm.com>

On 27/08/2019 09:44, Christoffer Dall wrote:
> On Wed, Aug 21, 2019 at 04:36:47PM +0100, Steven Price wrote:
>> Introduce a paravirtualization interface for KVM/arm64 based on the
>> "Arm Paravirtualized Time for Arm-Base Systems" specification DEN 0057A.
>>
>> This only adds the details about "Stolen Time" as the details of "Live
>> Physical Time" have not been fully agreed.
>>
>> User space can specify a reserved area of memory for the guest and
>> inform KVM to populate the memory with information on time that the host
>> kernel has stolen from the guest.
>>
>> A hypercall interface is provided for the guest to interrogate the
>> hypervisor's support for this interface and the location of the shared
>> memory structures.
>>
>> Signed-off-by: Steven Price <steven.price@arm.com>
>> ---
>>  Documentation/virt/kvm/arm/pvtime.txt | 100 ++++++++++++++++++++++++++
>>  1 file changed, 100 insertions(+)
>>  create mode 100644 Documentation/virt/kvm/arm/pvtime.txt
>>
>> diff --git a/Documentation/virt/kvm/arm/pvtime.txt b/Documentation/virt/kvm/arm/pvtime.txt
>> new file mode 100644
>> index 000000000000..1ceb118694e7
>> --- /dev/null
>> +++ b/Documentation/virt/kvm/arm/pvtime.txt
>> @@ -0,0 +1,100 @@
>> +Paravirtualized time support for arm64
>> +======================================
>> +
>> +Arm specification DEN0057/A defined a standard for paravirtualised time
>> +support for AArch64 guests:
>> +
>> +https://developer.arm.com/docs/den0057/a
>> +
>> +KVM/arm64 implements the stolen time part of this specification by providing
>> +some hypervisor service calls to support a paravirtualized guest obtaining a
>> +view of the amount of time stolen from its execution.
>> +
>> +Two new SMCCC compatible hypercalls are defined:
>> +
>> +PV_FEATURES 0xC5000020
>> +PV_TIME_ST  0xC5000022
>> +
>> +These are only available in the SMC64/HVC64 calling convention as
>> +paravirtualized time is not available to 32 bit Arm guests. The existence of
>> +the PV_FEATURES hypercall should be probed using the SMCCC 1.1 ARCH_FEATURES
>> +mechanism before calling it.
>> +
>> +PV_FEATURES
>> +    Function ID:  (uint32)  : 0xC5000020
>> +    PV_func_id:   (uint32)  : Either PV_TIME_LPT or PV_TIME_ST
>> +    Return value: (int32)   : NOT_SUPPORTED (-1) or SUCCESS (0) if the relevant
>> +                              PV-time feature is supported by the hypervisor.
>> +
>> +PV_TIME_ST
>> +    Function ID:  (uint32)  : 0xC5000022
>> +    Return value: (int64)   : IPA of the stolen time data structure for this
>> +                              (V)CPU. On failure:
>> +                              NOT_SUPPORTED (-1)
>> +
>> +The IPA returned by PV_TIME_ST should be mapped by the guest as normal memory
>> +with inner and outer write back caching attributes, in the inner shareable
>> +domain. A total of 16 bytes from the IPA returned are guaranteed to be
>> +meaningfully filled by the hypervisor (see structure below).
>> +
>> +PV_TIME_ST returns the structure for the calling VCPU.
>> +
>> +Stolen Time
>> +-----------
>> +
>> +The structure pointed to by the PV_TIME_ST hypercall is as follows:
>> +
>> +  Field       | Byte Length | Byte Offset | Description
>> +  ----------- | ----------- | ----------- | --------------------------
>> +  Revision    |      4      |      0      | Must be 0 for version 0.1
>> +  Attributes  |      4      |      4      | Must be 0
>> +  Stolen time |      8      |      8      | Stolen time in unsigned
>> +              |             |             | nanoseconds indicating how
>> +              |             |             | much time this VCPU thread
>> +              |             |             | was involuntarily not
>> +              |             |             | running on a physical CPU.
>> +
>> +The structure will be updated by the hypervisor prior to scheduling a VCPU. It
>> +will be present within a reserved region of the normal memory given to the
>> +guest. The guest should not attempt to write into this memory. There is a
>> +structure per VCPU of the guest.
>> +
>> +User space interface
>> +====================
>> +
>> +User space can request that KVM provide the paravirtualized time interface to
>> +a guest by creating a KVM_DEV_TYPE_ARM_PV_TIME device, for example:
>> +
> 
> I feel it would be more consistent to have the details of this in
> Documentation/virt/kvm/devices/arm-pv-time.txt and refer to this
> document from here.

Fair point - I'll move this lower part of the document and add a reference.

Thanks,

Steve

>> +    struct kvm_create_device pvtime_device = {
>> +            .type = KVM_DEV_TYPE_ARM_PV_TIME,
>> +            .attr = 0,
>> +            .flags = 0,
>> +    };
>> +
>> +    pvtime_fd = ioctl(vm_fd, KVM_CREATE_DEVICE, &pvtime_device);
>> +
>> +Creation of the device should be done after creating the vCPUs of the virtual
>> +machine.
>> +
>> +The IPA of the structures must be given to KVM. This is the base address
>> +of an array of stolen time structures (one for each VCPU). The base address
>> +must be page aligned. The size must be at least 64 * number of VCPUs and be a
>> +multiple of PAGE_SIZE.
>> +
>> +The memory for these structures should be added to the guest in the usual
>> +manner (e.g. using KVM_SET_USER_MEMORY_REGION).
>> +
>> +For example:
>> +
>> +    struct kvm_dev_arm_st_region region = {
>> +            .gpa = <IPA of guest base address>,
>> +            .size = <size in bytes>
>> +    };
>> +
>> +    struct kvm_device_attr st_base = {
>> +            .group = KVM_DEV_ARM_PV_TIME_PADDR,
>> +            .attr = KVM_DEV_ARM_PV_TIME_ST,
>> +            .addr = (u64)&region
>> +    };
>> +
>> +    ioctl(pvtime_fd, KVM_SET_DEVICE_ATTR, &st_base);
>> -- 
>> 2.20.1
>>
> 
> Thanks,
> 
>     Christoffer
> 


^ permalink raw reply

* Re: [PATCH 4.19 72/98] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
From: Thomas Gleixner @ 2019-08-28 10:47 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Greg Kroah-Hartman, linux-kernel, stable, Tom Lendacky,
	Borislav Petkov, Andrew Cooper, Andrew Morton, Chen Yu,
	H. Peter Anvin, Ingo Molnar, Jonathan Corbet, Josh Poimboeuf,
	Juergen Gross, Kees Cook, linux-doc@vger.kernel.org,
	linux-pm@vger.kernel.org, Nathan Chancellor, Paolo Bonzini,
	Rafael J. Wysocki, x86@kernel.org
In-Reply-To: <20190828103113.GA14677@amd>

Pavel,

On Wed, 28 Aug 2019, Pavel Machek wrote:
> On Tue 2019-08-27 15:30:30, Thomas Gleixner wrote:
> > There is no way to reinitialize RDRAND from the kernel otherwise we would
> > have exactly done that. If you know how to do that please tell.
> 
> Would they? AMD is not exactly doing good job with communication

Yes they would. Stop making up weird conspiracy theories.

> here. If BIOS can do it, kernel can do it, too...

May I recommend to read up on SMM and BIOS being able to lock down access
to certain facilities?

> or do you have information saying otherwise?

Yes. It was clearly stated by Tom that it can only be done in the BIOS.

> > Also disabling it for every BIOS is the only way which can be done because
> > there is no way to know whether the BIOS is fixed or not at cold boot
> > time. And it has to be known there because applications cache the
> 
> I'm pretty sure DMI-based whitelist would help here. It should be
> reasonably to fill it with the common machines at least.

Send patches to that effect.
 
> Plus, where is the CVE, and does AMD do anything to make BIOS vendors
> fix them?

May I redirect you to: https://www.amd.com/en/corporate/contact

Thanks,

	tglx

^ permalink raw reply

* Re: [PATCH 4.19 72/98] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
From: Pavel Machek @ 2019-08-28 10:31 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Pavel Machek, Greg Kroah-Hartman, linux-kernel, stable,
	Tom Lendacky, Borislav Petkov, Andrew Cooper, Andrew Morton,
	Chen Yu, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
	Josh Poimboeuf, Juergen Gross, Kees Cook,
	linux-doc@vger.kernel.org, linux-pm@vger.kernel.org,
	Nathan Chancellor, Paolo Bonzini, Rafael J. Wysocki,
	x86@kernel.org
In-Reply-To: <alpine.DEB.2.21.1908271525480.1939@nanos.tec.linutronix.de>

[-- Attachment #1: Type: text/plain, Size: 1636 bytes --]

On Tue 2019-08-27 15:30:30, Thomas Gleixner wrote:
> On Tue, 27 Aug 2019, Pavel Machek wrote:
> 
> > On Tue 2019-08-27 09:50:51, Greg Kroah-Hartman wrote:
> > > From: Tom Lendacky <thomas.lendacky@amd.com>
> > > 
> > > commit c49a0a80137c7ca7d6ced4c812c9e07a949f6f24 upstream.
> > > 
> > > There have been reports of RDRAND issues after resuming from suspend on
> > > some AMD family 15h and family 16h systems. This issue stems from a BIOS
> > > not performing the proper steps during resume to ensure RDRAND continues
> > > to function properly.
> > 
> > Yes. And instead of reinitializing the RDRAND on resume, this patch
> > breaks support even for people with properly functioning BIOSes...
> 
> There is no way to reinitialize RDRAND from the kernel otherwise we would
> have exactly done that. If you know how to do that please tell.

Would they? AMD is not exactly doing good job with communication
here. If BIOS can do it, kernel can do it, too... or do you have
information saying otherwise?

> Also disabling it for every BIOS is the only way which can be done because
> there is no way to know whether the BIOS is fixed or not at cold boot
> time. And it has to be known there because applications cache the

I'm pretty sure DMI-based whitelist would help here. It should be
reasonably to fill it with the common machines at least.

Plus, where is the CVE, and does AMD do anything to make BIOS vendors
fix them?

Best regards,
								Pavel
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply

* RE: [PATCH V15 1/5] dt-bindings: fsl: scu: add thermal binding
From: Anson Huang @ 2019-08-28  9:20 UTC (permalink / raw)
  To: Zhang Rui, robh+dt@kernel.org, mark.rutland@arm.com,
	corbet@lwn.net, shawnguo@kernel.org, s.hauer@pengutronix.de,
	kernel@pengutronix.de, festevam@gmail.com,
	catalin.marinas@arm.com, will.deacon@arm.com, edubezval@gmail.com,
	daniel.lezcano@linaro.org, Aisheng Dong, ulf.hansson@linaro.org,
	Peng Fan, mchehab+samsung@kernel.org, linux@roeck-us.net,
	Daniel Baluta, maxime.ripard@bootlin.com,
	horms+renesas@verge.net.au, olof@lixom.net,
	jagan@amarulasolutions.com, bjorn.andersson@linaro.org,
	Leonard Crestez, dinguyen@kernel.org,
	enric.balletbo@collabora.com, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, linux-pm@vger.kernel.org
  Cc: dl-linux-imx
In-Reply-To: <ff3fd0ab2ba659e98070760be8e7c0f223e921f9.camel@intel.com>

Hi, Rui

> Subject: Re: [PATCH V15 1/5] dt-bindings: fsl: scu: add thermal binding
> 
> Hi, Anson,
> 
> We're missing ACK from the maintainers for patch 4/5 and 5/5, if we want to
> shipped the patch via thermal tree.

I think 4/5 and 5/5 can be taken by Shawn Guo once the driver part is taken, he
normally do it in this way, once driver and dt-binding are taken, I will notify him.

> 
> For patch 2/5, as it introduces a new API for OF_THERMAL, I'd like to get
> Eduardo' feedback before taking them.

OK, NOT sure when he can take a look at it, we are kind of pending on this for some
time, let's wait. If you have chance to talk to him, please help ask for help from him about
this patch series, thanks a lot!

thanks,
Anson



^ permalink raw reply

* Re: [PATCH V15 1/5] dt-bindings: fsl: scu: add thermal binding
From: Zhang Rui @ 2019-08-28  9:14 UTC (permalink / raw)
  To: Anson Huang, robh+dt@kernel.org, mark.rutland@arm.com,
	corbet@lwn.net, shawnguo@kernel.org, s.hauer@pengutronix.de,
	kernel@pengutronix.de, festevam@gmail.com,
	catalin.marinas@arm.com, will.deacon@arm.com, edubezval@gmail.com,
	daniel.lezcano@linaro.org, Aisheng Dong, ulf.hansson@linaro.org,
	Peng Fan, mchehab+samsung@kernel.org, linux@roeck-us.net,
	Daniel Baluta, maxime.ripard@bootlin.com,
	horms+renesas@verge.net.au, olof@lixom.net,
	jagan@amarulasolutions.com, bjorn.andersson@linaro.org,
	Leonard Crestez, dinguyen@kernel.org,
	enric.balletbo@collabora.com, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, linux-pm@vger.kernel.org
  Cc: dl-linux-imx
In-Reply-To: <DB3PR0402MB39162EB555CD7AE75D58C582F5C60@DB3PR0402MB3916.eurprd04.prod.outlook.com>

Hi, Anson,

We're missing ACK from the maintainers for patch 4/5 and 5/5, if we
want to shipped the patch via thermal tree.

For patch 2/5, as it introduces a new API for OF_THERMAL, I'd like to
get Eduardo' feedback before taking them.

thanks,
rui

On Wed, 2019-07-24 at 03:16 +0000, Anson Huang wrote:
> Ping...
> 
> > Hi, Daniel/Rui/Eduardo
> > 	Could you please take a look at this patch series?
> > 
> > Anson
> > 
> > > From: Anson Huang <Anson.Huang@nxp.com>
> > > 
> > > NXP i.MX8QXP is an ARMv8 SoC with a Cortex-M4 core inside as
> > > system
> > > controller, the system controller is in charge of system power,
> > > clock
> > > and thermal sensors etc. management, Linux kernel has to
> > > communicate
> > > with system controller via MU (message unit) IPC to get
> > > temperature
> > > from thermal sensors, this patch adds binding doc for i.MX system
> > > controller thermal driver.
> > > 
> > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > > Reviewed-by: Rob Herring <robh@kernel.org>
> > > Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
> > > ---
> > > No change.
> > > ---
> > >  .../devicetree/bindings/arm/freescale/fsl,scu.txt        | 16
> > 
> > ++++++++++++++++
> > >  1 file changed, 16 insertions(+)
> > > 
> > > diff --git
> > > a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > index a575e42..fc3844e 100644
> > > --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > @@ -155,6 +155,17 @@ Required properties:
> > >  Optional properties:
> > >  - timeout-sec: contains the watchdog timeout in seconds.
> > > 
> > > +Thermal bindings based on SCU Message Protocol
> > > +------------------------------------------------------------
> > > +
> > > +Required properties:
> > > +- compatible:			Should be :
> > > +				  "fsl,imx8qxp-sc-thermal"
> > > +				followed by "fsl,imx-sc-thermal";
> > > +
> > > +- #thermal-sensor-cells:	See
> > > Documentation/devicetree/bindings/thermal/thermal.txt
> > > +				for a description.
> > > +
> > >  Example (imx8qxp):
> > >  -------------
> > >  aliases {
> > > @@ -222,6 +233,11 @@ firmware {
> > >  			compatible = "fsl,imx8qxp-sc-wdt", "fsl,imx-sc-
> > > wdt";
> > >  			timeout-sec = <60>;
> > >  		};
> > > +
> > > +		tsens: thermal-sensor {
> > > +			compatible = "fsl,imx8qxp-sc-thermal",
> > > "fsl,imx-sc-
> > > thermal";
> > > +			#thermal-sensor-cells = <1>;
> > > +		};
> > >  	};
> > >  };
> > > 
> > > --
> > > 2.7.4
> 
> 


^ permalink raw reply

* Re: [PATCH v8 11/27] x86/mm: Introduce _PAGE_DIRTY_SW
From: Peter Zijlstra @ 2019-08-28  7:03 UTC (permalink / raw)
  To: Yu-cheng Yu
  Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
	linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
	Andy Lutomirski, Balbir Singh, Borislav Petkov, Cyrill Gorcunov,
	Dave Hansen, Eugene Syromiatnikov, Florian Weimer, H.J. Lu,
	Jann Horn, Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit,
	Oleg Nesterov, Pavel Machek, Randy Dunlap, Ravi V. Shankar,
	Vedvyas Shanbhogue, Dave Martin
In-Reply-To: <6c3dc33e16c8bbb6d45c0a6ec7c684de197fa065.camel@intel.com>

On Tue, Aug 27, 2019 at 03:37:12PM -0700, Yu-cheng Yu wrote:
> On Fri, 2019-08-23 at 16:02 +0200, Peter Zijlstra wrote:
> > On Tue, Aug 13, 2019 at 01:52:09PM -0700, Yu-cheng Yu wrote:
> > 
> > > +static inline pte_t pte_move_flags(pte_t pte, pteval_t from, pteval_t to)
> > > +{
> > > +	if (pte_flags(pte) & from)
> > > +		pte = pte_set_flags(pte_clear_flags(pte, from), to);
> > > +	return pte;
> > > +}
> > 
> > Aside of the whole conditional thing (I agree it would be better to have
> > this unconditionally); the function doesn't really do as advertised.
> > 
> > That is, if @from is clear, it doesn't endeavour to make sure @to is
> > also clear.
> > 
> > Now it might be sufficient, but in that case it really needs a comment
> > and or different name.
> > 
> > An implementation that actually moves the bit is something like:
> > 
> > 	pteval_t a,b;
> > 
> > 	a = native_pte_value(pte);
> > 	b = (a >> from_bit) & 1;
> > 	a &= ~((1ULL << from_bit) | (1ULL << to_bit));
> > 	a |= b << to_bit;
> > 	return make_native_pte(a);
> 
> There can be places calling pte_wrprotect() on a PTE that is already RO +
> DIRTY_SW.  Then in pte_move_flags(pte, _PAGE_DIRTY_HW, _PAGE_DIRTY_SW) we do not
>  want to clear _PAGE_DIRTY_SW.  But, I will look into this and make it more
> obvious.

Well, then the name 'move' is just wrong, because that is not the
semantics you're looking for.

So the thing is; if you provide a generic function that 'munges' two
bits, then it's name had better be accurate. But AFAICT you only ever
used this for the DIRTY bits, so it might be better to have a function
specifically for that and with a comment that spells out the exact
semantics and reasons for them.

^ permalink raw reply

* Re: FAILED: patch "[PATCH] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h" failed to apply to 4.9-stable tree
From: Sasha Levin @ 2019-08-28  3:58 UTC (permalink / raw)
  To: gregkh
  Cc: thomas.lendacky, akpm, andrew.cooper3, bp, corbet, hpa, jgross,
	jpoimboe, keescook, linux-doc, linux-pm, mingo, natechancellor,
	pavel, pbonzini, rjw, stable, tglx, x86, yu.c.chen
In-Reply-To: <1566809975147242@kroah.com>

On Mon, Aug 26, 2019 at 10:59:35AM +0200, gregkh@linuxfoundation.org wrote:
>The patch below does not apply to the 4.9-stable tree.
>If someone wants it applied there, or to any other stable or longterm
>tree, then please email the backport, including the original git commit
>id to <stable@vger.kernel.org>.

I've fixed it up and queued for 4.9 and 4.4.

--
Thanks,
Sasha

^ permalink raw reply

* Re: [RFC v1 0/2] RCU dyntick nesting counter cleanups
From: Paul E. McKenney @ 2019-08-28  0:11 UTC (permalink / raw)
  To: Joel Fernandes (Google)
  Cc: linux-kernel, Frederic Weisbecker, Jonathan Corbet, Josh Triplett,
	kernel-team, Lai Jiangshan, linux-doc, Mathieu Desnoyers,
	Mauro Carvalho Chehab, rcu, Steven Rostedt
In-Reply-To: <5d648893.1c69fb81.5e60a.fc6c@mx.google.com>

On Mon, Aug 26, 2019 at 09:33:52PM -0400, Joel Fernandes (Google) wrote:
> These patches clean up the usage of dynticks nesting counters simplifying the
> code, while preserving the usecases.
> 
> It is a much needed simplification, makes the code less confusing, and prevents
> future bugs such as those that arise from forgetting that the
> dynticks_nmi_nesting counter is not a simple counter and can be "crowbarred" in
> common situations.
> 
> Several nights of rcutorture testing with CONFIG_RCU_EQS_DEBUG on all RCU
> kernel configurations have survived without any splats.
> 
> Further testing is in progress, hence marked as RFC!

My intent was to review this today, but this ran afoul of recent 3.10
work that made for some "interesting" review comments.  Fortunately,
I realized my mistake before sending the email.  I then reviewed the
current 2019 RCU dyntick-idle code.

I am doing some (possibly redundant) rcutorture runs on them here, and
will take a fresh look tomorrow.

							Thanx, Paul

> thanks,
> 
>  - Joel
> 
> Joel Fernandes (Google) (2):
> rcu/tree: Clean up dynticks counter usage
> rcu/tree: Remove dynticks_nmi_nesting counter
> 
> .../Data-Structures/Data-Structures.rst       | 31 ++----
> Documentation/RCU/stallwarn.txt               |  6 +-
> kernel/rcu/rcu.h                              |  4 -
> kernel/rcu/tree.c                             | 98 +++++++++----------
> kernel/rcu/tree.h                             |  4 +-
> kernel/rcu/tree_stall.h                       |  4 +-
> 6 files changed, 64 insertions(+), 83 deletions(-)
> 
> --
> 2.23.0.187.g17f5b7556c-goog
> 

^ permalink raw reply

* Re: [PATCH 2/5] rcu/tree: Add multiple in-flight batches of kfree_rcu work
From: Boqun Feng @ 2019-08-27 23:52 UTC (permalink / raw)
  To: Joel Fernandes (Google)
  Cc: linux-kernel, Paul E . McKenney, byungchul.park, Josh Triplett,
	Lai Jiangshan, linux-doc, Mathieu Desnoyers, rcu, Steven Rostedt,
	kernel-team
In-Reply-To: <5d657e35.1c69fb81.54250.01de@mx.google.com>

[-- Attachment #1: Type: text/plain, Size: 5608 bytes --]

Hi Joel,

On Tue, Aug 27, 2019 at 03:01:56PM -0400, Joel Fernandes (Google) wrote:
> During testing, it was observed that amount of memory consumed due
> kfree_rcu() batching is 300-400MB. Previously we had only a single
> head_free pointer pointing to the list of rcu_head(s) that are to be
> freed after a grace period. Until this list is drained, we cannot queue
> any more objects on it since such objects may not be ready to be
> reclaimed when the worker thread eventually gets to drainin g the
> head_free list.
> 
> We can do better by maintaining multiple lists as done by this patch.
> Testing shows that memory consumption came down by around 100-150MB with
> just adding another list. Adding more than 1 additional list did not
> show any improvement.
> 
> Suggested-by: Paul E. McKenney <paulmck@linux.ibm.com>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> ---
>  kernel/rcu/tree.c | 64 +++++++++++++++++++++++++++++++++--------------
>  1 file changed, 45 insertions(+), 19 deletions(-)
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 4f7c3096d786..9b9ae4db1c2d 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -2688,28 +2688,38 @@ EXPORT_SYMBOL_GPL(call_rcu);
>  
>  /* Maximum number of jiffies to wait before draining a batch. */
>  #define KFREE_DRAIN_JIFFIES (HZ / 50)
> +#define KFREE_N_BATCHES 2
> +
> +struct kfree_rcu_work {
> +	/* The rcu_work node for queuing work with queue_rcu_work(). The work
> +	 * is done after a grace period.
> +	 */
> +	struct rcu_work rcu_work;
> +
> +	/* The list of objects that have now left ->head and are queued for
> +	 * freeing after a grace period.
> +	 */
> +	struct rcu_head *head_free;
> +
> +	struct kfree_rcu_cpu *krcp;
> +};
> +static DEFINE_PER_CPU(__typeof__(struct kfree_rcu_work)[KFREE_N_BATCHES], krw);
>  

Why not

	static DEFINE_PER_CPU(struct kfree_rcu_work[KFREE_N_BATCHES], krw);

here? Am I missing something?

Further, given "struct kfree_rcu_cpu" is only for defining percpu
variables, how about orginazing the data structure like:

	struct kfree_rcu_cpu {
		...
		struct kfree_rcu_work krws[KFREE_N_BATCHES];
		...
	}

This could save one pointer in kfree_rcu_cpu, and I think it provides
better cache locality for accessing _cpu and _work on the same cpu.

Thoughts?

Regards,
Boqun


>  /*
>   * Maximum number of kfree(s) to batch, if this limit is hit then the batch of
>   * kfree(s) is queued for freeing after a grace period, right away.
>   */
>  struct kfree_rcu_cpu {
> -	/* The rcu_work node for queuing work with queue_rcu_work(). The work
> -	 * is done after a grace period.
> -	 */
> -	struct rcu_work rcu_work;
>  
>  	/* The list of objects being queued in a batch but are not yet
>  	 * scheduled to be freed.
>  	 */
>  	struct rcu_head *head;
>  
> -	/* The list of objects that have now left ->head and are queued for
> -	 * freeing after a grace period.
> -	 */
> -	struct rcu_head *head_free;
> +	/* Pointer to the per-cpu array of kfree_rcu_work structures */
> +	struct kfree_rcu_work *krwp;
>  
> -	/* Protect concurrent access to this structure. */
> +	/* Protect concurrent access to this structure and kfree_rcu_work. */
>  	spinlock_t lock;
>  
>  	/* The delayed work that flushes ->head to ->head_free incase ->head
> @@ -2730,12 +2740,14 @@ static void kfree_rcu_work(struct work_struct *work)
>  {
>  	unsigned long flags;
>  	struct rcu_head *head, *next;
> -	struct kfree_rcu_cpu *krcp = container_of(to_rcu_work(work),
> -					struct kfree_rcu_cpu, rcu_work);
> +	struct kfree_rcu_work *krwp = container_of(to_rcu_work(work),
> +					struct kfree_rcu_work, rcu_work);
> +	struct kfree_rcu_cpu *krcp;
> +
> +	krcp = krwp->krcp;
>  
>  	spin_lock_irqsave(&krcp->lock, flags);
> -	head = krcp->head_free;
> -	krcp->head_free = NULL;
> +	head = xchg(&krwp->head_free, NULL);
>  	spin_unlock_irqrestore(&krcp->lock, flags);
>  
>  	/*
> @@ -2758,19 +2770,28 @@ static void kfree_rcu_work(struct work_struct *work)
>   */
>  static inline bool queue_kfree_rcu_work(struct kfree_rcu_cpu *krcp)
>  {
> +	int i = 0;
> +	struct kfree_rcu_work *krwp = NULL;
> +
>  	lockdep_assert_held(&krcp->lock);
> +	while (i < KFREE_N_BATCHES) {
> +		if (!krcp->krwp[i].head_free) {
> +			krwp = &(krcp->krwp[i]);
> +			break;
> +		}
> +		i++;
> +	}
>  
> -	/* If a previous RCU batch work is already in progress, we cannot queue
> +	/* If both RCU batches are already in progress, we cannot queue
>  	 * another one, just refuse the optimization and it will be retried
>  	 * again in KFREE_DRAIN_JIFFIES time.
>  	 */
> -	if (krcp->head_free)
> +	if (!krwp)
>  		return false;
>  
> -	krcp->head_free = krcp->head;
> -	krcp->head = NULL;
> -	INIT_RCU_WORK(&krcp->rcu_work, kfree_rcu_work);
> -	queue_rcu_work(system_wq, &krcp->rcu_work);
> +	krwp->head_free = xchg(&krcp->head, NULL);
> +	INIT_RCU_WORK(&krwp->rcu_work, kfree_rcu_work);
> +	queue_rcu_work(system_wq, &krwp->rcu_work);
>  
>  	return true;
>  }
> @@ -3736,8 +3757,13 @@ static void __init kfree_rcu_batch_init(void)
>  
>  	for_each_possible_cpu(cpu) {
>  		struct kfree_rcu_cpu *krcp = per_cpu_ptr(&krc, cpu);
> +		struct kfree_rcu_work *krwp = &(per_cpu(krw, cpu)[0]);
> +		int i = KFREE_N_BATCHES;
>  
>  		spin_lock_init(&krcp->lock);
> +		krcp->krwp = krwp;
> +		while (i--)
> +			krwp[i].krcp = krcp;
>  		INIT_DELAYED_WORK(&krcp->monitor_work, kfree_rcu_monitor);
>  	}
>  }
> -- 
> 2.23.0.187.g17f5b7556c-goog
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply

* Re: [PATCH v2] vsprintf: introduce %dE for error constants
From: Joe Perches @ 2019-08-27 23:10 UTC (permalink / raw)
  To: Uwe Kleine-König, Sergey Senozhatsky, Jani Nikula,
	Petr Mladek, Steven Rostedt, Andrew Morton,
	Enrico Weigelt, metux IT consult
  Cc: Jonathan Corbet, linux-doc, linux-kernel
In-Reply-To: <59bfcf90-f7f8-4d55-b5bf-211f6ca67917@kleine-koenig.org>

On Tue, 2019-08-27 at 23:35 +0200, Uwe Kleine-König wrote:
> Hello Joe,
> 
> On 8/27/19 11:22 PM, Joe Perches wrote:
> > On Tue, 2019-08-27 at 23:12 +0200, Uwe Kleine-König wrote:
> > > The new format specifier %dE introduced with this patch pretty-prints
> > > the typical negative error values. So
> > > 
> > > 	pr_info("probing failed (%dE)\n", ret);
> > > 
> > > yields
> > > 
> > > 	probing failed (EIO)
> > > 
> > > if ret holds -EIO. This is easier to understand than the for now common
> > > 
> > > 	probing failed (-5)
> > 
> > I suggest using both outputs like '-5 -EIO'
> > rather than a single string
> 
> I like it the way it is implemented as it is more flexible. If you want
> to see both, you can still do
> 
> 	pr_info("probing failed (%d %dE)\n", ret, ret);
> 
> and people (like me) who think that giving only EIO can still do just that.

<shrug>  Up to you.  Just a suggestion.

btw:

The test for %<dixu>E (FORMAT_TYPE_INT)
should probably include a test for

(spec->flags & SIGN)

so that it only is used for %d and %i and
disregarded for %x and %u



^ permalink raw reply

* Re: [PATCH v8 11/27] x86/mm: Introduce _PAGE_DIRTY_SW
From: Yu-cheng Yu @ 2019-08-27 22:37 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
	linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
	Andy Lutomirski, Balbir Singh, Borislav Petkov, Cyrill Gorcunov,
	Dave Hansen, Eugene Syromiatnikov, Florian Weimer, H.J. Lu,
	Jann Horn, Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit,
	Oleg Nesterov, Pavel Machek, Randy Dunlap, Ravi V. Shankar,
	Vedvyas Shanbhogue, Dave Martin
In-Reply-To: <20190823140233.GC2332@hirez.programming.kicks-ass.net>

On Fri, 2019-08-23 at 16:02 +0200, Peter Zijlstra wrote:
> On Tue, Aug 13, 2019 at 01:52:09PM -0700, Yu-cheng Yu wrote:
> 
> > +static inline pte_t pte_move_flags(pte_t pte, pteval_t from, pteval_t to)
> > +{
> > +	if (pte_flags(pte) & from)
> > +		pte = pte_set_flags(pte_clear_flags(pte, from), to);
> > +	return pte;
> > +}
> 
> Aside of the whole conditional thing (I agree it would be better to have
> this unconditionally); the function doesn't really do as advertised.
> 
> That is, if @from is clear, it doesn't endeavour to make sure @to is
> also clear.
> 
> Now it might be sufficient, but in that case it really needs a comment
> and or different name.
> 
> An implementation that actually moves the bit is something like:
> 
> 	pteval_t a,b;
> 
> 	a = native_pte_value(pte);
> 	b = (a >> from_bit) & 1;
> 	a &= ~((1ULL << from_bit) | (1ULL << to_bit));
> 	a |= b << to_bit;
> 	return make_native_pte(a);

There can be places calling pte_wrprotect() on a PTE that is already RO +
DIRTY_SW.  Then in pte_move_flags(pte, _PAGE_DIRTY_HW, _PAGE_DIRTY_SW) we do not
 want to clear _PAGE_DIRTY_SW.  But, I will look into this and make it more
obvious.

Thanks,
Yu-cheng  

^ permalink raw reply

* Re: [PATCH v5 3/9] fpga: dfl: afu: convert platform_driver to use dev_groups
From: Wu Hao @ 2019-08-27 21:38 UTC (permalink / raw)
  To: Moritz Fischer
  Cc: gregkh, linux-fpga, linux-kernel, linux-api, linux-doc, atull
In-Reply-To: <20190822150701.GB22556@archbox>

On Thu, Aug 22, 2019 at 08:07:01AM -0700, Moritz Fischer wrote:
> Hi Hao,
> 
> On Mon, Aug 12, 2019 at 10:49:58AM +0800, Wu Hao wrote:
> > This patch takes advantage of driver core which helps to create
> > and remove sysfs attribute files, so there is no need to register
> > sysfs entries manually in dfl-afu platform river code.
> Same nit: s/river/driver
> > 
> > Signed-off-by: Wu Hao <hao.wu@intel.com>
> Acked-by: Moritz Fischer <mdf@kernel.org>


Hi Moritz

Thanks a lot for the review. : )

Have you got a chance to look into the other patches in this patchset?

Thanks
Hao

> > ---
> >  drivers/fpga/dfl-afu-main.c | 69 +++++++++++++++++++++++----------------------
> >  1 file changed, 36 insertions(+), 33 deletions(-)
> > 
> > diff --git a/drivers/fpga/dfl-afu-main.c b/drivers/fpga/dfl-afu-main.c
> > index e50c45e..e955149 100644
> > --- a/drivers/fpga/dfl-afu-main.c
> > +++ b/drivers/fpga/dfl-afu-main.c
> > @@ -282,24 +282,17 @@ static int port_get_id(struct platform_device *pdev)
> >  	&dev_attr_power_state.attr,
> >  	NULL,
> >  };
> > -ATTRIBUTE_GROUPS(port_hdr);
> > +
> > +static const struct attribute_group port_hdr_group = {
> > +	.attrs = port_hdr_attrs,
> > +};
> >  
> >  static int port_hdr_init(struct platform_device *pdev,
> >  			 struct dfl_feature *feature)
> >  {
> > -	dev_dbg(&pdev->dev, "PORT HDR Init.\n");
> > -
> >  	port_reset(pdev);
> >  
> > -	return device_add_groups(&pdev->dev, port_hdr_groups);
> > -}
> > -
> > -static void port_hdr_uinit(struct platform_device *pdev,
> > -			   struct dfl_feature *feature)
> > -{
> > -	dev_dbg(&pdev->dev, "PORT HDR UInit.\n");
> > -
> > -	device_remove_groups(&pdev->dev, port_hdr_groups);
> > +	return 0;
> >  }
> >  
> >  static long
> > @@ -330,7 +323,6 @@ static void port_hdr_uinit(struct platform_device *pdev,
> >  
> >  static const struct dfl_feature_ops port_hdr_ops = {
> >  	.init = port_hdr_init,
> > -	.uinit = port_hdr_uinit,
> >  	.ioctl = port_hdr_ioctl,
> >  };
> >  
> > @@ -361,32 +353,37 @@ static void port_hdr_uinit(struct platform_device *pdev,
> >  	&dev_attr_afu_id.attr,
> >  	NULL
> >  };
> > -ATTRIBUTE_GROUPS(port_afu);
> >  
> > -static int port_afu_init(struct platform_device *pdev,
> > -			 struct dfl_feature *feature)
> > +static umode_t port_afu_attrs_visible(struct kobject *kobj,
> > +				      struct attribute *attr, int n)
> >  {
> > -	struct resource *res = &pdev->resource[feature->resource_index];
> > -	int ret;
> > -
> > -	dev_dbg(&pdev->dev, "PORT AFU Init.\n");
> > +	struct device *dev = kobj_to_dev(kobj);
> >  
> > -	ret = afu_mmio_region_add(dev_get_platdata(&pdev->dev),
> > -				  DFL_PORT_REGION_INDEX_AFU, resource_size(res),
> > -				  res->start, DFL_PORT_REGION_READ |
> > -				  DFL_PORT_REGION_WRITE | DFL_PORT_REGION_MMAP);
> > -	if (ret)
> > -		return ret;
> > +	/*
> > +	 * sysfs entries are visible only if related private feature is
> > +	 * enumerated.
> > +	 */
> > +	if (!dfl_get_feature_by_id(dev, PORT_FEATURE_ID_AFU))
> > +		return 0;
> >  
> > -	return device_add_groups(&pdev->dev, port_afu_groups);
> > +	return attr->mode;
> >  }
> >  
> > -static void port_afu_uinit(struct platform_device *pdev,
> > -			   struct dfl_feature *feature)
> > +static const struct attribute_group port_afu_group = {
> > +	.attrs      = port_afu_attrs,
> > +	.is_visible = port_afu_attrs_visible,
> > +};
> > +
> > +static int port_afu_init(struct platform_device *pdev,
> > +			 struct dfl_feature *feature)
> >  {
> > -	dev_dbg(&pdev->dev, "PORT AFU UInit.\n");
> Thanks.
> > +	struct resource *res = &pdev->resource[feature->resource_index];
> >  
> > -	device_remove_groups(&pdev->dev, port_afu_groups);
> > +	return afu_mmio_region_add(dev_get_platdata(&pdev->dev),
> > +				   DFL_PORT_REGION_INDEX_AFU,
> > +				   resource_size(res), res->start,
> > +				   DFL_PORT_REGION_MMAP | DFL_PORT_REGION_READ |
> > +				   DFL_PORT_REGION_WRITE);
> >  }
> >  
> >  static const struct dfl_feature_id port_afu_id_table[] = {
> > @@ -396,7 +393,6 @@ static void port_afu_uinit(struct platform_device *pdev,
> >  
> >  static const struct dfl_feature_ops port_afu_ops = {
> >  	.init = port_afu_init,
> > -	.uinit = port_afu_uinit,
> >  };
> >  
> >  static struct dfl_feature_driver port_feature_drvs[] = {
> > @@ -748,9 +744,16 @@ static int afu_remove(struct platform_device *pdev)
> >  	return 0;
> >  }
> >  
> > +static const struct attribute_group *afu_dev_groups[] = {
> > +	&port_hdr_group,
> > +	&port_afu_group,
> > +	NULL
> > +};
> > +
> >  static struct platform_driver afu_driver = {
> >  	.driver	= {
> > -		.name    = DFL_FPGA_FEATURE_DEV_PORT,
> > +		.name	    = DFL_FPGA_FEATURE_DEV_PORT,
> > +		.dev_groups = afu_dev_groups,
> >  	},
> >  	.probe   = afu_probe,
> >  	.remove  = afu_remove,
> > -- 
> > 1.8.3.1
> > 
> 
> Thanks,
> Moritz

^ permalink raw reply

* Re: [PATCH v2] vsprintf: introduce %dE for error constants
From: Uwe Kleine-König @ 2019-08-27 21:35 UTC (permalink / raw)
  To: Joe Perches, Sergey Senozhatsky, Jani Nikula, Petr Mladek,
	Steven Rostedt, Andrew Morton, Enrico Weigelt, metux IT consult
  Cc: Jonathan Corbet, linux-doc, linux-kernel
In-Reply-To: <04b021b263465c62628964ac402e15fd4cdc13a0.camel@perches.com>


[-- Attachment #1.1: Type: text/plain, Size: 779 bytes --]

Hello Joe,

On 8/27/19 11:22 PM, Joe Perches wrote:
> On Tue, 2019-08-27 at 23:12 +0200, Uwe Kleine-König wrote:
>> The new format specifier %dE introduced with this patch pretty-prints
>> the typical negative error values. So
>>
>> 	pr_info("probing failed (%dE)\n", ret);
>>
>> yields
>>
>> 	probing failed (EIO)
>>
>> if ret holds -EIO. This is easier to understand than the for now common
>>
>> 	probing failed (-5)
> 
> I suggest using both outputs like '-5 -EIO'
> rather than a single string

I like it the way it is implemented as it is more flexible. If you want
to see both, you can still do

	pr_info("probing failed (%d %dE)\n", ret, ret);

and people (like me) who think that giving only EIO can still do just that.

Best regards
Uwe


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply

* Re: [PATCH v2] vsprintf: introduce %dE for error constants
From: Joe Perches @ 2019-08-27 21:22 UTC (permalink / raw)
  To: Uwe Kleine-König, Sergey Senozhatsky, Jani Nikula,
	Petr Mladek, Steven Rostedt, Andrew Morton, Enrico, Weigelt,
	metux IT consult
  Cc: Jonathan Corbet, linux-doc, linux-kernel
In-Reply-To: <20190827211244.7210-1-uwe@kleine-koenig.org>

On Tue, 2019-08-27 at 23:12 +0200, Uwe Kleine-König wrote:
> The new format specifier %dE introduced with this patch pretty-prints
> the typical negative error values. So
> 
> 	pr_info("probing failed (%dE)\n", ret);
> 
> yields
> 
> 	probing failed (EIO)
> 
> if ret holds -EIO. This is easier to understand than the for now common
> 
> 	probing failed (-5)

I suggest using both outputs like '-5 -EIO'
rather than a single string



^ permalink raw reply

* Re: [PATCH v2] vsprintf: introduce %dE for error constants
From: Uwe Kleine-König @ 2019-08-27 21:17 UTC (permalink / raw)
  To: Sergey Senozhatsky, Jani Nikula, Petr Mladek, Steven Rostedt,
	Andrew Morton, Enrico Weigelt, metux IT consult
  Cc: Jonathan Corbet, linux-doc, linux-kernel
In-Reply-To: <20190827211244.7210-1-uwe@kleine-koenig.org>


[-- Attachment #1.1: Type: text/plain, Size: 203 bytes --]

Hello,

I fat-fingered Enrico Weigelt's e-mail address. Please when you reply to
my patch fix that up (i.e. drop Enrico@kleine-koenig.org and
Weigelt@kleine-koenig.org).

Thanks and sorry,
Uwe


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply

* [PATCH v2] vsprintf: introduce %dE for error constants
From: Uwe Kleine-König @ 2019-08-27 21:12 UTC (permalink / raw)
  To: Sergey Senozhatsky, Jani Nikula, Petr Mladek, Steven Rostedt,
	Andrew Morton, Enrico, Weigelt, metux IT consult
  Cc: Jonathan Corbet, linux-doc, linux-kernel

The new format specifier %dE introduced with this patch pretty-prints
the typical negative error values. So

	pr_info("probing failed (%dE)\n", ret);

yields

	probing failed (EIO)

if ret holds -EIO. This is easier to understand than the for now common

	probing failed (-5)

(which used %d instead of %dE) for kernel developers who don't know the
numbers by heart, The error names are also known and understood by
userspace coders as they match the values the errno variable can have.
Also to debug the sitation that resulted in the above message very
probably involves EIO, so the message already gives the string to look
out for.

glibc (and other libc variants)'s printf have a similar feature: %m
expands to strerror(errno). I preferred using %dE however because %m in
userspace doesn't consume an argument (which is however necessary in the
kernel as there is no global errno variable). Also this is handled
correctly by the compiler's format string checker as there is a matching
int variable for the %d the compiler notices.

There are quite some code locations that could be adapted to benefit
from this:

	$ git grep -E '^\s*(printk|(kv?as|sn?|vs(c?n)?)printf|(kvm|dev|pr)_(emerg|alert|crit|err|warn(ing)?|notice|info|cont|devel|debug|dbg))\(.*(\(%d\)|: %d)\\n' v5.3-rc5 | wc -l
	9141

I expect there are some false negatives where the match is distributed
over two or more lines and so isn't found.

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
---
Hello,

in v1 I handled both positive and negative errors which was challenged.
I decided to drop that and only handle the (common) negative values.
Readding handling of positive values later is easier than dropping it.

Sergey Senozhatsky and Enrico Weigelt suggested to use strerror as name
for the function that I called errno2errstr. (In v1 it was not a
separate function). I didn't pick this up because the semantic of
strerror in userspace is different. It returns something like
"Interrupted system call" while errno2errstr only yields "EINTR".

I dropped EWOULDBLOCK from the list of codes as it is on all Linux archs
identical to EAGAIN and the way the lookup works now breaks otherwise.
(And having it wasn't useful already in v1.)

Rasmus Villemoes pointed out that the way I wrote the resulting string
to the buffer was broken. This is fixed according to his suggestion by
using string_nocheck(). I also added a test to lib/test_printf.c as he
requested.

Petr Mladek had some concerns:
> The array is long, created by cpu&paste, the index of each code
> is not obvious.

Yeah right, the array is long. This cannot really be changed because we
have that many error codes. I don't understand your concern about the
index not being obvious. The array was just a list of (number, string)
pairs where the position in the array didn't have any semantic.

I agree it would be nice to have only one place that has a collection of
error codes. I thought about reorganizing how the error constants are
defined, i.e. doing something like:

        DEFINE_ERROR(EIO, 5, "I/O error")

but I don't see a possibility to let this expand to

        #define EIO 5

without resorting to another macro language. If that were possible the
list of DEFINE_ERRORs could be reused to help generating the code for
errno2errstr. So if you have a good idea, please speak up.

> There are ideas to make the code even more tricky to reduce
> the size, keep it fast.

I think Enrico Weigelt's suggestion to use a case is the best
performance-wise so that's what I picked up. Also I hope that
performance isn't that important because the need to print an error
should not be so common that it really hurts in production.

> Both, %dE modifier and the output format (ECODE) is non-standard.

Yeah, obviously right. The problem is that the new modifier does
something that wasn't implemented before, so it cannot match any
standard. %pI is only known on Linux either, so I think being
non-standard is a weak argument. For user consumption it would be nice
if we'd get

        probing failed (I/O Error)

from pr_info("probing failed (%dE)\n", -EIO); but that would be still
more expensive because the collection of string constants would become
bigger that it is already now and "EIO" is the right one to search for
when debugging the underlaying problem. So I think "EIO" is even more
useful than "I/O Error".

> Upper letters gain a lot of attention. But the error code is
> only helper information. Also many error codes are misleading because
> they are used either wrongly or there was no better available.

This isn't really an argument against the patch I think. Sure, if a
function returned (say) EIO while ETIMEOUT would be better, my patch
doesn't improve that detail. Still

        mydev: Failed to initialize blablub: EIO

is more expressive than

        mydev: Failed to initialize blablub: -5

. With "EIO" in the message it is not worse than with "-5" independant of the
question if EIO is the right error code used.

> There is no proof that this approach would be widely acceptable for
> subsystem maintainers. Some might not like mass and "blind" code
> changes. Some might not like the output at all.

I don't intend to mass convert existing code. I would restrict myself to
updating the documentation and then maybe send a patch per subsystem as an
example to let maintainers know and judge for themselves if they like it or
not. And if it doesn't get picked up, we can just remove the feature again next
year (or so).

I dropped the example conversion, I think the idea should be clear now
even without an explicit example.

Best regards
Uwe

---
 Documentation/core-api/printk-formats.rst |   3 +
 lib/test_printf.c                         |   1 +
 lib/vsprintf.c                            | 188 +++++++++++++++++++++-
 3 files changed, 191 insertions(+), 1 deletion(-)

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index c6224d039bcb..81002414f956 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -35,6 +35,9 @@ Integer types
 		u64			%llu or %llx
 
 
+To print the name that corresponds to an integer error constant, use %dE and
+pass the int.
+
 If <type> is dependent on a config option for its size (e.g., sector_t,
 blkcnt_t) or is architecture-dependent for its size (e.g., tcflag_t), use a
 format specifier of its largest possible type and explicitly cast to it.
diff --git a/lib/test_printf.c b/lib/test_printf.c
index 944eb50f3862..9b0687928717 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -174,6 +174,7 @@ test_number(void)
 		test("0xfffffff0|0xf0|0xf0", "%#02x|%#02x|%#02x", val, val & 0xff, (u8)val);
 	}
 #endif
+	test("EIO", "%dE", -EIO);
 }
 
 static void __init
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index b0967cf17137..8fed03610402 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -682,6 +682,187 @@ static char *pointer_string(char *buf, char *end,
 	return number(buf, end, (unsigned long int)ptr, spec);
 }
 
+#define ERRORCODES \
+	ERRORCODE(EPERM) \
+	ERRORCODE(ENOENT) \
+	ERRORCODE(ESRCH) \
+	ERRORCODE(EINTR) \
+	ERRORCODE(EIO) \
+	ERRORCODE(ENXIO) \
+	ERRORCODE(E2BIG) \
+	ERRORCODE(ENOEXEC) \
+	ERRORCODE(EBADF) \
+	ERRORCODE(ECHILD) \
+	ERRORCODE(EAGAIN) \
+	ERRORCODE(ENOMEM) \
+	ERRORCODE(EACCES) \
+	ERRORCODE(EFAULT) \
+	ERRORCODE(ENOTBLK) \
+	ERRORCODE(EBUSY) \
+	ERRORCODE(EEXIST) \
+	ERRORCODE(EXDEV) \
+	ERRORCODE(ENODEV) \
+	ERRORCODE(ENOTDIR) \
+	ERRORCODE(EISDIR) \
+	ERRORCODE(EINVAL) \
+	ERRORCODE(ENFILE) \
+	ERRORCODE(EMFILE) \
+	ERRORCODE(ENOTTY) \
+	ERRORCODE(ETXTBSY) \
+	ERRORCODE(EFBIG) \
+	ERRORCODE(ENOSPC) \
+	ERRORCODE(ESPIPE) \
+	ERRORCODE(EROFS) \
+	ERRORCODE(EMLINK) \
+	ERRORCODE(EPIPE) \
+	ERRORCODE(EDOM) \
+	ERRORCODE(ERANGE) \
+	ERRORCODE(EDEADLK) \
+	ERRORCODE(ENAMETOOLONG) \
+	ERRORCODE(ENOLCK) \
+	ERRORCODE(ENOSYS) \
+	ERRORCODE(ENOTEMPTY) \
+	ERRORCODE(ELOOP) \
+	ERRORCODE(ENOMSG) \
+	ERRORCODE(EIDRM) \
+	ERRORCODE(ECHRNG) \
+	ERRORCODE(EL2NSYNC) \
+	ERRORCODE(EL3HLT) \
+	ERRORCODE(EL3RST) \
+	ERRORCODE(ELNRNG) \
+	ERRORCODE(EUNATCH) \
+	ERRORCODE(ENOCSI) \
+	ERRORCODE(EL2HLT) \
+	ERRORCODE(EBADE) \
+	ERRORCODE(EBADR) \
+	ERRORCODE(EXFULL) \
+	ERRORCODE(ENOANO) \
+	ERRORCODE(EBADRQC) \
+	ERRORCODE(EBADSLT) \
+	ERRORCODE(EBFONT) \
+	ERRORCODE(ENOSTR) \
+	ERRORCODE(ENODATA) \
+	ERRORCODE(ETIME) \
+	ERRORCODE(ENOSR) \
+	ERRORCODE(ENONET) \
+	ERRORCODE(ENOPKG) \
+	ERRORCODE(EREMOTE) \
+	ERRORCODE(ENOLINK) \
+	ERRORCODE(EADV) \
+	ERRORCODE(ESRMNT) \
+	ERRORCODE(ECOMM) \
+	ERRORCODE(EPROTO) \
+	ERRORCODE(EMULTIHOP) \
+	ERRORCODE(EDOTDOT) \
+	ERRORCODE(EBADMSG) \
+	ERRORCODE(EOVERFLOW) \
+	ERRORCODE(ENOTUNIQ) \
+	ERRORCODE(EBADFD) \
+	ERRORCODE(EREMCHG) \
+	ERRORCODE(ELIBACC) \
+	ERRORCODE(ELIBBAD) \
+	ERRORCODE(ELIBSCN) \
+	ERRORCODE(ELIBMAX) \
+	ERRORCODE(ELIBEXEC) \
+	ERRORCODE(EILSEQ) \
+	ERRORCODE(ERESTART) \
+	ERRORCODE(ESTRPIPE) \
+	ERRORCODE(EUSERS) \
+	ERRORCODE(ENOTSOCK) \
+	ERRORCODE(EDESTADDRREQ) \
+	ERRORCODE(EMSGSIZE) \
+	ERRORCODE(EPROTOTYPE) \
+	ERRORCODE(ENOPROTOOPT) \
+	ERRORCODE(EPROTONOSUPPORT) \
+	ERRORCODE(ESOCKTNOSUPPORT) \
+	ERRORCODE(EOPNOTSUPP) \
+	ERRORCODE(EPFNOSUPPORT) \
+	ERRORCODE(EAFNOSUPPORT) \
+	ERRORCODE(EADDRINUSE) \
+	ERRORCODE(EADDRNOTAVAIL) \
+	ERRORCODE(ENETDOWN) \
+	ERRORCODE(ENETUNREACH) \
+	ERRORCODE(ENETRESET) \
+	ERRORCODE(ECONNABORTED) \
+	ERRORCODE(ECONNRESET) \
+	ERRORCODE(ENOBUFS) \
+	ERRORCODE(EISCONN) \
+	ERRORCODE(ENOTCONN) \
+	ERRORCODE(ESHUTDOWN) \
+	ERRORCODE(ETOOMANYREFS) \
+	ERRORCODE(ETIMEDOUT) \
+	ERRORCODE(ECONNREFUSED) \
+	ERRORCODE(EHOSTDOWN) \
+	ERRORCODE(EHOSTUNREACH) \
+	ERRORCODE(EALREADY) \
+	ERRORCODE(EINPROGRESS) \
+	ERRORCODE(ESTALE) \
+	ERRORCODE(EUCLEAN) \
+	ERRORCODE(ENOTNAM) \
+	ERRORCODE(ENAVAIL) \
+	ERRORCODE(EISNAM) \
+	ERRORCODE(EREMOTEIO) \
+	ERRORCODE(EDQUOT) \
+	ERRORCODE(ENOMEDIUM) \
+	ERRORCODE(EMEDIUMTYPE) \
+	ERRORCODE(ECANCELED) \
+	ERRORCODE(ENOKEY) \
+	ERRORCODE(EKEYEXPIRED) \
+	ERRORCODE(EKEYREVOKED) \
+	ERRORCODE(EKEYREJECTED) \
+	ERRORCODE(EOWNERDEAD) \
+	ERRORCODE(ENOTRECOVERABLE) \
+	ERRORCODE(ERFKILL) \
+	ERRORCODE(EHWPOISON) \
+	ERRORCODE(ERESTARTSYS) \
+	ERRORCODE(ERESTARTNOINTR) \
+	ERRORCODE(ERESTARTNOHAND) \
+	ERRORCODE(ENOIOCTLCMD) \
+	ERRORCODE(ERESTART_RESTARTBLOCK) \
+	ERRORCODE(EPROBE_DEFER) \
+	ERRORCODE(EOPENSTALE) \
+	ERRORCODE(ENOPARAM) \
+	ERRORCODE(EBADHANDLE) \
+	ERRORCODE(ENOTSYNC) \
+	ERRORCODE(EBADCOOKIE) \
+	ERRORCODE(ENOTSUPP) \
+	ERRORCODE(ETOOSMALL) \
+	ERRORCODE(ESERVERFAULT) \
+	ERRORCODE(EBADTYPE) \
+	ERRORCODE(EJUKEBOX) \
+	ERRORCODE(EIOCBQUEUED) \
+	ERRORCODE(ERECALLCONFLICT)
+
+#define ERRORCODE(x) 	case x: return #x;
+
+static const char *errint2errstr(int err)
+{
+	switch(-err) {
+	ERRORCODES
+	}
+	return NULL;
+}
+#undef ERRORCODE
+
+static noinline_for_stack
+char *errstr(char *buf, char *end, unsigned long long num,
+	     struct printf_spec spec)
+{
+	const char *errstr = errint2errstr(num);
+	static const struct printf_spec strspec = {
+		.field_width = -1,
+		.precision = 10,
+		.flags = LEFT,
+	};
+
+	if (!errstr) {
+		/* fall back to ordinary number */
+		return number(buf, end, num, spec);
+	}
+
+	return string_nocheck(buf, end, errstr, strspec);
+}
+
 /* Make pointers available for printing early in the boot sequence. */
 static int debug_boot_weak_hash __ro_after_init;
 
@@ -2566,7 +2747,12 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 				num = va_arg(args, unsigned int);
 			}
 
-			str = number(str, end, num, spec);
+			if (spec.type == FORMAT_TYPE_INT && *fmt == 'E') {
+				fmt++;
+				str = errstr(str, end, num, spec);
+			} else {
+				str = number(str, end, num, spec);
+			}
 		}
 	}
 
-- 
2.23.0


^ permalink raw reply related

* Re: [PATCH v3 6/7] dt-bindings: serial: Document Freescale LINFlexD UART
From: Rob Herring @ 2019-08-27 20:26 UTC (permalink / raw)
  To: Stefan-gabriel Mirea
  Cc: corbet@lwn.net, robh+dt@kernel.org, mark.rutland@arm.com,
	gregkh@linuxfoundation.org, catalin.marinas@arm.com,
	will@kernel.org, shawnguo@kernel.org, Leo Li, jslaby@suse.com,
	linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
	devicetree@vger.kernel.org, linux-serial@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, Cosmin Stefan Stoica,
	Larisa Ileana Grigore
In-Reply-To: <20190823191115.18490-7-stefan-gabriel.mirea@nxp.com>

On Fri, 23 Aug 2019 19:11:40 +0000, Stefan-gabriel Mirea wrote:
> From: Stoica Cosmin-Stefan <cosmin.stoica@nxp.com>
> 
> Add documentation for the serial communication interface module (LINFlexD),
> found in two instances on S32V234.
> 
> Signed-off-by: Stoica Cosmin-Stefan <cosmin.stoica@nxp.com>
> Signed-off-by: Larisa Grigore <Larisa.Grigore@nxp.com>
> Signed-off-by: Stefan-Gabriel Mirea <stefan-gabriel.mirea@nxp.com>
> ---
>  .../bindings/serial/fsl,s32-linflexuart.txt   | 22 +++++++++++++++++++
>  1 file changed, 22 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.txt
> 

Reviewed-by: Rob Herring <robh@kernel.org>

^ permalink raw reply

* [PATCH 2/5] rcu/tree: Add multiple in-flight batches of kfree_rcu work
From: Joel Fernandes (Google) @ 2019-08-27 19:01 UTC (permalink / raw)
  To: linux-kernel
  Cc: Joel Fernandes (Google), Paul E . McKenney, byungchul.park,
	Josh Triplett, Lai Jiangshan, linux-doc, Mathieu Desnoyers, rcu,
	Steven Rostedt, kernel-team

During testing, it was observed that amount of memory consumed due
kfree_rcu() batching is 300-400MB. Previously we had only a single
head_free pointer pointing to the list of rcu_head(s) that are to be
freed after a grace period. Until this list is drained, we cannot queue
any more objects on it since such objects may not be ready to be
reclaimed when the worker thread eventually gets to drainin g the
head_free list.

We can do better by maintaining multiple lists as done by this patch.
Testing shows that memory consumption came down by around 100-150MB with
just adding another list. Adding more than 1 additional list did not
show any improvement.

Suggested-by: Paul E. McKenney <paulmck@linux.ibm.com>
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 kernel/rcu/tree.c | 64 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 45 insertions(+), 19 deletions(-)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 4f7c3096d786..9b9ae4db1c2d 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -2688,28 +2688,38 @@ EXPORT_SYMBOL_GPL(call_rcu);
 
 /* Maximum number of jiffies to wait before draining a batch. */
 #define KFREE_DRAIN_JIFFIES (HZ / 50)
+#define KFREE_N_BATCHES 2
+
+struct kfree_rcu_work {
+	/* The rcu_work node for queuing work with queue_rcu_work(). The work
+	 * is done after a grace period.
+	 */
+	struct rcu_work rcu_work;
+
+	/* The list of objects that have now left ->head and are queued for
+	 * freeing after a grace period.
+	 */
+	struct rcu_head *head_free;
+
+	struct kfree_rcu_cpu *krcp;
+};
+static DEFINE_PER_CPU(__typeof__(struct kfree_rcu_work)[KFREE_N_BATCHES], krw);
 
 /*
  * Maximum number of kfree(s) to batch, if this limit is hit then the batch of
  * kfree(s) is queued for freeing after a grace period, right away.
  */
 struct kfree_rcu_cpu {
-	/* The rcu_work node for queuing work with queue_rcu_work(). The work
-	 * is done after a grace period.
-	 */
-	struct rcu_work rcu_work;
 
 	/* The list of objects being queued in a batch but are not yet
 	 * scheduled to be freed.
 	 */
 	struct rcu_head *head;
 
-	/* The list of objects that have now left ->head and are queued for
-	 * freeing after a grace period.
-	 */
-	struct rcu_head *head_free;
+	/* Pointer to the per-cpu array of kfree_rcu_work structures */
+	struct kfree_rcu_work *krwp;
 
-	/* Protect concurrent access to this structure. */
+	/* Protect concurrent access to this structure and kfree_rcu_work. */
 	spinlock_t lock;
 
 	/* The delayed work that flushes ->head to ->head_free incase ->head
@@ -2730,12 +2740,14 @@ static void kfree_rcu_work(struct work_struct *work)
 {
 	unsigned long flags;
 	struct rcu_head *head, *next;
-	struct kfree_rcu_cpu *krcp = container_of(to_rcu_work(work),
-					struct kfree_rcu_cpu, rcu_work);
+	struct kfree_rcu_work *krwp = container_of(to_rcu_work(work),
+					struct kfree_rcu_work, rcu_work);
+	struct kfree_rcu_cpu *krcp;
+
+	krcp = krwp->krcp;
 
 	spin_lock_irqsave(&krcp->lock, flags);
-	head = krcp->head_free;
-	krcp->head_free = NULL;
+	head = xchg(&krwp->head_free, NULL);
 	spin_unlock_irqrestore(&krcp->lock, flags);
 
 	/*
@@ -2758,19 +2770,28 @@ static void kfree_rcu_work(struct work_struct *work)
  */
 static inline bool queue_kfree_rcu_work(struct kfree_rcu_cpu *krcp)
 {
+	int i = 0;
+	struct kfree_rcu_work *krwp = NULL;
+
 	lockdep_assert_held(&krcp->lock);
+	while (i < KFREE_N_BATCHES) {
+		if (!krcp->krwp[i].head_free) {
+			krwp = &(krcp->krwp[i]);
+			break;
+		}
+		i++;
+	}
 
-	/* If a previous RCU batch work is already in progress, we cannot queue
+	/* If both RCU batches are already in progress, we cannot queue
 	 * another one, just refuse the optimization and it will be retried
 	 * again in KFREE_DRAIN_JIFFIES time.
 	 */
-	if (krcp->head_free)
+	if (!krwp)
 		return false;
 
-	krcp->head_free = krcp->head;
-	krcp->head = NULL;
-	INIT_RCU_WORK(&krcp->rcu_work, kfree_rcu_work);
-	queue_rcu_work(system_wq, &krcp->rcu_work);
+	krwp->head_free = xchg(&krcp->head, NULL);
+	INIT_RCU_WORK(&krwp->rcu_work, kfree_rcu_work);
+	queue_rcu_work(system_wq, &krwp->rcu_work);
 
 	return true;
 }
@@ -3736,8 +3757,13 @@ static void __init kfree_rcu_batch_init(void)
 
 	for_each_possible_cpu(cpu) {
 		struct kfree_rcu_cpu *krcp = per_cpu_ptr(&krc, cpu);
+		struct kfree_rcu_work *krwp = &(per_cpu(krw, cpu)[0]);
+		int i = KFREE_N_BATCHES;
 
 		spin_lock_init(&krcp->lock);
+		krcp->krwp = krwp;
+		while (i--)
+			krwp[i].krcp = krcp;
 		INIT_DELAYED_WORK(&krcp->monitor_work, kfree_rcu_monitor);
 	}
 }
-- 
2.23.0.187.g17f5b7556c-goog


^ permalink raw reply related

* [PATCH 4/5] rcu: Remove kfree_rcu() special casing and lazy handling
From: Joel Fernandes (Google) @ 2019-08-27 19:01 UTC (permalink / raw)
  To: linux-kernel
  Cc: Joel Fernandes (Google), Paul E . McKenney, byungchul.park,
	Josh Triplett, Lai Jiangshan, linux-doc, Mathieu Desnoyers, rcu,
	Steven Rostedt, kernel-team

Remove kfree_rcu() special casing and lazy handling from RCU.
For Tiny RCU we fold the special handling into just Tiny RCU code.

Suggested-by: Paul E. McKenney <paulmck@linux.ibm.com>
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 Documentation/RCU/stallwarn.txt | 13 +++------
 include/linux/rcu_segcblist.h   |  2 --
 include/trace/events/rcu.h      | 32 +++++++++-------------
 kernel/rcu/rcu.h                | 27 -------------------
 kernel/rcu/rcu_segcblist.c      | 25 +++--------------
 kernel/rcu/rcu_segcblist.h      | 25 ++---------------
 kernel/rcu/srcutree.c           |  4 +--
 kernel/rcu/tiny.c               | 29 +++++++++++++++++++-
 kernel/rcu/tree.c               | 48 ++++++++++++++++++++++++---------
 kernel/rcu/tree.h               |  1 -
 kernel/rcu/tree_plugin.h        | 42 +++++++----------------------
 kernel/rcu/tree_stall.h         |  6 ++---
 12 files changed, 97 insertions(+), 157 deletions(-)

diff --git a/Documentation/RCU/stallwarn.txt b/Documentation/RCU/stallwarn.txt
index f48f4621ccbc..2c02b8de01e7 100644
--- a/Documentation/RCU/stallwarn.txt
+++ b/Documentation/RCU/stallwarn.txt
@@ -225,18 +225,12 @@ an estimate of the total number of RCU callbacks queued across all CPUs
 In kernels with CONFIG_RCU_FAST_NO_HZ, more information is printed
 for each CPU:
 
-	0: (64628 ticks this GP) idle=dd5/3fffffffffffffff/0 softirq=82/543 last_accelerate: a345/d342 Nonlazy posted: ..D
+	0: (64628 ticks this GP) idle=dd5/3fffffffffffffff/0 softirq=82/543 last_accelerate: a345/d342 dyntick_enabled: 1
 
 The "last_accelerate:" prints the low-order 16 bits (in hex) of the
 jiffies counter when this CPU last invoked rcu_try_advance_all_cbs()
 from rcu_needs_cpu() or last invoked rcu_accelerate_cbs() from
-rcu_prepare_for_idle().  The "Nonlazy posted:" indicates lazy-callback
-status, so that an "l" indicates that all callbacks were lazy at the start
-of the last idle period and an "L" indicates that there are currently
-no non-lazy callbacks (in both cases, "." is printed otherwise, as
-shown above) and "D" indicates that dyntick-idle processing is enabled
-("." is printed otherwise, for example, if disabled via the "nohz="
-kernel boot parameter).
+rcu_prepare_for_idle().
 
 If the grace period ends just as the stall warning starts printing,
 there will be a spurious stall-warning message, which will include
@@ -249,7 +243,8 @@ possible for a zero-jiffy stall to be flagged in this case, depending
 on how the stall warning and the grace-period initialization happen to
 interact.  Please note that it is not possible to entirely eliminate this
 sort of false positive without resorting to things like stop_machine(),
-which is overkill for this sort of problem.
+which is overkill for this sort of problem. "dyntick_enabled: 1" indicates that
+dyntick-idle processing is enabled.
 
 If all CPUs and tasks have passed through quiescent states, but the
 grace period has nevertheless failed to end, the stall-warning splat
diff --git a/include/linux/rcu_segcblist.h b/include/linux/rcu_segcblist.h
index 646759042333..b36afe7b22c9 100644
--- a/include/linux/rcu_segcblist.h
+++ b/include/linux/rcu_segcblist.h
@@ -22,7 +22,6 @@ struct rcu_cblist {
 	struct rcu_head *head;
 	struct rcu_head **tail;
 	long len;
-	long len_lazy;
 };
 
 #define RCU_CBLIST_INITIALIZER(n) { .head = NULL, .tail = &n.head }
@@ -73,7 +72,6 @@ struct rcu_segcblist {
 #else
 	long len;
 #endif
-	long len_lazy;
 	u8 enabled;
 	u8 offloaded;
 };
diff --git a/include/trace/events/rcu.h b/include/trace/events/rcu.h
index 694bd040cf51..0dd3478597ee 100644
--- a/include/trace/events/rcu.h
+++ b/include/trace/events/rcu.h
@@ -474,16 +474,14 @@ TRACE_EVENT_RCU(rcu_dyntick,
  */
 TRACE_EVENT_RCU(rcu_callback,
 
-	TP_PROTO(const char *rcuname, struct rcu_head *rhp, long qlen_lazy,
-		 long qlen),
+	TP_PROTO(const char *rcuname, struct rcu_head *rhp, long qlen),
 
-	TP_ARGS(rcuname, rhp, qlen_lazy, qlen),
+	TP_ARGS(rcuname, rhp, qlen),
 
 	TP_STRUCT__entry(
 		__field(const char *, rcuname)
 		__field(void *, rhp)
 		__field(void *, func)
-		__field(long, qlen_lazy)
 		__field(long, qlen)
 	),
 
@@ -491,13 +489,12 @@ TRACE_EVENT_RCU(rcu_callback,
 		__entry->rcuname = rcuname;
 		__entry->rhp = rhp;
 		__entry->func = rhp->func;
-		__entry->qlen_lazy = qlen_lazy;
 		__entry->qlen = qlen;
 	),
 
-	TP_printk("%s rhp=%p func=%ps %ld/%ld",
+	TP_printk("%s rhp=%p func=%ps %ld",
 		  __entry->rcuname, __entry->rhp, __entry->func,
-		  __entry->qlen_lazy, __entry->qlen)
+		  __entry->qlen)
 );
 
 /*
@@ -511,15 +508,14 @@ TRACE_EVENT_RCU(rcu_callback,
 TRACE_EVENT_RCU(rcu_kfree_callback,
 
 	TP_PROTO(const char *rcuname, struct rcu_head *rhp, unsigned long offset,
-		 long qlen_lazy, long qlen),
+		 long qlen),
 
-	TP_ARGS(rcuname, rhp, offset, qlen_lazy, qlen),
+	TP_ARGS(rcuname, rhp, offset, qlen),
 
 	TP_STRUCT__entry(
 		__field(const char *, rcuname)
 		__field(void *, rhp)
 		__field(unsigned long, offset)
-		__field(long, qlen_lazy)
 		__field(long, qlen)
 	),
 
@@ -527,13 +523,12 @@ TRACE_EVENT_RCU(rcu_kfree_callback,
 		__entry->rcuname = rcuname;
 		__entry->rhp = rhp;
 		__entry->offset = offset;
-		__entry->qlen_lazy = qlen_lazy;
 		__entry->qlen = qlen;
 	),
 
-	TP_printk("%s rhp=%p func=%ld %ld/%ld",
+	TP_printk("%s rhp=%p func=%ld %ld",
 		  __entry->rcuname, __entry->rhp, __entry->offset,
-		  __entry->qlen_lazy, __entry->qlen)
+		  __entry->qlen)
 );
 
 /*
@@ -545,27 +540,24 @@ TRACE_EVENT_RCU(rcu_kfree_callback,
  */
 TRACE_EVENT_RCU(rcu_batch_start,
 
-	TP_PROTO(const char *rcuname, long qlen_lazy, long qlen, long blimit),
+	TP_PROTO(const char *rcuname, long qlen, long blimit),
 
-	TP_ARGS(rcuname, qlen_lazy, qlen, blimit),
+	TP_ARGS(rcuname, qlen, blimit),
 
 	TP_STRUCT__entry(
 		__field(const char *, rcuname)
-		__field(long, qlen_lazy)
 		__field(long, qlen)
 		__field(long, blimit)
 	),
 
 	TP_fast_assign(
 		__entry->rcuname = rcuname;
-		__entry->qlen_lazy = qlen_lazy;
 		__entry->qlen = qlen;
 		__entry->blimit = blimit;
 	),
 
-	TP_printk("%s CBs=%ld/%ld bl=%ld",
-		  __entry->rcuname, __entry->qlen_lazy, __entry->qlen,
-		  __entry->blimit)
+	TP_printk("%s CBs=%ld bl=%ld",
+		  __entry->rcuname, __entry->qlen, __entry->blimit)
 );
 
 /*
diff --git a/kernel/rcu/rcu.h b/kernel/rcu/rcu.h
index aeec70fda82c..3f5a30ca7ed7 100644
--- a/kernel/rcu/rcu.h
+++ b/kernel/rcu/rcu.h
@@ -198,33 +198,6 @@ static inline void debug_rcu_head_unqueue(struct rcu_head *head)
 }
 #endif	/* #else !CONFIG_DEBUG_OBJECTS_RCU_HEAD */
 
-void kfree(const void *);
-
-/*
- * Reclaim the specified callback, either by invoking it (non-lazy case)
- * or freeing it directly (lazy case).  Return true if lazy, false otherwise.
- */
-static inline bool __rcu_reclaim(const char *rn, struct rcu_head *head)
-{
-	rcu_callback_t f;
-	unsigned long offset = (unsigned long)head->func;
-
-	rcu_lock_acquire(&rcu_callback_map);
-	if (__is_kfree_rcu_offset(offset)) {
-		trace_rcu_invoke_kfree_callback(rn, head, offset);
-		kfree((void *)head - offset);
-		rcu_lock_release(&rcu_callback_map);
-		return true;
-	} else {
-		trace_rcu_invoke_callback(rn, head);
-		f = head->func;
-		WRITE_ONCE(head->func, (rcu_callback_t)0L);
-		f(head);
-		rcu_lock_release(&rcu_callback_map);
-		return false;
-	}
-}
-
 #ifdef CONFIG_RCU_STALL_COMMON
 
 extern int rcu_cpu_stall_ftrace_dump;
diff --git a/kernel/rcu/rcu_segcblist.c b/kernel/rcu/rcu_segcblist.c
index cbc87b804db9..5f4fd3b8777c 100644
--- a/kernel/rcu/rcu_segcblist.c
+++ b/kernel/rcu/rcu_segcblist.c
@@ -20,14 +20,10 @@ void rcu_cblist_init(struct rcu_cblist *rclp)
 	rclp->head = NULL;
 	rclp->tail = &rclp->head;
 	rclp->len = 0;
-	rclp->len_lazy = 0;
 }
 
 /*
  * Enqueue an rcu_head structure onto the specified callback list.
- * This function assumes that the callback is non-lazy because it
- * is intended for use by no-CBs CPUs, which do not distinguish
- * between lazy and non-lazy RCU callbacks.
  */
 void rcu_cblist_enqueue(struct rcu_cblist *rclp, struct rcu_head *rhp)
 {
@@ -54,7 +50,6 @@ void rcu_cblist_flush_enqueue(struct rcu_cblist *drclp,
 	else
 		drclp->tail = &drclp->head;
 	drclp->len = srclp->len;
-	drclp->len_lazy = srclp->len_lazy;
 	if (!rhp) {
 		rcu_cblist_init(srclp);
 	} else {
@@ -62,16 +57,12 @@ void rcu_cblist_flush_enqueue(struct rcu_cblist *drclp,
 		srclp->head = rhp;
 		srclp->tail = &rhp->next;
 		WRITE_ONCE(srclp->len, 1);
-		srclp->len_lazy = 0;
 	}
 }
 
 /*
  * Dequeue the oldest rcu_head structure from the specified callback
- * list.  This function assumes that the callback is non-lazy, but
- * the caller can later invoke rcu_cblist_dequeued_lazy() if it
- * finds otherwise (and if it cares about laziness).  This allows
- * different users to have different ways of determining laziness.
+ * list.
  */
 struct rcu_head *rcu_cblist_dequeue(struct rcu_cblist *rclp)
 {
@@ -161,7 +152,6 @@ void rcu_segcblist_init(struct rcu_segcblist *rsclp)
 	for (i = 0; i < RCU_CBLIST_NSEGS; i++)
 		rsclp->tails[i] = &rsclp->head;
 	rcu_segcblist_set_len(rsclp, 0);
-	rsclp->len_lazy = 0;
 	rsclp->enabled = 1;
 }
 
@@ -173,7 +163,6 @@ void rcu_segcblist_disable(struct rcu_segcblist *rsclp)
 {
 	WARN_ON_ONCE(!rcu_segcblist_empty(rsclp));
 	WARN_ON_ONCE(rcu_segcblist_n_cbs(rsclp));
-	WARN_ON_ONCE(rcu_segcblist_n_lazy_cbs(rsclp));
 	rsclp->enabled = 0;
 }
 
@@ -253,11 +242,9 @@ bool rcu_segcblist_nextgp(struct rcu_segcblist *rsclp, unsigned long *lp)
  * absolutely not OK for it to ever miss posting a callback.
  */
 void rcu_segcblist_enqueue(struct rcu_segcblist *rsclp,
-			   struct rcu_head *rhp, bool lazy)
+			   struct rcu_head *rhp)
 {
 	rcu_segcblist_inc_len(rsclp);
-	if (lazy)
-		rsclp->len_lazy++;
 	smp_mb(); /* Ensure counts are updated before callback is enqueued. */
 	rhp->next = NULL;
 	WRITE_ONCE(*rsclp->tails[RCU_NEXT_TAIL], rhp);
@@ -275,15 +262,13 @@ void rcu_segcblist_enqueue(struct rcu_segcblist *rsclp,
  * period.  You have been warned.
  */
 bool rcu_segcblist_entrain(struct rcu_segcblist *rsclp,
-			   struct rcu_head *rhp, bool lazy)
+			   struct rcu_head *rhp)
 {
 	int i;
 
 	if (rcu_segcblist_n_cbs(rsclp) == 0)
 		return false;
 	rcu_segcblist_inc_len(rsclp);
-	if (lazy)
-		rsclp->len_lazy++;
 	smp_mb(); /* Ensure counts are updated before callback is entrained. */
 	rhp->next = NULL;
 	for (i = RCU_NEXT_TAIL; i > RCU_DONE_TAIL; i--)
@@ -307,8 +292,6 @@ bool rcu_segcblist_entrain(struct rcu_segcblist *rsclp,
 void rcu_segcblist_extract_count(struct rcu_segcblist *rsclp,
 					       struct rcu_cblist *rclp)
 {
-	rclp->len_lazy += rsclp->len_lazy;
-	rsclp->len_lazy = 0;
 	rclp->len = rcu_segcblist_xchg_len(rsclp, 0);
 }
 
@@ -361,9 +344,7 @@ void rcu_segcblist_extract_pend_cbs(struct rcu_segcblist *rsclp,
 void rcu_segcblist_insert_count(struct rcu_segcblist *rsclp,
 				struct rcu_cblist *rclp)
 {
-	rsclp->len_lazy += rclp->len_lazy;
 	rcu_segcblist_add_len(rsclp, rclp->len);
-	rclp->len_lazy = 0;
 	rclp->len = 0;
 }
 
diff --git a/kernel/rcu/rcu_segcblist.h b/kernel/rcu/rcu_segcblist.h
index 815c2fdd3fcc..5c293afc07b8 100644
--- a/kernel/rcu/rcu_segcblist.h
+++ b/kernel/rcu/rcu_segcblist.h
@@ -15,15 +15,6 @@ static inline long rcu_cblist_n_cbs(struct rcu_cblist *rclp)
 	return READ_ONCE(rclp->len);
 }
 
-/*
- * Account for the fact that a previously dequeued callback turned out
- * to be marked as lazy.
- */
-static inline void rcu_cblist_dequeued_lazy(struct rcu_cblist *rclp)
-{
-	rclp->len_lazy--;
-}
-
 void rcu_cblist_init(struct rcu_cblist *rclp);
 void rcu_cblist_enqueue(struct rcu_cblist *rclp, struct rcu_head *rhp);
 void rcu_cblist_flush_enqueue(struct rcu_cblist *drclp,
@@ -59,18 +50,6 @@ static inline long rcu_segcblist_n_cbs(struct rcu_segcblist *rsclp)
 #endif
 }
 
-/* Return number of lazy callbacks in segmented callback list. */
-static inline long rcu_segcblist_n_lazy_cbs(struct rcu_segcblist *rsclp)
-{
-	return rsclp->len_lazy;
-}
-
-/* Return number of lazy callbacks in segmented callback list. */
-static inline long rcu_segcblist_n_nonlazy_cbs(struct rcu_segcblist *rsclp)
-{
-	return rcu_segcblist_n_cbs(rsclp) - rsclp->len_lazy;
-}
-
 /*
  * Is the specified rcu_segcblist enabled, for example, not corresponding
  * to an offline CPU?
@@ -106,9 +85,9 @@ struct rcu_head *rcu_segcblist_first_cb(struct rcu_segcblist *rsclp);
 struct rcu_head *rcu_segcblist_first_pend_cb(struct rcu_segcblist *rsclp);
 bool rcu_segcblist_nextgp(struct rcu_segcblist *rsclp, unsigned long *lp);
 void rcu_segcblist_enqueue(struct rcu_segcblist *rsclp,
-			   struct rcu_head *rhp, bool lazy);
+			   struct rcu_head *rhp);
 bool rcu_segcblist_entrain(struct rcu_segcblist *rsclp,
-			   struct rcu_head *rhp, bool lazy);
+			   struct rcu_head *rhp);
 void rcu_segcblist_extract_count(struct rcu_segcblist *rsclp,
 				 struct rcu_cblist *rclp);
 void rcu_segcblist_extract_done_cbs(struct rcu_segcblist *rsclp,
diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 5dffade2d7cd..d0a9d5b69087 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -853,7 +853,7 @@ static void __call_srcu(struct srcu_struct *ssp, struct rcu_head *rhp,
 	local_irq_save(flags);
 	sdp = this_cpu_ptr(ssp->sda);
 	spin_lock_rcu_node(sdp);
-	rcu_segcblist_enqueue(&sdp->srcu_cblist, rhp, false);
+	rcu_segcblist_enqueue(&sdp->srcu_cblist, rhp);
 	rcu_segcblist_advance(&sdp->srcu_cblist,
 			      rcu_seq_current(&ssp->srcu_gp_seq));
 	s = rcu_seq_snap(&ssp->srcu_gp_seq);
@@ -1052,7 +1052,7 @@ void srcu_barrier(struct srcu_struct *ssp)
 		sdp->srcu_barrier_head.func = srcu_barrier_cb;
 		debug_rcu_head_queue(&sdp->srcu_barrier_head);
 		if (!rcu_segcblist_entrain(&sdp->srcu_cblist,
-					   &sdp->srcu_barrier_head, 0)) {
+					   &sdp->srcu_barrier_head)) {
 			debug_rcu_head_unqueue(&sdp->srcu_barrier_head);
 			atomic_dec(&ssp->srcu_barrier_cpu_cnt);
 		}
diff --git a/kernel/rcu/tiny.c b/kernel/rcu/tiny.c
index 477b4eb44af5..cc64dc8b3893 100644
--- a/kernel/rcu/tiny.c
+++ b/kernel/rcu/tiny.c
@@ -73,6 +73,33 @@ void rcu_sched_clock_irq(int user)
 	}
 }
 
+void kfree(const void *);
+
+/*
+ * Reclaim the specified callback, either by invoking it for non-kfree cases or
+ * freeing it directly (for kfree). Return true if kfreeing, false otherwise.
+ */
+static inline bool rcu_reclaim_tiny(struct rcu_head *head)
+{
+	rcu_callback_t f;
+	unsigned long offset = (unsigned long)head->func;
+
+	rcu_lock_acquire(&rcu_callback_map);
+	if (__is_kfree_rcu_offset(offset)) {
+		trace_rcu_invoke_kfree_callback("", head, offset);
+		kfree((void *)head - offset);
+		rcu_lock_release(&rcu_callback_map);
+		return true;
+	}
+
+	trace_rcu_invoke_callback("", head);
+	f = head->func;
+	WRITE_ONCE(head->func, (rcu_callback_t)0L);
+	f(head);
+	rcu_lock_release(&rcu_callback_map);
+	return false;
+}
+
 /* Invoke the RCU callbacks whose grace period has elapsed.  */
 static __latent_entropy void rcu_process_callbacks(struct softirq_action *unused)
 {
@@ -100,7 +127,7 @@ static __latent_entropy void rcu_process_callbacks(struct softirq_action *unused
 		prefetch(next);
 		debug_rcu_head_unqueue(list);
 		local_bh_disable();
-		__rcu_reclaim("", list);
+		rcu_reclaim_tiny(list);
 		local_bh_enable();
 		list = next;
 	}
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 64568f12641d..12c17e10f2b4 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -2129,6 +2129,23 @@ int rcutree_dead_cpu(unsigned int cpu)
 	return 0;
 }
 
+/*
+ * Reclaim the specified callback.
+ */
+static inline void rcu_reclaim_tree(const char *rn, struct rcu_head *head)
+{
+	rcu_callback_t f;
+
+	rcu_lock_acquire(&rcu_callback_map);
+	trace_rcu_invoke_callback(rn, head);
+
+	f = head->func;
+	WRITE_ONCE(head->func, (rcu_callback_t)0L);
+	f(head);
+
+	rcu_lock_release(&rcu_callback_map);
+}
+
 /*
  * Invoke any RCU callbacks that have made it to the end of their grace
  * period.  Thottle as specified by rdp->blimit.
@@ -2146,7 +2163,6 @@ static void rcu_do_batch(struct rcu_data *rdp)
 	/* If no callbacks are ready, just return. */
 	if (!rcu_segcblist_ready_cbs(&rdp->cblist)) {
 		trace_rcu_batch_start(rcu_state.name,
-				      rcu_segcblist_n_lazy_cbs(&rdp->cblist),
 				      rcu_segcblist_n_cbs(&rdp->cblist), 0);
 		trace_rcu_batch_end(rcu_state.name, 0,
 				    !rcu_segcblist_empty(&rdp->cblist),
@@ -2168,7 +2184,6 @@ static void rcu_do_batch(struct rcu_data *rdp)
 	if (unlikely(bl > 100))
 		tlimit = local_clock() + rcu_resched_ns;
 	trace_rcu_batch_start(rcu_state.name,
-			      rcu_segcblist_n_lazy_cbs(&rdp->cblist),
 			      rcu_segcblist_n_cbs(&rdp->cblist), bl);
 	rcu_segcblist_extract_done_cbs(&rdp->cblist, &rcl);
 	if (offloaded)
@@ -2181,8 +2196,8 @@ static void rcu_do_batch(struct rcu_data *rdp)
 	rhp = rcu_cblist_dequeue(&rcl);
 	for (; rhp; rhp = rcu_cblist_dequeue(&rcl)) {
 		debug_rcu_head_unqueue(rhp);
-		if (__rcu_reclaim(rcu_state.name, rhp))
-			rcu_cblist_dequeued_lazy(&rcl);
+		rcu_reclaim_tree(rcu_state.name, rhp);
+
 		/*
 		 * Stop only if limit reached and CPU has something to do.
 		 * Note: The rcl structure counts down from zero.
@@ -2585,7 +2600,7 @@ static void rcu_leak_callback(struct rcu_head *rhp)
  * is expected to specify a CPU.
  */
 static void
-__call_rcu(struct rcu_head *head, rcu_callback_t func, bool lazy)
+__call_rcu(struct rcu_head *head, rcu_callback_t func)
 {
 	unsigned long flags;
 	struct rcu_data *rdp;
@@ -2620,18 +2635,17 @@ __call_rcu(struct rcu_head *head, rcu_callback_t func, bool lazy)
 		if (rcu_segcblist_empty(&rdp->cblist))
 			rcu_segcblist_init(&rdp->cblist);
 	}
+
 	if (rcu_nocb_try_bypass(rdp, head, &was_alldone, flags))
 		return; // Enqueued onto ->nocb_bypass, so just leave.
 	/* If we get here, rcu_nocb_try_bypass() acquired ->nocb_lock. */
-	rcu_segcblist_enqueue(&rdp->cblist, head, lazy);
+	rcu_segcblist_enqueue(&rdp->cblist, head);
 	if (__is_kfree_rcu_offset((unsigned long)func))
 		trace_rcu_kfree_callback(rcu_state.name, head,
 					 (unsigned long)func,
-					 rcu_segcblist_n_lazy_cbs(&rdp->cblist),
 					 rcu_segcblist_n_cbs(&rdp->cblist));
 	else
 		trace_rcu_callback(rcu_state.name, head,
-				   rcu_segcblist_n_lazy_cbs(&rdp->cblist),
 				   rcu_segcblist_n_cbs(&rdp->cblist));
 
 	/* Go handle any RCU core processing required. */
@@ -2681,7 +2695,7 @@ __call_rcu(struct rcu_head *head, rcu_callback_t func, bool lazy)
  */
 void call_rcu(struct rcu_head *head, rcu_callback_t func)
 {
-	__call_rcu(head, func, 0);
+	__call_rcu(head, func);
 }
 EXPORT_SYMBOL_GPL(call_rcu);
 
@@ -2755,10 +2769,18 @@ static void kfree_rcu_work(struct work_struct *work)
 	 * access is Ok.
 	 */
 	for (; head; head = next) {
+		unsigned long offset = (unsigned long)head->func;
+
 		next = head->next;
-		/* Could be possible to optimize with kfree_bulk in future */
+
 		debug_rcu_head_unqueue(head);
-		__rcu_reclaim(rcu_state.name, head);
+		rcu_lock_acquire(&rcu_callback_map);
+		trace_rcu_invoke_kfree_callback(rcu_state.name, head, offset);
+
+		/* Could be possible to optimize with kfree_bulk in future */
+		kfree((void *)head - offset);
+
+		rcu_lock_release(&rcu_callback_map);
 		cond_resched_tasks_rcu_qs();
 	}
 }
@@ -2840,7 +2862,7 @@ static void kfree_rcu_monitor(struct work_struct *work)
  */
 void kfree_call_rcu_nobatch(struct rcu_head *head, rcu_callback_t func)
 {
-	__call_rcu(head, func, 1);
+	__call_rcu(head, func);
 }
 EXPORT_SYMBOL_GPL(kfree_call_rcu_nobatch);
 
@@ -3094,7 +3116,7 @@ static void rcu_barrier_func(void *unused)
 	debug_rcu_head_queue(&rdp->barrier_head);
 	rcu_nocb_lock(rdp);
 	WARN_ON_ONCE(!rcu_nocb_flush_bypass(rdp, NULL, jiffies));
-	if (rcu_segcblist_entrain(&rdp->cblist, &rdp->barrier_head, 0)) {
+	if (rcu_segcblist_entrain(&rdp->cblist, &rdp->barrier_head)) {
 		atomic_inc(&rcu_state.barrier_cpu_count);
 	} else {
 		debug_rcu_head_unqueue(&rdp->barrier_head);
diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
index 055c31781d3a..15405420b40c 100644
--- a/kernel/rcu/tree.h
+++ b/kernel/rcu/tree.h
@@ -183,7 +183,6 @@ struct rcu_data {
 	bool rcu_urgent_qs;		/* GP old need light quiescent state. */
 	bool rcu_forced_tick;		/* Forced tick to provide QS. */
 #ifdef CONFIG_RCU_FAST_NO_HZ
-	bool all_lazy;			/* All CPU's CBs lazy at idle start? */
 	unsigned long last_accelerate;	/* Last jiffy CBs were accelerated. */
 	unsigned long last_advance_all;	/* Last jiffy CBs were all advanced. */
 	int tick_nohz_enabled_snap;	/* Previously seen value from sysfs. */
diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
index 2defc7fe74c3..0d0fa615c5fa 100644
--- a/kernel/rcu/tree_plugin.h
+++ b/kernel/rcu/tree_plugin.h
@@ -1274,21 +1274,15 @@ static void rcu_prepare_for_idle(void)
  *	number, be warned: Setting RCU_IDLE_GP_DELAY too high can hang your
  *	system.  And if you are -that- concerned about energy efficiency,
  *	just power the system down and be done with it!
- * RCU_IDLE_LAZY_GP_DELAY gives the number of jiffies that a CPU is
- *	permitted to sleep in dyntick-idle mode with only lazy RCU
- *	callbacks pending.  Setting this too high can OOM your system.
  *
  * The values below work well in practice.  If future workloads require
  * adjustment, they can be converted into kernel config parameters, though
  * making the state machine smarter might be a better option.
  */
 #define RCU_IDLE_GP_DELAY 4		/* Roughly one grace period. */
-#define RCU_IDLE_LAZY_GP_DELAY (6 * HZ)	/* Roughly six seconds. */
 
 static int rcu_idle_gp_delay = RCU_IDLE_GP_DELAY;
 module_param(rcu_idle_gp_delay, int, 0644);
-static int rcu_idle_lazy_gp_delay = RCU_IDLE_LAZY_GP_DELAY;
-module_param(rcu_idle_lazy_gp_delay, int, 0644);
 
 /*
  * Try to advance callbacks on the current CPU, but only if it has been
@@ -1327,8 +1321,7 @@ static bool __maybe_unused rcu_try_advance_all_cbs(void)
 /*
  * Allow the CPU to enter dyntick-idle mode unless it has callbacks ready
  * to invoke.  If the CPU has callbacks, try to advance them.  Tell the
- * caller to set the timeout based on whether or not there are non-lazy
- * callbacks.
+ * caller about what to set the timeout.
  *
  * The caller must have disabled interrupts.
  */
@@ -1354,25 +1347,19 @@ int rcu_needs_cpu(u64 basemono, u64 *nextevt)
 	}
 	rdp->last_accelerate = jiffies;
 
-	/* Request timer delay depending on laziness, and round. */
-	rdp->all_lazy = !rcu_segcblist_n_nonlazy_cbs(&rdp->cblist);
-	if (rdp->all_lazy) {
-		dj = round_jiffies(rcu_idle_lazy_gp_delay + jiffies) - jiffies;
-	} else {
-		dj = round_up(rcu_idle_gp_delay + jiffies,
-			       rcu_idle_gp_delay) - jiffies;
-	}
+	/* Request timer and round. */
+	dj = round_up(rcu_idle_gp_delay + jiffies,
+		      rcu_idle_gp_delay) - jiffies;
+
 	*nextevt = basemono + dj * TICK_NSEC;
 	return 0;
 }
 
 /*
- * Prepare a CPU for idle from an RCU perspective.  The first major task
- * is to sense whether nohz mode has been enabled or disabled via sysfs.
- * The second major task is to check to see if a non-lazy callback has
- * arrived at a CPU that previously had only lazy callbacks.  The third
- * major task is to accelerate (that is, assign grace-period numbers to)
- * any recently arrived callbacks.
+ * Prepare a CPU for idle from an RCU perspective.  The first major task is to
+ * sense whether nohz mode has been enabled or disabled via sysfs.  The second
+ * major task is to accelerate (that is, assign grace-period numbers to) any
+ * recently arrived callbacks.
  *
  * The caller must have disabled interrupts.
  */
@@ -1398,17 +1385,6 @@ static void rcu_prepare_for_idle(void)
 	if (!tne)
 		return;
 
-	/*
-	 * If a non-lazy callback arrived at a CPU having only lazy
-	 * callbacks, invoke RCU core for the side-effect of recalculating
-	 * idle duration on re-entry to idle.
-	 */
-	if (rdp->all_lazy && rcu_segcblist_n_nonlazy_cbs(&rdp->cblist)) {
-		rdp->all_lazy = false;
-		invoke_rcu_core();
-		return;
-	}
-
 	/*
 	 * If we have not yet accelerated this jiffy, accelerate all
 	 * callbacks on this CPU.
diff --git a/kernel/rcu/tree_stall.h b/kernel/rcu/tree_stall.h
index 841ab43f3e60..67530c23c708 100644
--- a/kernel/rcu/tree_stall.h
+++ b/kernel/rcu/tree_stall.h
@@ -263,11 +263,9 @@ static void print_cpu_stall_fast_no_hz(char *cp, int cpu)
 {
 	struct rcu_data *rdp = &per_cpu(rcu_data, cpu);
 
-	sprintf(cp, "last_accelerate: %04lx/%04lx, Nonlazy posted: %c%c%c",
+	sprintf(cp, "last_accelerate: %04lx/%04lx dyntick_enabled: %d",
 		rdp->last_accelerate & 0xffff, jiffies & 0xffff,
-		".l"[rdp->all_lazy],
-		".L"[!rcu_segcblist_n_nonlazy_cbs(&rdp->cblist)],
-		".D"[!!rdp->tick_nohz_enabled_snap]);
+		!!rdp->tick_nohz_enabled_snap);
 }
 
 #else /* #ifdef CONFIG_RCU_FAST_NO_HZ */
-- 
2.23.0.187.g17f5b7556c-goog


^ permalink raw reply related

* [PATCH 3/5] rcu/tree: Add support for debug_objects debugging for kfree_rcu()
From: Joel Fernandes (Google) @ 2019-08-27 19:01 UTC (permalink / raw)
  To: linux-kernel
  Cc: Joel Fernandes (Google), byungchul.park, Josh Triplett,
	Lai Jiangshan, linux-doc, Mathieu Desnoyers, Paul E. McKenney,
	rcu, Steven Rostedt, kernel-team

Make use of RCU's debug_objects debugging support
(CONFIG_DEBUG_OBJECTS_RCU_HEAD) similar to call_rcu() and other flavors.
We queue the object during the kfree_rcu() call and dequeue it during
reclaim.

Tested that enabling CONFIG_DEBUG_OBJECTS_RCU_HEAD successfully detects
double kfree_rcu() calls.

Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 kernel/rcu/tree.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 9b9ae4db1c2d..64568f12641d 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -2757,6 +2757,7 @@ static void kfree_rcu_work(struct work_struct *work)
 	for (; head; head = next) {
 		next = head->next;
 		/* Could be possible to optimize with kfree_bulk in future */
+		debug_rcu_head_unqueue(head);
 		__rcu_reclaim(rcu_state.name, head);
 		cond_resched_tasks_rcu_qs();
 	}
@@ -2868,6 +2869,13 @@ void kfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
 	if (rcu_scheduler_active != RCU_SCHEDULER_RUNNING)
 		return kfree_call_rcu_nobatch(head, func);
 
+	if (debug_rcu_head_queue(head)) {
+		/* Probable double kfree_rcu() */
+		WARN_ONCE(1, "kfree_call_rcu(): Double-freed call. rcu_head %p\n",
+			  head);
+		return;
+	}
+
 	head->func = func;
 
 	local_irq_save(flags);	/* For safely calling this_cpu_ptr(). */
-- 
2.23.0.187.g17f5b7556c-goog


^ permalink raw reply related

* [PATCH 1/5] rcu/rcuperf: Add kfree_rcu() performance Tests
From: Joel Fernandes (Google) @ 2019-08-27 19:01 UTC (permalink / raw)
  To: linux-kernel
  Cc: Joel Fernandes (Google), byungchul.park, Josh Triplett,
	Lai Jiangshan, linux-doc, Mathieu Desnoyers, Paul E. McKenney,
	rcu, Steven Rostedt, kernel-team

This test runs kfree_rcu() in a loop to measure performance of the new
kfree_rcu() batching functionality.

The following table shows results when booting with arguments:
rcuperf.kfree_loops=20000 rcuperf.kfree_alloc_num=8000 rcuperf.kfree_rcu_test=1

In addition, rcuperf.kfree_no_batch is used to toggle the batching of
kfree_rcu()s for a test run.

patch applied		GPs	time (seconds)
 yes			1732	14.5
 no			9133 	11.5

On a 16 CPU system with the above boot parameters, we see that the total
number of grace periods that elapse during the test drops from 9133 when
not batching to 1732 when batching (a 5X improvement). The kfree_rcu()
flood itself slows down a bit when batching, though, as shown.

Note that the active memory consumption during the kfree_rcu() flood
does increase to around 200-250MB due to the batching (from around 50MB
without batching). However, this memory consumption is relatively
constant. In other words, the system is able to keep up with the
kfree_rcu() load. The memory consumption comes down considerably if
KFREE_DRAIN_JIFFIES is increased from HZ/50 to HZ/80.

Also, when running the test, please disable CONFIG_DEBUG_PREEMPT and
CONFIG_PROVE_RCU for realistic comparisons with/without batching.

Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 .../admin-guide/kernel-parameters.txt         |  17 ++
 kernel/rcu/rcuperf.c                          | 181 +++++++++++++++++-
 2 files changed, 190 insertions(+), 8 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 79b983bedcaa..24fe8aefb12c 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3896,6 +3896,23 @@
 			test until boot completes in order to avoid
 			interference.
 
+	rcuperf.kfree_rcu_test= [KNL]
+			Set to measure performance of kfree_rcu() flooding.
+
+	rcuperf.kfree_nthreads= [KNL]
+			The number of threads running loops of kfree_rcu().
+
+	rcuperf.kfree_alloc_num= [KNL]
+			Number of allocations and frees done in an iteration.
+
+	rcuperf.kfree_loops= [KNL]
+			Number of loops doing rcuperf.kfree_alloc_num number
+			of allocations and frees.
+
+	rcuperf.kfree_no_batch= [KNL]
+			Use the non-batching (less efficient) version of kfree_rcu().
+			This is useful for comparing with the batched version.
+
 	rcuperf.nreaders= [KNL]
 			Set number of RCU readers.  The value -1 selects
 			N, where N is the number of CPUs.  A value
diff --git a/kernel/rcu/rcuperf.c b/kernel/rcu/rcuperf.c
index 5f884d560384..c1e25fd10f2a 100644
--- a/kernel/rcu/rcuperf.c
+++ b/kernel/rcu/rcuperf.c
@@ -86,6 +86,7 @@ torture_param(bool, shutdown, RCUPERF_SHUTDOWN,
 	      "Shutdown at end of performance tests.");
 torture_param(int, verbose, 1, "Enable verbose debugging printk()s");
 torture_param(int, writer_holdoff, 0, "Holdoff (us) between GPs, zero to disable");
+torture_param(int, kfree_rcu_test, 0, "Do we run a kfree_rcu() perf test?");
 
 static char *perf_type = "rcu";
 module_param(perf_type, charp, 0444);
@@ -105,8 +106,8 @@ static atomic_t n_rcu_perf_writer_finished;
 static wait_queue_head_t shutdown_wq;
 static u64 t_rcu_perf_writer_started;
 static u64 t_rcu_perf_writer_finished;
-static unsigned long b_rcu_perf_writer_started;
-static unsigned long b_rcu_perf_writer_finished;
+static unsigned long b_rcu_gp_test_started;
+static unsigned long b_rcu_gp_test_finished;
 static DEFINE_PER_CPU(atomic_t, n_async_inflight);
 
 #define MAX_MEAS 10000
@@ -378,10 +379,10 @@ rcu_perf_writer(void *arg)
 	if (atomic_inc_return(&n_rcu_perf_writer_started) >= nrealwriters) {
 		t_rcu_perf_writer_started = t;
 		if (gp_exp) {
-			b_rcu_perf_writer_started =
+			b_rcu_gp_test_started =
 				cur_ops->exp_completed() / 2;
 		} else {
-			b_rcu_perf_writer_started = cur_ops->get_gp_seq();
+			b_rcu_gp_test_started = cur_ops->get_gp_seq();
 		}
 	}
 
@@ -429,10 +430,10 @@ rcu_perf_writer(void *arg)
 				PERFOUT_STRING("Test complete");
 				t_rcu_perf_writer_finished = t;
 				if (gp_exp) {
-					b_rcu_perf_writer_finished =
+					b_rcu_gp_test_finished =
 						cur_ops->exp_completed() / 2;
 				} else {
-					b_rcu_perf_writer_finished =
+					b_rcu_gp_test_finished =
 						cur_ops->get_gp_seq();
 				}
 				if (shutdown) {
@@ -515,8 +516,8 @@ rcu_perf_cleanup(void)
 			 t_rcu_perf_writer_finished -
 			 t_rcu_perf_writer_started,
 			 ngps,
-			 rcuperf_seq_diff(b_rcu_perf_writer_finished,
-					  b_rcu_perf_writer_started));
+			 rcuperf_seq_diff(b_rcu_gp_test_finished,
+					  b_rcu_gp_test_started));
 		for (i = 0; i < nrealwriters; i++) {
 			if (!writer_durations)
 				break;
@@ -584,6 +585,167 @@ rcu_perf_shutdown(void *arg)
 	return -EINVAL;
 }
 
+/*
+ * kfree_rcu() performance tests: Start a kfree_rcu() loop on all CPUs for number
+ * of iterations and measure total time and number of GP for all iterations to complete.
+ */
+
+torture_param(int, kfree_nthreads, -1, "Number of threads running loops of kfree_rcu().");
+torture_param(int, kfree_alloc_num, 8000, "Number of allocations and frees done in an iteration.");
+torture_param(int, kfree_loops, 10, "Number of loops doing kfree_alloc_num allocations and frees.");
+torture_param(int, kfree_no_batch, 0, "Use the non-batching (slower) version of kfree_rcu().");
+
+static struct task_struct **kfree_reader_tasks;
+static int kfree_nrealthreads;
+static atomic_t n_kfree_perf_thread_started;
+static atomic_t n_kfree_perf_thread_ended;
+
+struct kfree_obj {
+	char kfree_obj[8];
+	struct rcu_head rh;
+};
+
+static int
+kfree_perf_thread(void *arg)
+{
+	int i, loop = 0;
+	long me = (long)arg;
+	struct kfree_obj *alloc_ptr;
+	u64 start_time, end_time;
+
+	VERBOSE_PERFOUT_STRING("kfree_perf_thread task started");
+	set_cpus_allowed_ptr(current, cpumask_of(me % nr_cpu_ids));
+	set_user_nice(current, MAX_NICE);
+
+	start_time = ktime_get_mono_fast_ns();
+
+	if (atomic_inc_return(&n_kfree_perf_thread_started) >= kfree_nrealthreads) {
+		if (gp_exp)
+			b_rcu_gp_test_started = cur_ops->exp_completed() / 2;
+		else
+			b_rcu_gp_test_started = cur_ops->get_gp_seq();
+	}
+
+	do {
+		for (i = 0; i < kfree_alloc_num; i++) {
+			alloc_ptr = kmalloc(sizeof(struct kfree_obj), GFP_KERNEL);
+			if (!alloc_ptr)
+				return -ENOMEM;
+
+			if (!kfree_no_batch) {
+				kfree_rcu(alloc_ptr, rh);
+			} else {
+				rcu_callback_t cb;
+
+				cb = (rcu_callback_t)(unsigned long)offsetof(struct kfree_obj, rh);
+				kfree_call_rcu_nobatch(&(alloc_ptr->rh), cb);
+			}
+		}
+
+		cond_resched();
+	} while (!torture_must_stop() && ++loop < kfree_loops);
+
+	if (atomic_inc_return(&n_kfree_perf_thread_ended) >= kfree_nrealthreads) {
+		end_time = ktime_get_mono_fast_ns();
+
+		if (gp_exp)
+			b_rcu_gp_test_finished = cur_ops->exp_completed() / 2;
+		else
+			b_rcu_gp_test_finished = cur_ops->get_gp_seq();
+
+		pr_alert("Total time taken by all kfree'ers: %llu ns, loops: %d, batches: %ld\n",
+		       (unsigned long long)(end_time - start_time), kfree_loops,
+		       rcuperf_seq_diff(b_rcu_gp_test_finished, b_rcu_gp_test_started));
+		if (shutdown) {
+			smp_mb(); /* Assign before wake. */
+			wake_up(&shutdown_wq);
+		}
+	}
+
+	torture_kthread_stopping("kfree_perf_thread");
+	return 0;
+}
+
+static void
+kfree_perf_cleanup(void)
+{
+	int i;
+
+	if (torture_cleanup_begin())
+		return;
+
+	if (kfree_reader_tasks) {
+		for (i = 0; i < kfree_nrealthreads; i++)
+			torture_stop_kthread(kfree_perf_thread,
+					     kfree_reader_tasks[i]);
+		kfree(kfree_reader_tasks);
+	}
+
+	torture_cleanup_end();
+}
+
+/*
+ * shutdown kthread.  Just waits to be awakened, then shuts down system.
+ */
+static int
+kfree_perf_shutdown(void *arg)
+{
+	do {
+		wait_event(shutdown_wq,
+			   atomic_read(&n_kfree_perf_thread_ended) >=
+			   kfree_nrealthreads);
+	} while (atomic_read(&n_kfree_perf_thread_ended) < kfree_nrealthreads);
+
+	smp_mb(); /* Wake before output. */
+
+	kfree_perf_cleanup();
+	kernel_power_off();
+	return -EINVAL;
+}
+
+static int __init
+kfree_perf_init(void)
+{
+	long i;
+	int firsterr = 0;
+
+	kfree_nrealthreads = compute_real(kfree_nthreads);
+	/* Start up the kthreads. */
+	if (shutdown) {
+		init_waitqueue_head(&shutdown_wq);
+		firsterr = torture_create_kthread(kfree_perf_shutdown, NULL,
+						  shutdown_task);
+		if (firsterr)
+			goto unwind;
+		schedule_timeout_uninterruptible(1);
+	}
+
+	kfree_reader_tasks = kcalloc(kfree_nrealthreads, sizeof(kfree_reader_tasks[0]),
+			       GFP_KERNEL);
+	if (kfree_reader_tasks == NULL) {
+		firsterr = -ENOMEM;
+		goto unwind;
+	}
+
+	for (i = 0; i < kfree_nrealthreads; i++) {
+		firsterr = torture_create_kthread(kfree_perf_thread, (void *)i,
+						  kfree_reader_tasks[i]);
+		if (firsterr)
+			goto unwind;
+	}
+
+	while (atomic_read(&n_kfree_perf_thread_started) < kfree_nrealthreads)
+		schedule_timeout_uninterruptible(1);
+
+	torture_init_end();
+	return 0;
+
+unwind:
+	torture_init_end();
+	kfree_perf_cleanup();
+	return firsterr;
+}
+
 static int __init
 rcu_perf_init(void)
 {
@@ -616,6 +778,9 @@ rcu_perf_init(void)
 	if (cur_ops->init)
 		cur_ops->init();
 
+	if (kfree_rcu_test)
+		return kfree_perf_init();
+
 	nrealwriters = compute_real(nwriters);
 	nrealreaders = compute_real(nreaders);
 	atomic_set(&n_rcu_perf_reader_started, 0);
-- 
2.23.0.187.g17f5b7556c-goog


^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox