LinuxPPC-Dev Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH RESEND] tty: hvc_console: Remove the repeated words 'no' and 'from'
From: Xiaofei Tan @ 2021-05-21  2:52 UTC (permalink / raw)
  To: gregkh, jirislaby; +Cc: Xiaofei Tan, linuxppc-dev, linux-kernel, linuxarm

Remove the repeated words 'no' and 'from', reported by checkpatch.pl.

Signed-off-by: Xiaofei Tan <tanxiaofei@huawei.com>
---
 drivers/tty/hvc/hvc_console.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
index cdcc64e..bdd8d2b 100644
--- a/drivers/tty/hvc/hvc_console.c
+++ b/drivers/tty/hvc/hvc_console.c
@@ -292,7 +292,7 @@ int hvc_instantiate(uint32_t vtermno, int index, const struct hv_ops *ops)
 	if (vtermnos[index] != -1)
 		return -1;
 
-	/* make sure no no tty has been registered in this index */
+	/* make sure no tty has been registered in this index */
 	hp = hvc_get_by_index(index);
 	if (hp) {
 		tty_port_put(&hp->port);
@@ -620,7 +620,7 @@ static u32 timeout = MIN_TIMEOUT;
 /*
  * Maximum number of bytes to get from the console driver if hvc_poll is
  * called from driver (and can't sleep). Any more than this and we break
- * and start polling with khvcd. This value was derived from from an OpenBMC
+ * and start polling with khvcd. This value was derived from an OpenBMC
  * console with the OPAL driver that results in about 0.25ms interrupts off
  * latency.
  */
-- 
2.8.1


^ permalink raw reply related

* Re: [PATCH v5 7/9] mm/mremap: Move TLB flush outside page table lock
From: Linus Torvalds @ 2021-05-21  2:40 UTC (permalink / raw)
  To: Aneesh Kumar K.V
  Cc: Nick Piggin, Linux-MM, Kalesh Singh, Joel Fernandes,
	Andrew Morton, linuxppc-dev
In-Reply-To: <2eafd7df-65fd-1e2c-90b6-d143557a1fdc@linux.ibm.com>

On Thu, May 20, 2021 at 6:57 AM Aneesh Kumar K.V
<aneesh.kumar@linux.ibm.com> wrote:
>
> Wondering whether this is correct considering we are holding mmap_sem in
> write mode in mremap.

Right. So *normally* the rule is to EITHER

 - hold the mmap_sem for writing

OR

 - hold the page table lock

and that the TLB flush needs to happen before you release that lock.

But as that commit message of commit eb66ae030829 ("mremap: properly
flush TLB before releasing the page") says, "mremap()" is a bit
special. It's special because mremap() didn't take ownership of the
page - it only moved it somewhere else. So now the page-out logic -
that relies on the page table lock - can free the page immediately
after we've released the page table lock.

So basically, in order to delay the TLB flush after releasing the page
table lock, it's not really sufficient to _just_ hold the mmap_sem for
writing. You also need to guarantee that the lifetime of the page
itself is held until after the TLB flush.

For normal operations like "munmap()", this happens naturally, because
we remove the page from the page table, and add it to the list of
pages to be freed after the TLB flush.

But mremap never did that "remove the page and add it to a list to be
free'd later". Instead, it just moved the page somewhere else. And
thus there is no guarantee that the page that got moved will continue
to exist until a TLB flush is done.

So mremap does need to flush the TLB before releasing the page table
lock, because that's the lifetime boundary for the page that got
moved.

                  Linus

^ permalink raw reply

* Re: [PATCH 1/3] sched/topology: Allow archs to populate distance map
From: Srikar Dronamraju @ 2021-05-21  2:38 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Nathan Lynch, Gautham R Shenoy, Vincent Guittot, Rik van Riel,
	linuxppc-dev, Scott Cheloha, Geetika Moolchandani, LKML,
	Ingo Molnar, Thomas Gleixner, Mel Gorman, Valentin Schneider,
	Dietmar Eggemann
In-Reply-To: <YKaw33d71FpHjGnR@hirez.programming.kicks-ass.net>

* Peter Zijlstra <peterz@infradead.org> [2021-05-20 20:56:31]:

> On Thu, May 20, 2021 at 09:14:25PM +0530, Srikar Dronamraju wrote:
> > Currently scheduler populates the distance map by looking at distance
> > of each node from all other nodes. This should work for most
> > architectures and platforms.
> > 
> > However there are some architectures like POWER that may not expose
> > the distance of nodes that are not yet onlined because those resources
> > are not yet allocated to the OS instance. Such architectures have
> > other means to provide valid distance data for the current platform.
> > 
> > For example distance info from numactl from a fully populated 8 node
> > system at boot may look like this.
> > 
> > node distances:
> > node   0   1   2   3   4   5   6   7
> >   0:  10  20  40  40  40  40  40  40
> >   1:  20  10  40  40  40  40  40  40
> >   2:  40  40  10  20  40  40  40  40
> >   3:  40  40  20  10  40  40  40  40
> >   4:  40  40  40  40  10  20  40  40
> >   5:  40  40  40  40  20  10  40  40
> >   6:  40  40  40  40  40  40  10  20
> >   7:  40  40  40  40  40  40  20  10
> > 
> > However the same system when only two nodes are online at boot, then the
> > numa topology will look like
> > node distances:
> > node   0   1
> >   0:  10  20
> >   1:  20  10
> > 
> > It may be implementation dependent on what node_distance(0,3) where
> > node 0 is online and node 3 is offline. In POWER case, it returns
> > LOCAL_DISTANCE(10). Here at boot the scheduler would assume that the max
> > distance between nodes is 20. However that would not be true.
> > 
> > When Nodes are onlined and CPUs from those nodes are hotplugged,
> > the max node distance would be 40.
> > 
> > To handle such scenarios, let scheduler allow architectures to populate
> > the distance map. Architectures that like to populate the distance map
> > can overload arch_populate_distance_map().
> 
> Why? Why can't your node_distance() DTRT? The arch interface is
> nr_node_ids and node_distance(), I don't see why we need something new
> and then replace one special use of it.
> 
> By virtue of you being able to actually implement this new hook, you
> supposedly can actually do node_distance() right too.

Since for an offline node, arch interface code doesn't have the info.
As far as I know/understand, in POWER, unless there is an active memory or
CPU that's getting onlined, arch can't fetch the correct node distance.

Taking the above example: node 3 is offline, then node_distance of (3,X)
where X is anything other than 3, is not reliable. The moment node 3 is
onlined, the node distance is reliable.

This problem will not happen even on POWER if all the nodes have either
memory or CPUs active at the time of boot.

-- 
Thanks and Regards
Srikar Dronamraju

^ permalink raw reply

* Re: [PATCH 6/9] tty: hvc_console: Fix coding style issues of block comments
From: Xiaofei Tan @ 2021-05-21  2:23 UTC (permalink / raw)
  To: Johan Hovold; +Cc: gregkh, linuxppc-dev, jirislaby, linux-kernel, linuxarm
In-Reply-To: <YKZpi8cmH3mtXT99@hovoldconsulting.com>


Hi Johan,

On 2021/5/20 21:52, Johan Hovold wrote:
> On Thu, May 20, 2021 at 09:21:25PM +0800, Xiaofei Tan wrote:
>
>>> Checkpatch already has too many checks IMO and I'm a bit surprised that
>>> it doesn't check this already. Perhaps it's because you used the -f to
>>> run checkpatch on in-kernel code, which you should not.
>>>
>>>>> Second, that sentence is not capitalised so why do add a period?
>>>>>
>>>>
>>>> How about capitalize the sentence, or just remove the period ?
>>>
>>> How about just leaving this unchanged?
>>
>> OK
>> And I will keep the patch 8/9, and combine space issues into
>> one new patch, and remove the others.
>
> Yeah, 8/9 is arguably a fix even if it's for a very minor issue
> (repeated words in a comment).
>
> It doesn't look like any of the white space issues are worth fixing,
> though. Such pedantry can usually be addressed when the code in question
> is being modified for other reasons.
>

OK, it is reasonable. thanks.

> Johan
>
> .
>


^ permalink raw reply

* Re: [PATCH 0/9] tty: hvc_console: Fix some coding style issues
From: Xiaofei Tan @ 2021-05-21  2:18 UTC (permalink / raw)
  To: Greg KH; +Cc: linuxppc-dev, jirislaby, linux-kernel, linuxarm
In-Reply-To: <YKZ8M8j5/bUJxLq5@kroah.com>

Hi Greg,

On 2021/5/20 23:11, Greg KH wrote:
> On Mon, May 17, 2021 at 02:37:04PM +0800, Xiaofei Tan wrote:
>> Fix some issues reported by checkpatch.pl. All of them are
>> coding style issues, no function changes.
>>
>> Xiaofei Tan (9):
>>   tty: hvc_console: Fix spaces required around that '='
>>   tty: hvc_console: Fix "foo * bar" should be "foo *bar"
>>   tty: hvc_console: Remove trailing whitespace
>>   tty: hvc_console: Fix issues of code indent should use tabs
>>   tty: hvc_console: Delete spaces prohibited around open parenthesis '('
>>     and ')'
>>   tty: hvc_console: Fix coding style issues of block comments
>>   tty: hvc_console: Add a blank line after declarations
>>   tty: hvc_console: Remove the repeated words 'no' and 'from'
>>   tty: hvc_console: Move open brace { on the previous line
>>
>>  drivers/tty/hvc/hvc_console.c | 37 ++++++++++++++++++++++---------------
>>  1 file changed, 22 insertions(+), 15 deletions(-)
>
> Do you use this driver?

No, i don't use it.

  If so, great, I'm sure there are other "real"
> issues in it that need some work.  But as Johan points out, doing
> drive-by checkpatch cleanups on random files that you do not use, isn't
> the best thing to do.
>

Sure

> If you just want to do this type of work, please do so in
> drivers/staging/ as it is most welcome there if you wish to get involved
> in kernel work to get experience before doing "real" stuff.
>

OK, thanks.

> thanks,
> g
> reg k-h
>
> .
>


^ permalink raw reply

* Re: [PATCH v2 2/4] dt-bindings: nintendo-otp: Document the Wii and Wii U OTP support
From: Rob Herring @ 2021-05-21  1:37 UTC (permalink / raw)
  To: Emmanuel Gil Peyrot
  Cc: devicetree, linux-kernel, Srinivas Kandagatla, Ash Logan,
	Paul Mackerras, linuxppc-dev, Jonathan Neuschäfer
In-Reply-To: <20210519095044.4109-3-linkmauve@linkmauve.fr>

On Wed, May 19, 2021 at 11:50:42AM +0200, Emmanuel Gil Peyrot wrote:
> Both of these consoles use the exact same two registers, even at the
> same address, but the Wii U has eight banks of 128 bytes memory while
> the Wii only has one, hence the two compatible strings.
> 
> Signed-off-by: Emmanuel Gil Peyrot <linkmauve@linkmauve.fr>
> ---
>  .../devicetree/bindings/nvmem/nintendo-otp.txt     | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/nvmem/nintendo-otp.txt

Bindings should be in DT schema format now.

> 
> diff --git a/Documentation/devicetree/bindings/nvmem/nintendo-otp.txt b/Documentation/devicetree/bindings/nvmem/nintendo-otp.txt
> new file mode 100644
> index 000000000000..b26d705ec52d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/nvmem/nintendo-otp.txt
> @@ -0,0 +1,14 @@
> +Nintendo Wii and Wii U OTP
> +
> +Required Properties:
> +- compatible: depending on the console this should be one of:
> +	- "nintendo,hollywood-otp" for the Wii
> +	- "nintendo,latte-otp" for the Wii U
> +- reg: base address and size of the OTP registers
> +
> +
> +Example:
> +	otp@d8001ec {
> +		compatible = "nintendo,latte-otp";
> +		reg = <0x0d8001ec 0x8>;
> +	};
> -- 
> 2.31.1
> 

^ permalink raw reply

* Re: [PATCH v2 net-next resend] ibmvnic: remove default label from to_string switch
From: patchwork-bot+netdevbpf @ 2021-05-20 22:50 UTC (permalink / raw)
  To: Michal Suchanek
  Cc: lijunp213, netdev, linux-kernel, tlfalcon, paulus, drt, kuba,
	sukadev, linuxppc-dev, davem
In-Reply-To: <20210520065034.5912-1-msuchanek@suse.de>

Hello:

This patch was applied to netdev/net-next.git (refs/heads/master):

On Thu, 20 May 2021 08:50:34 +0200 you wrote:
> This way the compiler warns when a new value is added to the enum but
> not to the string translation like:
> 
> drivers/net/ethernet/ibm/ibmvnic.c: In function 'adapter_state_to_string':
> drivers/net/ethernet/ibm/ibmvnic.c:832:2: warning: enumeration value 'VNIC_FOOBAR' not handled in switch [-Wswitch]
>   switch (state) {
>   ^~~~~~
> drivers/net/ethernet/ibm/ibmvnic.c: In function 'reset_reason_to_string':
> drivers/net/ethernet/ibm/ibmvnic.c:1935:2: warning: enumeration value 'VNIC_RESET_FOOBAR' not handled in switch [-Wswitch]
>   switch (reason) {
>   ^~~~~~
> 
> [...]

Here is the summary with links:
  - [v2,net-next,resend] ibmvnic: remove default label from to_string switch
    https://git.kernel.org/netdev/net-next/c/07b5dc1d515a

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply

* Re: [PATCH v3 1/2] ASoC: dt-bindings: imx-card: Add binding doc for imx sound card
From: Mark Brown @ 2021-05-20 21:08 UTC (permalink / raw)
  To: linux-imx, perex, linux-arm-kernel, nicoleotsuka, festevam,
	shawnguo, Xiubo.Lee, alsa-devel, timur, s.hauer, Shengjiu Wang,
	robh+dt, kernel, linuxppc-dev, linux-kernel, devicetree,
	lgirdwood, tiwai
  Cc: Mark Brown
In-Reply-To: <1621247488-21412-1-git-send-email-shengjiu.wang@nxp.com>

On Mon, 17 May 2021 18:31:27 +0800, Shengjiu Wang wrote:
> Imx-card is a new added machine driver for supporting
> ak4458/ak5558/ak5552/ak4497 codec on i.MX platforms. But these
> DAC/ADCs are not only supported codecs. This machine driver is
> designed to be a more common machine driver for i.MX platform,
> it can support widely cpu dai interface and codec dai interface.

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/2] ASoC: dt-bindings: imx-card: Add binding doc for imx sound card
      commit: 623cd9cfcac522647e3624e48bf0661a39e8502a
[2/2] ASoC: imx-card: Add imx-card machine driver
      commit: aa736700f42fa0813e286ca2f9274ffaa25163b9

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

^ permalink raw reply

* Re: [PATCH v5 3/9] mm/mremap: Use pmd/pud_poplulate to update page table entries
From: Kalesh Singh @ 2021-05-20 20:25 UTC (permalink / raw)
  To: Peter Xu
  Cc: Aneesh Kumar K.V, npiggin, Nathan Chancellor,
	open list:MEMORY MANAGEMENT, Zi Yan, joel, Andrew Morton,
	linuxppc-dev
In-Reply-To: <YKbABNL07RIN0qFN@t490s>

On Thu, May 20, 2021 at 4:01 PM Peter Xu <peterx@redhat.com> wrote:
>
> On Thu, May 20, 2021 at 03:06:30PM -0400, Zi Yan wrote:
> > On 20 May 2021, at 10:57, Peter Xu wrote:
> >
> > > On Thu, May 20, 2021 at 07:07:57PM +0530, Aneesh Kumar K.V wrote:
> > >> "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
> > >>
> > >>> On 5/20/21 6:16 PM, Peter Xu wrote:
> > >>>> On Thu, May 20, 2021 at 01:56:54PM +0530, Aneesh Kumar K.V wrote:
> > >>>>>> This seems to work at least for my userfaultfd test on shmem, however I don't
> > >>>>>> fully understand the commit message [1] on: How do we guarantee we're not
> > >>>>>> moving a thp pte?
> > >>>>>>
> > >>>>>
> > >>>>> move_page_tables() checks for pmd_trans_huge() and ends up calling
> > >>>>> move_huge_pmd if it is a THP entry.
> > >>>>
> > >>>> Sorry to be unclear: what if a huge pud thp?
> > >>>>
> > >>>
> > >>> I am still checking. Looking at the code before commit
> > >>> c49dd340180260c6239e453263a9a244da9a7c85, I don't see kernel handling
> > >>> huge pud thp. I haven't studied huge pud thp enough to understand
> > >>> whether c49dd340180260c6239e453263a9a244da9a7c85 intent to add that
> > >>> support.
> > >>>
> > >>> We can do a move_huge_pud() like we do for huge pmd thp. But I am not
> > >>> sure whether we handle those VMA's earlier and restrict mremap on them?
> > >>
> > >> something like this? (not even compile tested). I am still not sure
> > >> whether this is really needed or we handle DAX VMA's in some other form.
> > >
> > > Yeah maybe (you may want to at least drop that extra "case HPAGE_PUD").
> > >
> > > It's just that if with CONFIG_HAVE_MOVE_PUD (x86 and arm64 enables it by
> > > default so far) it does seem to work even with huge pud, while after this patch
> > > it seems to be not working anymore, even with your follow up fix.
> > >
> > > Indeed I saw CONFIG_HAVE_MOVE_PUD is introduced a few months ago so breaking
> > > someone seems to be unlikely, perhaps no real user yet to mremap() a huge pud
> > > for dax or whatever backend?
> > >
> > > Ideally maybe rework this patch (or series?) and repost it for a better review?
> > > Agree the risk seems low.  I'll leave that to you and Andrew to decide..
> >
> > It seems that the mremap function for 1GB DAX THP was not added when 1GB DAX THP
> > was implemented[1].
>
> Yes, but trickily as I mentioned it seems Android's CONFIG_HAVE_MOVE_PUD has
> done this right (with no intention I guess) with the set_pud_at() before this
> patch is merged, so we might have a short period that this might start to work..
>
It may have coincidentally handled the huge PUD case, but I hadn't
considered huge PUDs when implementing the HAVE_MOVE_PUD patchset.  Or
as Zi suggested, huge PUD mremap may be unused atm, I haven't seen any
related breakages since enabling HAVE_MOVE_PUD for x86 and arm64

> > I guess no one is using mremap on 1GB DAX THP. Maybe we want
> > to at least add a warning or VM_BUG_ON to catch this or use Aneesh’s move_huge_pud()
> > to handle the situation properly?
>
> Agreed, if we decide to go with the patches, some warning (or even VM_BUG_ON,
> which iiuc should be very not-suggested in most cases) looks better than
> pgtable corruption reports.
>
> --
> Peter Xu
>

^ permalink raw reply

* Re: [PATCH v5 3/9] mm/mremap: Use pmd/pud_poplulate to update page table entries
From: Peter Xu @ 2021-05-20 20:01 UTC (permalink / raw)
  To: Zi Yan
  Cc: Aneesh Kumar K.V, kaleshsingh, npiggin, Nathan Chancellor,
	linux-mm, joel, akpm, linuxppc-dev
In-Reply-To: <4CE7132C-3800-456B-91DA-613391361B94@nvidia.com>

On Thu, May 20, 2021 at 03:06:30PM -0400, Zi Yan wrote:
> On 20 May 2021, at 10:57, Peter Xu wrote:
> 
> > On Thu, May 20, 2021 at 07:07:57PM +0530, Aneesh Kumar K.V wrote:
> >> "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
> >>
> >>> On 5/20/21 6:16 PM, Peter Xu wrote:
> >>>> On Thu, May 20, 2021 at 01:56:54PM +0530, Aneesh Kumar K.V wrote:
> >>>>>> This seems to work at least for my userfaultfd test on shmem, however I don't
> >>>>>> fully understand the commit message [1] on: How do we guarantee we're not
> >>>>>> moving a thp pte?
> >>>>>>
> >>>>>
> >>>>> move_page_tables() checks for pmd_trans_huge() and ends up calling
> >>>>> move_huge_pmd if it is a THP entry.
> >>>>
> >>>> Sorry to be unclear: what if a huge pud thp?
> >>>>
> >>>
> >>> I am still checking. Looking at the code before commit
> >>> c49dd340180260c6239e453263a9a244da9a7c85, I don't see kernel handling
> >>> huge pud thp. I haven't studied huge pud thp enough to understand
> >>> whether c49dd340180260c6239e453263a9a244da9a7c85 intent to add that
> >>> support.
> >>>
> >>> We can do a move_huge_pud() like we do for huge pmd thp. But I am not
> >>> sure whether we handle those VMA's earlier and restrict mremap on them?
> >>
> >> something like this? (not even compile tested). I am still not sure
> >> whether this is really needed or we handle DAX VMA's in some other form.
> >
> > Yeah maybe (you may want to at least drop that extra "case HPAGE_PUD").
> >
> > It's just that if with CONFIG_HAVE_MOVE_PUD (x86 and arm64 enables it by
> > default so far) it does seem to work even with huge pud, while after this patch
> > it seems to be not working anymore, even with your follow up fix.
> >
> > Indeed I saw CONFIG_HAVE_MOVE_PUD is introduced a few months ago so breaking
> > someone seems to be unlikely, perhaps no real user yet to mremap() a huge pud
> > for dax or whatever backend?
> >
> > Ideally maybe rework this patch (or series?) and repost it for a better review?
> > Agree the risk seems low.  I'll leave that to you and Andrew to decide..
> 
> It seems that the mremap function for 1GB DAX THP was not added when 1GB DAX THP
> was implemented[1].

Yes, but trickily as I mentioned it seems Android's CONFIG_HAVE_MOVE_PUD has
done this right (with no intention I guess) with the set_pud_at() before this
patch is merged, so we might have a short period that this might start to work..

> I guess no one is using mremap on 1GB DAX THP. Maybe we want
> to at least add a warning or VM_BUG_ON to catch this or use Aneesh’s move_huge_pud()
> to handle the situation properly?

Agreed, if we decide to go with the patches, some warning (or even VM_BUG_ON,
which iiuc should be very not-suggested in most cases) looks better than
pgtable corruption reports.

-- 
Peter Xu


^ permalink raw reply

* Re: [PATCH v5 3/9] mm/mremap: Use pmd/pud_poplulate to update page table entries
From: Zi Yan @ 2021-05-20 19:06 UTC (permalink / raw)
  To: Peter Xu
  Cc: Aneesh Kumar K.V, kaleshsingh, npiggin, Nathan Chancellor,
	linux-mm, joel, akpm, linuxppc-dev
In-Reply-To: <YKZ49Nrz2OojQUBR@t490s>

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

On 20 May 2021, at 10:57, Peter Xu wrote:

> On Thu, May 20, 2021 at 07:07:57PM +0530, Aneesh Kumar K.V wrote:
>> "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
>>
>>> On 5/20/21 6:16 PM, Peter Xu wrote:
>>>> On Thu, May 20, 2021 at 01:56:54PM +0530, Aneesh Kumar K.V wrote:
>>>>>> This seems to work at least for my userfaultfd test on shmem, however I don't
>>>>>> fully understand the commit message [1] on: How do we guarantee we're not
>>>>>> moving a thp pte?
>>>>>>
>>>>>
>>>>> move_page_tables() checks for pmd_trans_huge() and ends up calling
>>>>> move_huge_pmd if it is a THP entry.
>>>>
>>>> Sorry to be unclear: what if a huge pud thp?
>>>>
>>>
>>> I am still checking. Looking at the code before commit
>>> c49dd340180260c6239e453263a9a244da9a7c85, I don't see kernel handling
>>> huge pud thp. I haven't studied huge pud thp enough to understand
>>> whether c49dd340180260c6239e453263a9a244da9a7c85 intent to add that
>>> support.
>>>
>>> We can do a move_huge_pud() like we do for huge pmd thp. But I am not
>>> sure whether we handle those VMA's earlier and restrict mremap on them?
>>
>> something like this? (not even compile tested). I am still not sure
>> whether this is really needed or we handle DAX VMA's in some other form.
>
> Yeah maybe (you may want to at least drop that extra "case HPAGE_PUD").
>
> It's just that if with CONFIG_HAVE_MOVE_PUD (x86 and arm64 enables it by
> default so far) it does seem to work even with huge pud, while after this patch
> it seems to be not working anymore, even with your follow up fix.
>
> Indeed I saw CONFIG_HAVE_MOVE_PUD is introduced a few months ago so breaking
> someone seems to be unlikely, perhaps no real user yet to mremap() a huge pud
> for dax or whatever backend?
>
> Ideally maybe rework this patch (or series?) and repost it for a better review?
> Agree the risk seems low.  I'll leave that to you and Andrew to decide..

It seems that the mremap function for 1GB DAX THP was not added when 1GB DAX THP
was implemented[1]. I guess no one is using mremap on 1GB DAX THP. Maybe we want
to at least add a warning or VM_BUG_ON to catch this or use Aneesh’s move_huge_pud()
to handle the situation properly?

[1] https://lore.kernel.org/linux-ext4/148545012634.17912.13951763606410303827.stgit@djiang5-desk3.ch.intel.com/


—
Best Regards,
Yan, Zi

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

^ permalink raw reply

* Re: [PATCH 1/3] sched/topology: Allow archs to populate distance map
From: Peter Zijlstra @ 2021-05-20 18:56 UTC (permalink / raw)
  To: Srikar Dronamraju
  Cc: Nathan Lynch, Gautham R Shenoy, Vincent Guittot, Rik van Riel,
	linuxppc-dev, Scott Cheloha, Geetika Moolchandani, LKML,
	Ingo Molnar, Thomas Gleixner, Mel Gorman, Valentin Schneider,
	Dietmar Eggemann
In-Reply-To: <20210520154427.1041031-2-srikar@linux.vnet.ibm.com>

On Thu, May 20, 2021 at 09:14:25PM +0530, Srikar Dronamraju wrote:
> Currently scheduler populates the distance map by looking at distance
> of each node from all other nodes. This should work for most
> architectures and platforms.
> 
> However there are some architectures like POWER that may not expose
> the distance of nodes that are not yet onlined because those resources
> are not yet allocated to the OS instance. Such architectures have
> other means to provide valid distance data for the current platform.
> 
> For example distance info from numactl from a fully populated 8 node
> system at boot may look like this.
> 
> node distances:
> node   0   1   2   3   4   5   6   7
>   0:  10  20  40  40  40  40  40  40
>   1:  20  10  40  40  40  40  40  40
>   2:  40  40  10  20  40  40  40  40
>   3:  40  40  20  10  40  40  40  40
>   4:  40  40  40  40  10  20  40  40
>   5:  40  40  40  40  20  10  40  40
>   6:  40  40  40  40  40  40  10  20
>   7:  40  40  40  40  40  40  20  10
> 
> However the same system when only two nodes are online at boot, then the
> numa topology will look like
> node distances:
> node   0   1
>   0:  10  20
>   1:  20  10
> 
> It may be implementation dependent on what node_distance(0,3) where
> node 0 is online and node 3 is offline. In POWER case, it returns
> LOCAL_DISTANCE(10). Here at boot the scheduler would assume that the max
> distance between nodes is 20. However that would not be true.
> 
> When Nodes are onlined and CPUs from those nodes are hotplugged,
> the max node distance would be 40.
> 
> To handle such scenarios, let scheduler allow architectures to populate
> the distance map. Architectures that like to populate the distance map
> can overload arch_populate_distance_map().

Why? Why can't your node_distance() DTRT? The arch interface is
nr_node_ids and node_distance(), I don't see why we need something new
and then replace one special use of it.

By virtue of you being able to actually implement this new hook, you
supposedly can actually do node_distance() right too.

^ permalink raw reply

* Re: [PATCH 07/31] powerpc/xive: Fix xive_irq_set_affinity for MSI
From: Cédric Le Goater @ 2021-05-20 17:25 UTC (permalink / raw)
  To: Thomas Gleixner, linuxppc-dev
In-Reply-To: <87pmxt2g9o.ffs@nanos.tec.linutronix.de>

On 5/14/21 10:48 PM, Thomas Gleixner wrote:
> On Fri, Apr 30 2021 at 10:03, Cédric Le Goater wrote:
>> The MSI affinity is automanaged and it can be set before starting the
>> associated IRQ.
>>
>> ( Should we simply remove the irqd_is_started() test ? )
> 
> If the hardware can handle it properly.
> 
> But see:
> 
>   cffb717ceb8e ("powerpc/xive: Ensure active irqd when setting affinity")

Thanks for digging. That's a patch from the early days of XIVE support. 

> which introduced that condition. It mutters something about migration of
> shutdown interrupts:
> 
>        [  123.053037264,3] XIVE[ IC 00  ] ISN 2 lead to invalid IVE !

The XIVE driver in OPAL is complaining.

Linux is trying to configure the target of HW IRQ number 2 but OPAL refuses
because it's invalid. The first 16 are reserved (like on Linux).

So it's another problem. 2 could be a value from an "interrupts" property,
giving the INTx number assigned to a PCI device or an OPAL event IRQ 
number leaked into the XIVE domain. Given the low Linux IRQ number that 
might be the latter. 

>        [   77.885859] xive: Error -6 reconfiguring irq 17
>        [   77.885862] IRQ17: set affinity failed(-6).
> 
> Not that I can decode that :)

A device name would help but you have guessed most of it ;)

> 
> Non-managed interrupts have the sequence:
> 
>       startup()
>       set_affinity()
> 
> which is historical and an earlier attempt to flip it caused havoc in
> some places.
> 
> With managed we needed to make sure that the affinity is set correctly
> right at start. So it needs to be done the other way round and it turned
> out that for MSI this works.
> 
> I have no idea, whether that might make the above issue reappear or
> not. If so, then we need some extra state to make it work.
> 
> The root cause which triggered the problem got fixed, so there should be
> no issue _if_ this was specifically related to that CPU unplug case.

I would vote for this option. I will simply remove the irqd_is_started() 
test which looks bogus and do some extra tests on all platforms.

Thanks,

C.


 
>> diff --git a/arch/powerpc/sysdev/xive/common.c b/arch/powerpc/sysdev/xive/common.c
>> index 96737938e8e3..3485baf9ec8c 100644
>> --- a/arch/powerpc/sysdev/xive/common.c
>> +++ b/arch/powerpc/sysdev/xive/common.c
>> @@ -710,7 +710,7 @@ static int xive_irq_set_affinity(struct irq_data *d,
>>  		return -EINVAL;
>>  
>>  	/* Don't do anything if the interrupt isn't started */
>> -	if (!irqd_is_started(d))
>> +	if (!irqd_is_started(d) && !irqd_affinity_is_managed(d))
>>  		return IRQ_SET_MASK_OK;
>>  
>>  	/*
> 
> Thanks,
> 
>         tglx
> 


^ permalink raw reply

* Re: [PATCH v5 7/9] mm/mremap: Move TLB flush outside page table lock
From: Aneesh Kumar K.V @ 2021-05-20 16:57 UTC (permalink / raw)
  To: linux-mm, akpm, Linus Torvalds; +Cc: npiggin, kaleshsingh, joel, linuxppc-dev
In-Reply-To: <b3047082-fc82-b326-dbdb-835b88df78d0@linux.ibm.com>

On 5/20/21 8:56 PM, Aneesh Kumar K.V wrote:
> On 4/22/21 11:13 AM, Aneesh Kumar K.V wrote:
>> Move TLB flush outside page table lock so that kernel does
>> less with page table lock held. Releasing the ptl with old
>> TLB contents still valid will behave such that such access
>> happened before the level3 or level2 entry update.
>>
> 
> 
> Ok this break the page lifetime rule
> 
> commit: eb66ae030829 ("mremap: properly flush TLB before releasing the 
> page")
> 
> I will respin dropping this change and add a comment around explaining 
> why we need to do tlb flush before dropping ptl.

Wondering whether this is correct considering we are holding mmap_sem in 
write mode in mremap. Can a parallel free/zap happen?

> 
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>> ---
>>   mm/mremap.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/mm/mremap.c b/mm/mremap.c
>> index 109560977944..9effca76bf17 100644
>> --- a/mm/mremap.c
>> +++ b/mm/mremap.c
>> @@ -258,7 +258,7 @@ static bool move_normal_pmd(struct vm_area_struct 
>> *vma, unsigned long old_addr,
>>        * We don't have to worry about the ordering of src and dst
>>        * ptlocks because exclusive mmap_lock prevents deadlock.
>>        */
>> -    old_ptl = pmd_lock(vma->vm_mm, old_pmd);
>> +    old_ptl = pmd_lock(mm, old_pmd);
>>       new_ptl = pmd_lockptr(mm, new_pmd);
>>       if (new_ptl != old_ptl)
>>           spin_lock_nested(new_ptl, SINGLE_DEPTH_NESTING);
>> @@ -270,11 +270,11 @@ static bool move_normal_pmd(struct 
>> vm_area_struct *vma, unsigned long old_addr,
>>       VM_BUG_ON(!pmd_none(*new_pmd));
>>       pmd_populate(mm, new_pmd, (pgtable_t)pmd_page_vaddr(pmd));
>> -    flush_pte_tlb_pwc_range(vma, old_addr, old_addr + PMD_SIZE);
>>       if (new_ptl != old_ptl)
>>           spin_unlock(new_ptl);
>>       spin_unlock(old_ptl);
>> +    flush_pte_tlb_pwc_range(vma, old_addr, old_addr + PMD_SIZE);
>>       return true;
>>   }
>>   #else
>> @@ -305,7 +305,7 @@ static bool move_normal_pud(struct vm_area_struct 
>> *vma, unsigned long old_addr,
>>        * We don't have to worry about the ordering of src and dst
>>        * ptlocks because exclusive mmap_lock prevents deadlock.
>>        */
>> -    old_ptl = pud_lock(vma->vm_mm, old_pud);
>> +    old_ptl = pud_lock(mm, old_pud);
>>       new_ptl = pud_lockptr(mm, new_pud);
>>       if (new_ptl != old_ptl)
>>           spin_lock_nested(new_ptl, SINGLE_DEPTH_NESTING);
>> @@ -317,11 +317,11 @@ static bool move_normal_pud(struct 
>> vm_area_struct *vma, unsigned long old_addr,
>>       VM_BUG_ON(!pud_none(*new_pud));
>>       pud_populate(mm, new_pud, (pmd_t *)pud_page_vaddr(pud));
>> -    flush_pte_tlb_pwc_range(vma, old_addr, old_addr + PUD_SIZE);
>>       if (new_ptl != old_ptl)
>>           spin_unlock(new_ptl);
>>       spin_unlock(old_ptl);
>> +    flush_pte_tlb_pwc_range(vma, old_addr, old_addr + PUD_SIZE);
>>       return true;
>>   }
>>   #else
>>
> 


^ permalink raw reply

* [PATCH 3/3] sched/topology: Skip updating masks for non-online nodes
From: Srikar Dronamraju @ 2021-05-20 15:44 UTC (permalink / raw)
  To: Ingo Molnar, Peter Zijlstra
  Cc: Nathan Lynch, Gautham R Shenoy, Vincent Guittot,
	Srikar Dronamraju, Rik van Riel, linuxppc-dev, Scott Cheloha,
	Geetika Moolchandani, LKML, Dietmar Eggemann, Thomas Gleixner,
	Mel Gorman, Valentin Schneider
In-Reply-To: <20210520154427.1041031-1-srikar@linux.vnet.ibm.com>

Currently scheduler doesn't check if node is online before adding CPUs
to the node mask. However on some architectures, node distance is only
available for nodes that are online. Its not sure how much to rely on
the node distance, when one of the nodes is offline.

If said node distance is fake (since one of the nodes is offline) and
the actual node distance is different, then the cpumask of such nodes
when the nodes become becomes online will be wrong.

This can cause topology_span_sane to throw up a warning message and the
rest of the topology being not updated properly.

Resolve this by skipping update of cpumask for nodes that are not
online.

Cc: LKML <linux-kernel@vger.kernel.org>
Cc: linuxppc-dev@lists.ozlabs.org
Cc: Nathan Lynch <nathanl@linux.ibm.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Valentin Schneider <valentin.schneider@arm.com>
Cc: Scott Cheloha <cheloha@linux.ibm.com>
Cc: Gautham R Shenoy <ego@linux.vnet.ibm.com>
Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
Cc: Mel Gorman <mgorman@techsingularity.net>
Cc: Vincent Guittot <vincent.guittot@linaro.org>
Cc: Rik van Riel <riel@surriel.com>
Cc: Geetika Moolchandani <Geetika.Moolchandani1@ibm.com>
Reported-by: Geetika Moolchandani <Geetika.Moolchandani1@ibm.com>
Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
---
 kernel/sched/topology.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
index ccb9aff59add..ba0555e83548 100644
--- a/kernel/sched/topology.c
+++ b/kernel/sched/topology.c
@@ -1731,6 +1731,9 @@ void sched_init_numa(void)
 			sched_domains_numa_masks[i][j] = mask;
 
 			for_each_node(k) {
+				if (!node_online(j))
+					continue;
+
 				if (sched_debug() && (node_distance(j, k) != node_distance(k, j)))
 					sched_numa_warn("Node-distance not symmetric");
 
@@ -1793,6 +1796,9 @@ void sched_domains_numa_masks_set(unsigned int cpu)
 
 	for (i = 0; i < sched_domains_numa_levels; i++) {
 		for (j = 0; j < nr_node_ids; j++) {
+			if (!node_online(j))
+				continue;
+
 			if (node_distance(j, node) <= sched_domains_numa_distance[i])
 				cpumask_set_cpu(cpu, sched_domains_numa_masks[i][j]);
 		}
-- 
2.27.0


^ permalink raw reply related

* [PATCH 2/3] powerpc/numa: Populate distance map correctly
From: Srikar Dronamraju @ 2021-05-20 15:44 UTC (permalink / raw)
  To: Ingo Molnar, Peter Zijlstra
  Cc: Nathan Lynch, Gautham R Shenoy, Vincent Guittot,
	Srikar Dronamraju, Rik van Riel, linuxppc-dev, Scott Cheloha,
	Geetika Moolchandani, LKML, Dietmar Eggemann, Thomas Gleixner,
	Mel Gorman, Valentin Schneider
In-Reply-To: <20210520154427.1041031-1-srikar@linux.vnet.ibm.com>

As per PAPR that defines the OS to hypervisor interface on POWER,
there is no way to calculate the node_distance between 2 nodes, when
either of the nodes are offline. However scheduler needs the distance
map to be populated at boot time. On POWER, this information is
provided within the distance_ref_points_depth array, which needs to be
parsed to extract the potential node distances.

To handle this scenario, lets overload arch_populate_distance_map(),
to provide all the distances that are possible in the current
platform.

Cc: LKML <linux-kernel@vger.kernel.org>
Cc: linuxppc-dev@lists.ozlabs.org
Cc: Nathan Lynch <nathanl@linux.ibm.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Valentin Schneider <valentin.schneider@arm.com>
Cc: Scott Cheloha <cheloha@linux.ibm.com>
Cc: Gautham R Shenoy <ego@linux.vnet.ibm.com>
Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
Cc: Mel Gorman <mgorman@techsingularity.net>
Cc: Vincent Guittot <vincent.guittot@linaro.org>
Cc: Rik van Riel <riel@surriel.com>
Cc: Geetika Moolchandani <Geetika.Moolchandani1@ibm.com>
Reported-by: Geetika Moolchandani <Geetika.Moolchandani1@ibm.com>
Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/topology.h |  3 +++
 arch/powerpc/mm/numa.c              | 19 +++++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/arch/powerpc/include/asm/topology.h b/arch/powerpc/include/asm/topology.h
index e4db64c0e184..d7605d833b8d 100644
--- a/arch/powerpc/include/asm/topology.h
+++ b/arch/powerpc/include/asm/topology.h
@@ -22,6 +22,9 @@ struct drmem_lmb;
 			       cpu_all_mask :				\
 			       node_to_cpumask_map[node])
 
+#define arch_populate_distance_map arch_populate_distance_map
+extern int arch_populate_distance_map(unsigned long *distance_map);
+
 struct pci_bus;
 #ifdef CONFIG_PCI
 extern int pcibus_to_node(struct pci_bus *bus);
diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
index f2bf98bdcea2..9a225b29814a 100644
--- a/arch/powerpc/mm/numa.c
+++ b/arch/powerpc/mm/numa.c
@@ -221,6 +221,25 @@ static void initialize_distance_lookup_table(int nid,
 	}
 }
 
+int arch_populate_distance_map(unsigned long *distance_map)
+{
+	int i;
+	int distance = LOCAL_DISTANCE;
+
+	bitmap_set(distance_map, distance, 1);
+
+	if (!form1_affinity) {
+		bitmap_set(distance_map, REMOTE_DISTANCE, 1);
+		return 0;
+	}
+
+	for (i = 0; i < distance_ref_points_depth; i++) {
+		distance *= 2;
+		bitmap_set(distance_map, distance, 1);
+	}
+	return 0;
+}
+
 /*
  * Returns nid in the range [0..nr_node_ids], or -1 if no useful NUMA
  * info is found.
-- 
2.27.0


^ permalink raw reply related

* [PATCH 1/3] sched/topology: Allow archs to populate distance map
From: Srikar Dronamraju @ 2021-05-20 15:44 UTC (permalink / raw)
  To: Ingo Molnar, Peter Zijlstra
  Cc: Nathan Lynch, Gautham R Shenoy, Vincent Guittot,
	Srikar Dronamraju, Rik van Riel, linuxppc-dev, Scott Cheloha,
	Geetika Moolchandani, LKML, Dietmar Eggemann, Thomas Gleixner,
	Mel Gorman, Valentin Schneider
In-Reply-To: <20210520154427.1041031-1-srikar@linux.vnet.ibm.com>

Currently scheduler populates the distance map by looking at distance
of each node from all other nodes. This should work for most
architectures and platforms.

However there are some architectures like POWER that may not expose
the distance of nodes that are not yet onlined because those resources
are not yet allocated to the OS instance. Such architectures have
other means to provide valid distance data for the current platform.

For example distance info from numactl from a fully populated 8 node
system at boot may look like this.

node distances:
node   0   1   2   3   4   5   6   7
  0:  10  20  40  40  40  40  40  40
  1:  20  10  40  40  40  40  40  40
  2:  40  40  10  20  40  40  40  40
  3:  40  40  20  10  40  40  40  40
  4:  40  40  40  40  10  20  40  40
  5:  40  40  40  40  20  10  40  40
  6:  40  40  40  40  40  40  10  20
  7:  40  40  40  40  40  40  20  10

However the same system when only two nodes are online at boot, then the
numa topology will look like
node distances:
node   0   1
  0:  10  20
  1:  20  10

It may be implementation dependent on what node_distance(0,3) where
node 0 is online and node 3 is offline. In POWER case, it returns
LOCAL_DISTANCE(10). Here at boot the scheduler would assume that the max
distance between nodes is 20. However that would not be true.

When Nodes are onlined and CPUs from those nodes are hotplugged,
the max node distance would be 40.

To handle such scenarios, let scheduler allow architectures to populate
the distance map. Architectures that like to populate the distance map
can overload arch_populate_distance_map().

Cc: LKML <linux-kernel@vger.kernel.org>
Cc: linuxppc-dev@lists.ozlabs.org
Cc: Nathan Lynch <nathanl@linux.ibm.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Valentin Schneider <valentin.schneider@arm.com>
Cc: Scott Cheloha <cheloha@linux.ibm.com>
Cc: Gautham R Shenoy <ego@linux.vnet.ibm.com>
Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
Cc: Mel Gorman <mgorman@techsingularity.net>
Cc: Vincent Guittot <vincent.guittot@linaro.org>
Cc: Rik van Riel <riel@surriel.com>
Cc: Geetika Moolchandani <Geetika.Moolchandani1@ibm.com>
Reported-by: Geetika Moolchandani <Geetika.Moolchandani1@ibm.com>
Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
---
 kernel/sched/topology.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
index 053115b55f89..ccb9aff59add 100644
--- a/kernel/sched/topology.c
+++ b/kernel/sched/topology.c
@@ -1630,6 +1630,26 @@ static void init_numa_topology_type(void)
 
 #define NR_DISTANCE_VALUES (1 << DISTANCE_BITS)
 
+#ifndef arch_populate_distance_map
+static int arch_populate_distance_map(unsigned long *distance_map)
+{
+	int i, j;
+
+	for (i = 0; i < nr_node_ids; i++) {
+		for (j = 0; j < nr_node_ids; j++) {
+			int distance = node_distance(i, j);
+
+			if (distance < LOCAL_DISTANCE || distance >= NR_DISTANCE_VALUES) {
+				sched_numa_warn("Invalid distance value range");
+				return -1;
+			}
+			bitmap_set(distance_map, distance, 1);
+		}
+	}
+	return 0;
+}
+#endif
+
 void sched_init_numa(void)
 {
 	struct sched_domain_topology_level *tl;
@@ -1646,18 +1666,10 @@ void sched_init_numa(void)
 		return;
 
 	bitmap_zero(distance_map, NR_DISTANCE_VALUES);
-	for (i = 0; i < nr_node_ids; i++) {
-		for (j = 0; j < nr_node_ids; j++) {
-			int distance = node_distance(i, j);
 
-			if (distance < LOCAL_DISTANCE || distance >= NR_DISTANCE_VALUES) {
-				sched_numa_warn("Invalid distance value range");
-				return;
-			}
+	if (arch_populate_distance_map(distance_map))
+		return;
 
-			bitmap_set(distance_map, distance, 1);
-		}
-	}
 	/*
 	 * We can now figure out how many unique distance values there are and
 	 * allocate memory accordingly.
-- 
2.27.0


^ permalink raw reply related

* [PATCH 0/3] Skip numa distance for offline nodes
From: Srikar Dronamraju @ 2021-05-20 15:44 UTC (permalink / raw)
  To: Ingo Molnar, Peter Zijlstra
  Cc: Nathan Lynch, Gautham R Shenoy, Vincent Guittot,
	Srikar Dronamraju, Rik van Riel, linuxppc-dev, Scott Cheloha,
	Geetika Moolchandani, LKML, Dietmar Eggemann, Thomas Gleixner,
	Mel Gorman, Valentin Schneider

Geetika reported yet another trace while doing a dlpar CPU add
operation. This was true even on top of a recent commit
6980d13f0dd1 ("powerpc/smp: Set numa node before updating mask") which fixed
a similar trace.

WARNING: CPU: 40 PID: 2954 at kernel/sched/topology.c:2088 build_sched_domains+0x6e8/0x1540
Modules linked in: nft_counter nft_compat rpadlpar_io rpaphp mptcp_diag
xsk_diag tcp_diag udp_diag raw_diag inet_diag unix_diag af_packet_diag
netlink_diag bonding tls nft_fib_inet nft_fib_ipv4 nft_fib_ipv6 nft_fib
nft_reject_inet nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_ct nft_chain_nat
nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 ip_set rfkill nf_tables
nfnetlink dm_multipath pseries_rng xts vmx_crypto binfmt_misc ip_tables xfs
libcrc32c sd_mod t10_pi sg ibmvscsi ibmveth scsi_transport_srp dm_mirror
dm_region_hash dm_log dm_mod fuse
CPU: 40 PID: 2954 Comm: kworker/40:0 Not tainted 5.13.0-rc1+ #19
Workqueue: events cpuset_hotplug_workfn
NIP:  c0000000001de588 LR: c0000000001de584 CTR: 00000000006cd36c
REGS: c00000002772b250 TRAP: 0700   Not tainted  (5.12.0-rc5-master+)
MSR:  8000000000029033 <SF,EE,ME,IR,DR,RI,LE>  CR: 28828422  XER: 0000000d
CFAR: c00000000020c2f8 IRQMASK: 0 #012GPR00: c0000000001de584 c00000002772b4f0
c000000001f55400 0000000000000036 #012GPR04: c0000063c6368010 c0000063c63f0a00
0000000000000027 c0000063c6368018 #012GPR08: 0000000000000023 c0000063c636ef48
00000063c4de0000 c0000063bfe9ffe8 #012GPR12: 0000000028828424 c0000063fe68fe80
0000000000000000 0000000000000417 #012GPR16: 0000000000000028 c00000000740dcd8
c00000000205db68 c000000001a3a4a0 #012GPR20: c000000091ed7d20 c000000091ed8520
0000000000000001 0000000000000000 #012GPR24: c0000000113a9600 0000000000000190
0000000000000028 c0000000010e3ac0 #012GPR28: 0000000000000000 c00000000740dd00
c0000000317b5900 0000000000000190 
NIP [c0000000001de588] build_sched_domains+0x6e8/0x1540
LR [c0000000001de584] build_sched_domains+0x6e4/0x1540
Call Trace:
[c00000002772b4f0] [c0000000001de584] build_sched_domains+0x6e4/0x1540 (unreliable)
[c00000002772b640] [c0000000001e08dc] partition_sched_domains_locked+0x3ec/0x530
[c00000002772b6e0] [c0000000002a2144] rebuild_sched_domains_locked+0x524/0xbf0
[c00000002772b7e0] [c0000000002a5620] rebuild_sched_domains+0x40/0x70
[c00000002772b810] [c0000000002a58e4] cpuset_hotplug_workfn+0x294/0xe20
[c00000002772bc30] [c000000000187510] process_one_work+0x300/0x670
[c00000002772bd10] [c0000000001878f8] worker_thread+0x78/0x520
[c00000002772bda0] [c0000000001937f0] kthread+0x1a0/0x1b0
[c00000002772be10] [c00000000000d6ec] ret_from_kernel_thread+0x5c/0x70
Instruction dump:
7ee5bb78 7f0ac378 7f29cb78 7f68db78 7f46d378 7f84e378 f8610068 3c62ff19
fbe10060 3863e558 4802dd31 60000000 <0fe00000> 3920fff4 f9210080 e86100b0

Detailed analysis of the failing scenario showed that the span in
question belongs to NODE domain and further the cpumasks for some
cpus in NODE overlapped. There are two possible reasons how we ended
up here:

(1) The numa node was offline or blank with no CPUs or memory. Hence
the sched_max_numa_distance could not be set correctly, or the
sched_domains_numa_distance happened to be partially populated.

(2) Depending on a bogus node_distance of an offline node to populate
cpumasks is the issue.  On POWER platform the node_distance is
correctly available only for an online node which has some CPU or
memory resource associated with it.

For example distance info from numactl from a fully populated 8 node
system at boot may look like this.

node distances:
node   0   1   2   3   4   5   6   7
  0:  10  20  40  40  40  40  40  40
  1:  20  10  40  40  40  40  40  40
  2:  40  40  10  20  40  40  40  40
  3:  40  40  20  10  40  40  40  40
  4:  40  40  40  40  10  20  40  40
  5:  40  40  40  40  20  10  40  40
  6:  40  40  40  40  40  40  10  20
  7:  40  40  40  40  40  40  20  10

However the same system when only two nodes are online at boot, then the
numa topology will look like
node distances:
node   0   1
  0:  10  20
  1:  20  10

This series tries to fix both these problems.
Note: These problems are now visible, thanks to 
Commit ccf74128d66c ("sched/topology: Assert non-NUMA topology masks don't
(partially) overlap")


Cc: LKML <linux-kernel@vger.kernel.org>
Cc: linuxppc-dev@lists.ozlabs.org
Cc: Nathan Lynch <nathanl@linux.ibm.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Valentin Schneider <valentin.schneider@arm.com>
Cc: Scott Cheloha <cheloha@linux.ibm.com>
Cc: Gautham R Shenoy <ego@linux.vnet.ibm.com>
Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
Cc: Mel Gorman <mgorman@techsingularity.net>
Cc: Vincent Guittot <vincent.guittot@linaro.org>
Cc: Rik van Riel <riel@surriel.com>
Cc: Geetika Moolchandani <Geetika.Moolchandani1@ibm.com>

Srikar Dronamraju (3):
  sched/topology: Allow archs to populate distance map
  powerpc/numa: Populate distance map correctly
  sched/topology: Skip updating masks for non-online nodes

 arch/powerpc/include/asm/topology.h |  3 +++
 arch/powerpc/mm/numa.c              | 19 +++++++++++++++
 kernel/sched/topology.c             | 38 +++++++++++++++++++++--------
 3 files changed, 50 insertions(+), 10 deletions(-)


base-commit: 1699949d3314e5d1956fb082e4cd4798bf6149fc
-- 
2.27.0


^ permalink raw reply

* Re: [PATCH v5 7/9] mm/mremap: Move TLB flush outside page table lock
From: Aneesh Kumar K.V @ 2021-05-20 15:26 UTC (permalink / raw)
  To: linux-mm, akpm; +Cc: npiggin, kaleshsingh, joel, linuxppc-dev
In-Reply-To: <20210422054323.150993-8-aneesh.kumar@linux.ibm.com>

On 4/22/21 11:13 AM, Aneesh Kumar K.V wrote:
> Move TLB flush outside page table lock so that kernel does
> less with page table lock held. Releasing the ptl with old
> TLB contents still valid will behave such that such access
> happened before the level3 or level2 entry update.
> 


Ok this break the page lifetime rule

commit: eb66ae030829 ("mremap: properly flush TLB before releasing the 
page")

I will respin dropping this change and add a comment around explaining 
why we need to do tlb flush before dropping ptl.

> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
>   mm/mremap.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/mremap.c b/mm/mremap.c
> index 109560977944..9effca76bf17 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -258,7 +258,7 @@ static bool move_normal_pmd(struct vm_area_struct *vma, unsigned long old_addr,
>   	 * We don't have to worry about the ordering of src and dst
>   	 * ptlocks because exclusive mmap_lock prevents deadlock.
>   	 */
> -	old_ptl = pmd_lock(vma->vm_mm, old_pmd);
> +	old_ptl = pmd_lock(mm, old_pmd);
>   	new_ptl = pmd_lockptr(mm, new_pmd);
>   	if (new_ptl != old_ptl)
>   		spin_lock_nested(new_ptl, SINGLE_DEPTH_NESTING);
> @@ -270,11 +270,11 @@ static bool move_normal_pmd(struct vm_area_struct *vma, unsigned long old_addr,
>   	VM_BUG_ON(!pmd_none(*new_pmd));
>   	pmd_populate(mm, new_pmd, (pgtable_t)pmd_page_vaddr(pmd));
>   
> -	flush_pte_tlb_pwc_range(vma, old_addr, old_addr + PMD_SIZE);
>   	if (new_ptl != old_ptl)
>   		spin_unlock(new_ptl);
>   	spin_unlock(old_ptl);
>   
> +	flush_pte_tlb_pwc_range(vma, old_addr, old_addr + PMD_SIZE);
>   	return true;
>   }
>   #else
> @@ -305,7 +305,7 @@ static bool move_normal_pud(struct vm_area_struct *vma, unsigned long old_addr,
>   	 * We don't have to worry about the ordering of src and dst
>   	 * ptlocks because exclusive mmap_lock prevents deadlock.
>   	 */
> -	old_ptl = pud_lock(vma->vm_mm, old_pud);
> +	old_ptl = pud_lock(mm, old_pud);
>   	new_ptl = pud_lockptr(mm, new_pud);
>   	if (new_ptl != old_ptl)
>   		spin_lock_nested(new_ptl, SINGLE_DEPTH_NESTING);
> @@ -317,11 +317,11 @@ static bool move_normal_pud(struct vm_area_struct *vma, unsigned long old_addr,
>   	VM_BUG_ON(!pud_none(*new_pud));
>   
>   	pud_populate(mm, new_pud, (pmd_t *)pud_page_vaddr(pud));
> -	flush_pte_tlb_pwc_range(vma, old_addr, old_addr + PUD_SIZE);
>   	if (new_ptl != old_ptl)
>   		spin_unlock(new_ptl);
>   	spin_unlock(old_ptl);
>   
> +	flush_pte_tlb_pwc_range(vma, old_addr, old_addr + PUD_SIZE);
>   	return true;
>   }
>   #else
> 


^ permalink raw reply

* Re: [PATCH 6/9] tty: hvc_console: Fix coding style issues of block comments
From: Joe Perches @ 2021-05-20 15:21 UTC (permalink / raw)
  To: Johan Hovold, Xiaofei Tan
  Cc: gregkh, linuxppc-dev, jirislaby, linux-kernel, linuxarm
In-Reply-To: <YKYcFfKiHT39Gyey@hovoldconsulting.com>

On Thu, 2021-05-20 at 10:21 +0200, Johan Hovold wrote:
> On Tue, May 18, 2021 at 12:01:22PM +0800, Xiaofei Tan wrote:
> > On 2021/5/17 22:15, Johan Hovold wrote:
> > > On Mon, May 17, 2021 at 02:37:10PM +0800, Xiaofei Tan wrote:
> > > > Fix coding style issues of block comments, reported by checkpatch.pl.
> > > > Besides, add a period at the end of the sentenses.
[]
> > > > diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
[]
> > > > @@ -177,7 +177,8 @@ static void hvc_console_print(struct console *co, const char *b,
> > > >  			r = cons_ops[index]->put_chars(vtermnos[index], c, i);
> > > >  			if (r <= 0) {
> > > >  				/* throw away characters on error
> > > > -				 * but spin in case of -EAGAIN */
> > > > +				 * but spin in case of -EAGAIN.
> > > > +				 */
> > > 
> > > How is this an improvement? First, the multi-line comment style is
> > > 
> > > 	/*
> > > 	 * ...
> > > 	 */
> > > 
> > 
> > Yes, mostly we use this style. I can follow it if new version is needed.
> 
> This is the preferred style outside of networking.
> 
> > BTW, How about add the '/*' check into checkpatch.pl?
> 
> Checkpatch already has too many checks IMO

I sometimes agree.  What checkpatch messages do you think are excessive?

> and I'm a bit surprised that
> it doesn't check this already. Perhaps it's because you used the -f to
> run checkpatch on in-kernel code, which you should not.

Likely not.  If it was run on a suggested patch, checkpatch doesn't emit
many messages on unmodified patch context lines.  And it shouldn't.

> it's just that you
> introduce noise in the logs and do pointless changes of context which
> makes it harder to use tools like git blame and makes backporting harder
> for no good reason.

Pretty pointless metric IMO.  Context changes in comments are mostly harmless.
IMO: backporting of these sorts non-bug fix changes is done _far_ too often.


^ permalink raw reply

* Re: [PATCH 0/9] tty: hvc_console: Fix some coding style issues
From: Greg KH @ 2021-05-20 15:11 UTC (permalink / raw)
  To: Xiaofei Tan; +Cc: linuxppc-dev, jirislaby, linux-kernel, linuxarm
In-Reply-To: <1621233433-27094-1-git-send-email-tanxiaofei@huawei.com>

On Mon, May 17, 2021 at 02:37:04PM +0800, Xiaofei Tan wrote:
> Fix some issues reported by checkpatch.pl. All of them are
> coding style issues, no function changes.
> 
> Xiaofei Tan (9):
>   tty: hvc_console: Fix spaces required around that '='
>   tty: hvc_console: Fix "foo * bar" should be "foo *bar"
>   tty: hvc_console: Remove trailing whitespace
>   tty: hvc_console: Fix issues of code indent should use tabs
>   tty: hvc_console: Delete spaces prohibited around open parenthesis '('
>     and ')'
>   tty: hvc_console: Fix coding style issues of block comments
>   tty: hvc_console: Add a blank line after declarations
>   tty: hvc_console: Remove the repeated words 'no' and 'from'
>   tty: hvc_console: Move open brace { on the previous line
> 
>  drivers/tty/hvc/hvc_console.c | 37 ++++++++++++++++++++++---------------
>  1 file changed, 22 insertions(+), 15 deletions(-)

Do you use this driver?  If so, great, I'm sure there are other "real"
issues in it that need some work.  But as Johan points out, doing
drive-by checkpatch cleanups on random files that you do not use, isn't
the best thing to do.

If you just want to do this type of work, please do so in
drivers/staging/ as it is most welcome there if you wish to get involved
in kernel work to get experience before doing "real" stuff.

thanks,
g
reg k-h

^ permalink raw reply

* Re: [PATCH v5 3/9] mm/mremap: Use pmd/pud_poplulate to update page table entries
From: Peter Xu @ 2021-05-20 14:57 UTC (permalink / raw)
  To: Aneesh Kumar K.V
  Cc: npiggin, Nathan Chancellor, linux-mm, kaleshsingh, joel, akpm,
	linuxppc-dev
In-Reply-To: <87o8d5le4q.fsf@linux.ibm.com>

On Thu, May 20, 2021 at 07:07:57PM +0530, Aneesh Kumar K.V wrote:
> "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
> 
> > On 5/20/21 6:16 PM, Peter Xu wrote:
> >> On Thu, May 20, 2021 at 01:56:54PM +0530, Aneesh Kumar K.V wrote:
> >>>> This seems to work at least for my userfaultfd test on shmem, however I don't
> >>>> fully understand the commit message [1] on: How do we guarantee we're not
> >>>> moving a thp pte?
> >>>>
> >>>
> >>> move_page_tables() checks for pmd_trans_huge() and ends up calling
> >>> move_huge_pmd if it is a THP entry.
> >> 
> >> Sorry to be unclear: what if a huge pud thp?
> >> 
> >
> > I am still checking. Looking at the code before commit 
> > c49dd340180260c6239e453263a9a244da9a7c85, I don't see kernel handling 
> > huge pud thp. I haven't studied huge pud thp enough to understand 
> > whether c49dd340180260c6239e453263a9a244da9a7c85 intent to add that 
> > support.
> >
> > We can do a move_huge_pud() like we do for huge pmd thp. But I am not 
> > sure whether we handle those VMA's earlier and restrict mremap on them?
> 
> something like this? (not even compile tested). I am still not sure
> whether this is really needed or we handle DAX VMA's in some other form.

Yeah maybe (you may want to at least drop that extra "case HPAGE_PUD").

It's just that if with CONFIG_HAVE_MOVE_PUD (x86 and arm64 enables it by
default so far) it does seem to work even with huge pud, while after this patch
it seems to be not working anymore, even with your follow up fix.

Indeed I saw CONFIG_HAVE_MOVE_PUD is introduced a few months ago so breaking
someone seems to be unlikely, perhaps no real user yet to mremap() a huge pud
for dax or whatever backend?

Ideally maybe rework this patch (or series?) and repost it for a better review?
Agree the risk seems low.  I'll leave that to you and Andrew to decide..

-- 
Peter Xu


^ permalink raw reply

* Re: [PATCH 31/31] genirq: Improve "hwirq" output in /proc and /sys/
From: Cédric Le Goater @ 2021-05-20 12:27 UTC (permalink / raw)
  To: Thomas Gleixner, linuxppc-dev
In-Reply-To: <87mtsx2g7l.ffs@nanos.tec.linutronix.de>

On 5/14/21 10:49 PM, Thomas Gleixner wrote:
> On Fri, Apr 30 2021 at 10:04, Cédric Le Goater wrote:
>> The HW IRQ numbers generated by the PCI MSI layer can be quite large
>> on a pSeries machine when running under the IBM Hypervisor and they
>> appear as negative. Use '%u' to show them correctly.
>>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  kernel/irq/irqdesc.c | 2 +-
>>  kernel/irq/proc.c    | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
>> index cc1a09406c6e..85054eb2ae51 100644
>> --- a/kernel/irq/irqdesc.c
>> +++ b/kernel/irq/irqdesc.c
>> @@ -188,7 +188,7 @@ static ssize_t hwirq_show(struct kobject *kobj,
>>  
>>  	raw_spin_lock_irq(&desc->lock);
>>  	if (desc->irq_data.domain)
>> -		ret = sprintf(buf, "%d\n", (int)desc->irq_data.hwirq);
>> +		ret = sprintf(buf, "%u\n", (int)desc->irq_data.hwirq);
> 
> Which makes the (int) cast pointless, right?

Well, hwirq is a long. Would you prefer a "%lu" for both ?

Thanks,

C.

> 
>>  	raw_spin_unlock_irq(&desc->lock);
>>  
>>  	return ret;
>> diff --git a/kernel/irq/proc.c b/kernel/irq/proc.c
>> index 98138788cb04..e2392f05da04 100644
>> --- a/kernel/irq/proc.c
>> +++ b/kernel/irq/proc.c
>> @@ -513,7 +513,7 @@ int show_interrupts(struct seq_file *p, void *v)
>>  		seq_printf(p, " %8s", "None");
>>  	}
>>  	if (desc->irq_data.domain)
>> -		seq_printf(p, " %*d", prec, (int) desc->irq_data.hwirq);
>> +		seq_printf(p, " %*u", prec, (int)desc->irq_data.hwirq);
> 
> ditto.
> 
> Thanks,
> 
>         tglx
> 


^ permalink raw reply

* [PATCH] selftests/powerpc: Fix "no_handler" EBB selftest for ISA v3.1
From: Athira Rajeev @ 2021-05-20 14:35 UTC (permalink / raw)
  To: mpe; +Cc: nasastry, shirisha.ganta1, maddy, linuxppc-dev

The "no_handler_test" in ebb selftests attempts to read the PMU
registers after closing of the event via helper function
"dump_ebb_state". With the MMCR0 control bit (PMCCEXT) in ISA v3.1,
read access to group B registers is restricted when MMCR0 PMCC=0b00.
Hence the call to dump_ebb_state after closing of event will generate
a SIGILL, which is expected.

Test has below in logs:

<<>>
!! child died by signal 4
failure: no_handler_test
<<>>

In other platforms (like power9), the older behaviour works where
group B PMU SPRs are readable. Patch fixes the selftest to handle
the sigill for ISA v3.1.

Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
Reported-by: Shirisha Ganta <shirisha.ganta1@ibm.com>
---
 tools/testing/selftests/powerpc/pmu/ebb/no_handler_test.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/powerpc/pmu/ebb/no_handler_test.c b/tools/testing/selftests/powerpc/pmu/ebb/no_handler_test.c
index fc5bf48..5f57a9d 100644
--- a/tools/testing/selftests/powerpc/pmu/ebb/no_handler_test.c
+++ b/tools/testing/selftests/powerpc/pmu/ebb/no_handler_test.c
@@ -50,7 +50,17 @@ static int no_handler_test(void)
 
 	event_close(&event);
 
-	dump_ebb_state();
+	/*
+	 * For ISA v3.1, verify the test takes a SIGILL when reading
+	 * PMU regs after the event is closed. With the control bit
+	 * in MMCR0 (PMCCEXT) restricting access to group B PMU regs,
+	 * sigill is expected.
+	 */
+
+	if (have_hwcap2(PPC_FEATURE2_ARCH_3_1))
+		FAIL_IF(catch_sigill(dump_ebb_state));
+	else
+		dump_ebb_state();
 
 	/* The real test is that we never took an EBB at 0x0 */
 
-- 
1.8.3.1


^ permalink raw reply related

* Re: [PATCH 6/9] tty: hvc_console: Fix coding style issues of block comments
From: Johan Hovold @ 2021-05-20 13:52 UTC (permalink / raw)
  To: Xiaofei Tan; +Cc: gregkh, linuxppc-dev, jirislaby, linux-kernel, linuxarm
In-Reply-To: <7e63a708-64c4-b369-066b-7f83d65bf178@huawei.com>

On Thu, May 20, 2021 at 09:21:25PM +0800, Xiaofei Tan wrote:

> > Checkpatch already has too many checks IMO and I'm a bit surprised that
> > it doesn't check this already. Perhaps it's because you used the -f to
> > run checkpatch on in-kernel code, which you should not.
> >
> >>> Second, that sentence is not capitalised so why do add a period?
> >>>
> >>
> >> How about capitalize the sentence, or just remove the period ?
> >
> > How about just leaving this unchanged?
> 
> OK
> And I will keep the patch 8/9, and combine space issues into
> one new patch, and remove the others.

Yeah, 8/9 is arguably a fix even if it's for a very minor issue
(repeated words in a comment).

It doesn't look like any of the white space issues are worth fixing,
though. Such pedantry can usually be addressed when the code in question
is being modified for other reasons.

Johan

^ permalink raw reply


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