* Re: RFC: Restricting userspace interfaces for CXL fabric management
From: Sreenivas Bagalkote @ 2024-03-21 21:41 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-cxl, Brett Henning, Harold Johnson, Sumanesh Samanta,
Williams, Dan J, linux-kernel, Davidlohr Bueso, Dave Jiang,
Alison Schofield, Vishal Verma, Ira Weiny, linuxarm, linux-api,
Lorenzo Pieralisi, Natu, Mahesh
In-Reply-To: <20240321174423.00007e0d@Huawei.com>
[-- Attachment #1.1: Type: text/plain, Size: 12219 bytes --]
Thank you for kicking off this discussion, Jonathan.
We need guidance from the community.
1. Datacenter customers must be able to manage PCIe switches in-band.
2. Management of switches includes getting health, performance, and error
telemetry.
3. These telemetry functions are not yet part of the CXL standard
4. We built the CCI mailboxes into our PCIe switches per CXL spec and
developed our management scheme around them.
If the Linux community does not allow a CXL spec-compliant switch to be
managed via the CXL spec-defined CCI mailbox, then please guide us on
the right approach. Please tell us how you propose we manage our switches
in-band.
Thank you
Sreeni
On Thu, Mar 21, 2024 at 10:44 AM Jonathan Cameron <
Jonathan.Cameron@huawei.com> wrote:
> Hi All,
>
> This is has come up in a number of discussions both on list and in private,
> so I wanted to lay out a potential set of rules when deciding whether or
> not
> to provide a user space interface for a particular feature of CXL Fabric
> Management. The intent is to drive discussion, not to simply tell people
> a set of rules. I've brought this to the public lists as it's a Linux
> kernel
> policy discussion, not a standards one.
>
> Whilst I'm writing the RFC this my attempt to summarize a possible
> position rather than necessarily being my personal view.
>
> It's a straw man - shoot at it!
>
> Not everyone in this discussion is familiar with relevant kernel or CXL
> concepts
> so I've provided more info than I normally would.
>
> First some background:
> ======================
>
> CXL has two different types of Fabric. The comments here refer to both, but
> for now the kernel stack is focused on the simpler VCS fabric, not the more
> recent Port Based Routing (PBR) Fabrics. A typical example for 2 hosts
> connected to a common switch looks something like:
>
> ________________ _______________
> | | | | Hosts - each sees
> | HOST A | | HOST B | a PCIe style tree
> | | | | but from a fabric
> config
> | |Root Port| | | |Root Port| | point of view it's more
> -------|-------- -------|------- complex.
> | |
> | |
> _______|______________________________|________
> | USP (SW-CCI) USP | Switch can have lots of
> | | | | Upstream Ports. Each one
> | ____|________ _______|______ | has a virtual hierarchy.
> | | | | | |
> | vPPB vPPB vPPB vPPB| There are virtual
> | x | | | | "downstream
> ports."(vPPBs)
> | \ / / | That can be bound to real
> | \ / / | downstream ports.
> | \ / / |
> | \ / / | Multi Logical Devices are
> | DSP0 DSP1 DSP 2 | support more than one
> vPPB
> ------------------------------------------------ bound to a single
> physical
> | | | DSP (transactions are
> tagged
> | | | with an LD-ID)
> SLD0 MLD0 SLD1
>
> Some typical fabric management activities:
> 1) Bind/Unbind vPPB to physical DSP (Results in hotplug / unplug events)
> 2) Access config space or BAR space of End Points below the switch.
> 3) Tunneling messages through to devices downstream (e.g Dynamic Capacity
> Forced Remove that will blow away some memory even if a host is using
> it).
> 4) Non destructive stuff like status read back.
>
> Given the hosts may be using the Type 3 hosted memory (either Single
> Logical
> Device - SLD, or an LD on a Multi logical Device - MLD) as normal memory,
> unbinding a device in use can result in the memory access from a
> different host being removed. The 'blast radius' is perhaps a rack of
> servers. This discussion applies equally to FM-API commands sent to Multi
> Head Devices (see CXL r3.1).
>
> The Fabric Management actions are done using the CXL spec defined Fabric
> Management API, (FM-API) which is transported over various means including
> OoB MCTP over your favourite transport (I2C, PCIe-VDM...) or via normal
> PCIe read/write to a Switch-CCI. A Switch-CCI is mailbox in PCI BAR
> space on a function found alongside one of the switch upstream ports;
> this mailbox is very similar to the MMPT definition found in PCIe r6.2.
>
> In many cases this switch CCI / MCTP connection is used by a BMC rather
> than a normal host, but there have been some questions raised about whether
> a general purpose server OS would have a valid reason to use this interface
> (beyond debug and testing) to configure the switch or an MHD.
>
> If people have a use case for this, please reply to this thread to give
> more details.
>
> The most recently posted CXL Switch-CCI support only provided the RAW CXL
> command IOCTL interface that is already available for Type 3 memory
> devices.
> That allows for unfettered control of the switch but, because it is
> extremely easy to shoot yourself in the foot and cause unsolvable bug
> reports,
> it taints the kernel. There have been several requests to provide this
> interface
> without the taint for these switch configuration mailboxes.
>
> Last posted series:
>
> https://lore.kernel.org/all/20231016125323.18318-1-Jonathan.Cameron@huawei.com/
> Note there are unrelated reasons why that code hasn't been updated since
> v6.6 time,
> but I am planning to get back to it shortly.
>
> Similar issues will occur for other uses of PCIe MMPT (new mailbox in PCI
> that
> sometimes is used for similarly destructive activity such as PLDM based
> firmware update).
>
>
> On to the proposed rules:
>
> 1) Kernel space use of the various mailboxes, or filtered controls from
> user space.
>
> ==================================================================================
>
> Absolutely fine - no one worries about this, but the mediated traffic will
> be filtered for potentially destructive side effects. E.g. it will reject
> attempts to change anything routing related if the kernel either knows a
> host is
> using memory that will be blown away, or has no way to know (so affecting
> routing to another host). This includes blocking 'all' vendor defined
> messages as we have no idea what the do. Note this means the kernel has
> an allow list and new commands are not initially allowed.
>
> This isn't currently enabled for Switch CCIs because they are only really
> interesting if the potentially destructive stuff is available (an earlier
> version did enable query commands, but it wasn't particularly useful to
> know what your switch could do but not be allowed to do any of it).
> If you take a MMPT usecase of PLDM firmware update, the filtering would
> check that the device was in a state where a firmware update won't rip
> memory out from under a host, which would be messy if that host is
> doing the update.
>
> 2) Unfiltered userspace use of mailbox for Fabric Management - BMC kernels
> ==========================================================================
>
> (This would just be a kernel option that we'd advise normal server
> distributions not to turn on. Would be enabled by openBMC etc)
>
> This is fine - there is some work to do, but the switch-cci PCI driver
> will hopefully be ready for upstream merge soon. There is no filtering of
> accesses. Think of this as similar to all the damage you can do via
> MCTP from a BMC. Similarly it is likely that much of the complexity
> of the actual commands will be left to user space tooling:
> https://gitlab.com/jic23/cxl-fmapi-tests has some test examples.
>
> Whether Kconfig help text is strong enough to ensure this only gets
> enabled for BMC targeted distros is an open question we can address
> alongside an updated patch set.
>
> (On to the one that the "debate" is about)
>
> 3) Unfiltered user space use of mailbox for Fabric Management - Distro
> kernels
>
> =============================================================================
> (General purpose Linux Server Distro (Redhat, Suse etc))
>
> This is equivalent of RAW command support on CXL Type 3 memory devices.
> You can enable those in a distro kernel build despite the scary config
> help text, but if you use it the kernel is tainted. The result
> of the taint is to add a flag to bug reports and print a big message to say
> that you've used a feature that might result in you shooting yourself
> in the foot.
>
> The taint is there because software is not at first written to deal with
> everything that can happen smoothly (e.g. surprise removal) It's hard
> to survive some of these events, so is never on the initial feature list
> for any bus, so this flag is just to indicate we have entered a world
> where almost all bets are off wrt to stability. We might not know what
> a command does so we can't assess the impact (and no one trusts vendor
> commands to report affects right in the Command Effects Log - which
> in theory tells you if a command can result problems).
>
> A concern was raised about GAE/FAST/LDST tables for CXL Fabrics
> (a r3.1 feature) but, as I understand it, these are intended for a
> host to configure and should not have side effects on other hosts?
> My working assumption is that the kernel driver stack will handle
> these (once we catch up with the current feature backlog!) Currently
> we have no visibility of what the OS driver stack for a fabrics will
> actually look like - the spec is just the starting point for that.
> (patches welcome ;)
>
> The various CXL upstream developers and maintainers may have
> differing views of course, but my current understanding is we want
> to support 1 and 2, but are very resistant to 3!
>
> General Notes
> =============
>
> One side aspect of why we really don't like unfiltered userspace access to
> any
> of these devices is that people start building non standard hacks in and we
> lose the ecosystem advantages. Forcing a considered discussion + patches
> to let a particular command be supported, drives standardization.
>
>
> https://lore.kernel.org/linux-cxl/CAPcyv4gDShAYih5iWabKg_eTHhuHm54vEAei8ZkcmHnPp3B0cw@mail.gmail.com/
> provides some history on vendor specific extensions and why in general we
> won't support them upstream.
>
> To address another question raised in an earlier discussion:
> Putting these Fabric Management interfaces behind guard rails of some type
> (e.g. CONFIG_IM_A_BMC_AND_CAN_MAKE_A_MESS) does not encourage the risk
> of non standard interfaces, because we will be even less likely to accept
> those upstream!
>
> If anyone needs more details on any aspect of this please ask.
> There are a lot of things involved and I've only tried to give a fairly
> minimal illustration to drive the discussion. I may well have missed
> something crucial.
>
> Jonathan
>
>
--
This electronic communication and the information and any files transmitted
with it, or attached to it, are confidential and are intended solely for
the use of the individual or entity to whom it is addressed and may contain
information that is confidential, legally privileged, protected by privacy
laws, or otherwise restricted from disclosure to anyone else. If you are
not the intended recipient or the person responsible for delivering the
e-mail to the intended recipient, you are hereby notified that any use,
copying, distributing, dissemination, forwarding, printing, or copying of
this e-mail is strictly prohibited. If you received this e-mail in error,
please return the e-mail to the sender, delete it from your computer, and
destroy any printed copy of it.
[-- Attachment #1.2: Type: text/html, Size: 14975 bytes --]
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4230 bytes --]
^ permalink raw reply
* RFC: Restricting userspace interfaces for CXL fabric management
From: Jonathan Cameron @ 2024-03-21 17:44 UTC (permalink / raw)
To: linux-cxl
Cc: Sreenivas Bagalkote, Brett Henning, Harold Johnson,
Sumanesh Samanta, Williams, Dan J, linux-kernel, Davidlohr Bueso,
Dave Jiang, Alison Schofield, Vishal Verma, Ira Weiny, linuxarm,
linux-api, Lorenzo Pieralisi, Natu, Mahesh
Hi All,
This is has come up in a number of discussions both on list and in private,
so I wanted to lay out a potential set of rules when deciding whether or not
to provide a user space interface for a particular feature of CXL Fabric
Management. The intent is to drive discussion, not to simply tell people
a set of rules. I've brought this to the public lists as it's a Linux kernel
policy discussion, not a standards one.
Whilst I'm writing the RFC this my attempt to summarize a possible
position rather than necessarily being my personal view.
It's a straw man - shoot at it!
Not everyone in this discussion is familiar with relevant kernel or CXL concepts
so I've provided more info than I normally would.
First some background:
======================
CXL has two different types of Fabric. The comments here refer to both, but
for now the kernel stack is focused on the simpler VCS fabric, not the more
recent Port Based Routing (PBR) Fabrics. A typical example for 2 hosts
connected to a common switch looks something like:
________________ _______________
| | | | Hosts - each sees
| HOST A | | HOST B | a PCIe style tree
| | | | but from a fabric config
| |Root Port| | | |Root Port| | point of view it's more
-------|-------- -------|------- complex.
| |
| |
_______|______________________________|________
| USP (SW-CCI) USP | Switch can have lots of
| | | | Upstream Ports. Each one
| ____|________ _______|______ | has a virtual hierarchy.
| | | | | |
| vPPB vPPB vPPB vPPB| There are virtual
| x | | | | "downstream ports."(vPPBs)
| \ / / | That can be bound to real
| \ / / | downstream ports.
| \ / / |
| \ / / | Multi Logical Devices are
| DSP0 DSP1 DSP 2 | support more than one vPPB
------------------------------------------------ bound to a single physical
| | | DSP (transactions are tagged
| | | with an LD-ID)
SLD0 MLD0 SLD1
Some typical fabric management activities:
1) Bind/Unbind vPPB to physical DSP (Results in hotplug / unplug events)
2) Access config space or BAR space of End Points below the switch.
3) Tunneling messages through to devices downstream (e.g Dynamic Capacity
Forced Remove that will blow away some memory even if a host is using it).
4) Non destructive stuff like status read back.
Given the hosts may be using the Type 3 hosted memory (either Single Logical
Device - SLD, or an LD on a Multi logical Device - MLD) as normal memory,
unbinding a device in use can result in the memory access from a
different host being removed. The 'blast radius' is perhaps a rack of
servers. This discussion applies equally to FM-API commands sent to Multi
Head Devices (see CXL r3.1).
The Fabric Management actions are done using the CXL spec defined Fabric
Management API, (FM-API) which is transported over various means including
OoB MCTP over your favourite transport (I2C, PCIe-VDM...) or via normal
PCIe read/write to a Switch-CCI. A Switch-CCI is mailbox in PCI BAR
space on a function found alongside one of the switch upstream ports;
this mailbox is very similar to the MMPT definition found in PCIe r6.2.
In many cases this switch CCI / MCTP connection is used by a BMC rather
than a normal host, but there have been some questions raised about whether
a general purpose server OS would have a valid reason to use this interface
(beyond debug and testing) to configure the switch or an MHD.
If people have a use case for this, please reply to this thread to give
more details.
The most recently posted CXL Switch-CCI support only provided the RAW CXL
command IOCTL interface that is already available for Type 3 memory devices.
That allows for unfettered control of the switch but, because it is
extremely easy to shoot yourself in the foot and cause unsolvable bug reports,
it taints the kernel. There have been several requests to provide this interface
without the taint for these switch configuration mailboxes.
Last posted series:
https://lore.kernel.org/all/20231016125323.18318-1-Jonathan.Cameron@huawei.com/
Note there are unrelated reasons why that code hasn't been updated since v6.6 time,
but I am planning to get back to it shortly.
Similar issues will occur for other uses of PCIe MMPT (new mailbox in PCI that
sometimes is used for similarly destructive activity such as PLDM based
firmware update).
On to the proposed rules:
1) Kernel space use of the various mailboxes, or filtered controls from user space.
==================================================================================
Absolutely fine - no one worries about this, but the mediated traffic will
be filtered for potentially destructive side effects. E.g. it will reject
attempts to change anything routing related if the kernel either knows a host is
using memory that will be blown away, or has no way to know (so affecting
routing to another host). This includes blocking 'all' vendor defined
messages as we have no idea what the do. Note this means the kernel has
an allow list and new commands are not initially allowed.
This isn't currently enabled for Switch CCIs because they are only really
interesting if the potentially destructive stuff is available (an earlier
version did enable query commands, but it wasn't particularly useful to
know what your switch could do but not be allowed to do any of it).
If you take a MMPT usecase of PLDM firmware update, the filtering would
check that the device was in a state where a firmware update won't rip
memory out from under a host, which would be messy if that host is
doing the update.
2) Unfiltered userspace use of mailbox for Fabric Management - BMC kernels
==========================================================================
(This would just be a kernel option that we'd advise normal server
distributions not to turn on. Would be enabled by openBMC etc)
This is fine - there is some work to do, but the switch-cci PCI driver
will hopefully be ready for upstream merge soon. There is no filtering of
accesses. Think of this as similar to all the damage you can do via
MCTP from a BMC. Similarly it is likely that much of the complexity
of the actual commands will be left to user space tooling:
https://gitlab.com/jic23/cxl-fmapi-tests has some test examples.
Whether Kconfig help text is strong enough to ensure this only gets
enabled for BMC targeted distros is an open question we can address
alongside an updated patch set.
(On to the one that the "debate" is about)
3) Unfiltered user space use of mailbox for Fabric Management - Distro kernels
=============================================================================
(General purpose Linux Server Distro (Redhat, Suse etc))
This is equivalent of RAW command support on CXL Type 3 memory devices.
You can enable those in a distro kernel build despite the scary config
help text, but if you use it the kernel is tainted. The result
of the taint is to add a flag to bug reports and print a big message to say
that you've used a feature that might result in you shooting yourself
in the foot.
The taint is there because software is not at first written to deal with
everything that can happen smoothly (e.g. surprise removal) It's hard
to survive some of these events, so is never on the initial feature list
for any bus, so this flag is just to indicate we have entered a world
where almost all bets are off wrt to stability. We might not know what
a command does so we can't assess the impact (and no one trusts vendor
commands to report affects right in the Command Effects Log - which
in theory tells you if a command can result problems).
A concern was raised about GAE/FAST/LDST tables for CXL Fabrics
(a r3.1 feature) but, as I understand it, these are intended for a
host to configure and should not have side effects on other hosts?
My working assumption is that the kernel driver stack will handle
these (once we catch up with the current feature backlog!) Currently
we have no visibility of what the OS driver stack for a fabrics will
actually look like - the spec is just the starting point for that.
(patches welcome ;)
The various CXL upstream developers and maintainers may have
differing views of course, but my current understanding is we want
to support 1 and 2, but are very resistant to 3!
General Notes
=============
One side aspect of why we really don't like unfiltered userspace access to any
of these devices is that people start building non standard hacks in and we
lose the ecosystem advantages. Forcing a considered discussion + patches
to let a particular command be supported, drives standardization.
https://lore.kernel.org/linux-cxl/CAPcyv4gDShAYih5iWabKg_eTHhuHm54vEAei8ZkcmHnPp3B0cw@mail.gmail.com/
provides some history on vendor specific extensions and why in general we
won't support them upstream.
To address another question raised in an earlier discussion:
Putting these Fabric Management interfaces behind guard rails of some type
(e.g. CONFIG_IM_A_BMC_AND_CAN_MAKE_A_MESS) does not encourage the risk
of non standard interfaces, because we will be even less likely to accept
those upstream!
If anyone needs more details on any aspect of this please ask.
There are a lot of things involved and I've only tried to give a fairly
minimal illustration to drive the discussion. I may well have missed
something crucial.
Jonathan
^ permalink raw reply
* Re: [PATCH v7] posix-timers: add clock_compare system call
From: Sagi Maimon @ 2024-03-20 14:42 UTC (permalink / raw)
To: Thomas Gleixner
Cc: richardcochran, luto, mingo, bp, dave.hansen, x86, hpa, arnd,
geert, peterz, hannes, sohil.mehta, rick.p.edgecombe, nphamcs,
palmer, keescook, legion, mark.rutland, mszeredi, casey, reibax,
davem, brauner, linux-kernel, linux-api, linux-arch, netdev
In-Reply-To: <874jd8n0ta.ffs@tglx>
On Thu, Mar 14, 2024 at 8:08 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> Sagi!
>
> On Thu, Mar 14 2024 at 14:19, Sagi Maimon wrote:
> > On Thu, Mar 14, 2024 at 1:12 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> >> On Thu, Mar 14 2024 at 11:05, Sagi Maimon wrote:
> >> > Some user space applications need to read a couple of different clocks.
> >> > Each read requires moving from user space to kernel space.
> >> > Reading each clock separately (syscall) introduces extra
> >> > unpredictable/unmeasurable delay. Minimizing this delay contributes to user
> >> > space actions on these clocks (e.g. synchronization etc).
> >>
> >> I asked for a proper description of the actual problem several times now
> >> and you still provide some handwaving blurb. Feel free to ignore me, but
> >> then please don't be surprised if I ignore you too.
> >>
> > Nobody is ignoring your notes, and I address any notes given by any
> > maintainer in the most serious way.
> > As far as explaining the actual problem this is the best that I can,
> > but let me try to explain better:
> > We did many tests with different CPU loading and compared sampling the
> > same clock twice,
> > once in user space and once by using the system call.
> > We have noticed an improvement up to hundreds of nanoseconds while
> > using the system call.
> > Those results improved our ability to sync different PHCs
>
> So let me express how I understand the problem - as far as I decoded it
> from your writeups:
>
> Synchronizing two PHCs requires to read timestamps from both and
> correlate them. Currently this requires several seperate system calls.
> This is subject to unmeasurable delays due to system call overhead,
> preemption and interrupts which makes the correlation imprecise.
>
> Therefore you want a system call, which samples two clocks at once, to
> make the correlation more precise.
>
> Right? For the further comments I assume this is what you are trying to
> say and to solve
You are right.
>
> So far so good, except that I do not agree with that reasoning at all:
>
> 1. The delays are measurable and as precise as the cross time stamp
> mechanism (hardware or software based) allows.
>
Most of the PHCs do not support crosstime stamps.
> 2. The system call overhead is completely irrelevant.
>
You are right in case of long preemption, but in other cases it is relevant.
> 3. The time deltas between the sample points are irrelevant within a
> reasonable upper bound to the time delta between the two outer
> sample points.
>
See below
> 4. The alledged higher precision is based on a guesstimate and not on
> correctness. Just because it behaves slightly better in testing
> does not make it any more correct.
>
See below
> 5. The problem can be solved with maximal possible accuracy by using
> the existing PTP IOCTLs.
>
> See below.
>
> >> Also why does reading two random clocks make any sense at all? Your code
> >> allows to read CLOCK_TAI and CLOCK_THREAD_CPUTIME_ID. What for?
> >>
> > Initially we needed to sync some different PHCs for our user space
> > application, that is why we came with this idea.
> > The first idea was an IOCTL that returned samples of several PHCs for
> > the need of synchronization.
> > Richard Cochran suggested a system call instead, which will add the
> > ability to get various system clocks, while this
> > implementation is more complex then IOCTL, I think that he was right,
> > for future usage.
>
> Which future usage? We are not introducing swiss army knife interfaces
> just because there might be an illusional use case somewhere in the
> unspecified future.
>
> Adding a system call needs a proper design and justification. Handwaving
> future usage is not enough.
>
> Documentation/process/adding-syscalls.rst is very clear about what is
> required for a new system call.
>
> >> This needs to be split up into:
> >>
> >> 1) Infrastructure in posix-timers.c
> >> 2) Wire up the syscall in x86
> >> 3) Infrastructure in posix-clock.c
> >> 4) Usage in ptp_clock.c
> >>
> >> and not as a big lump of everything.
> >>
> > I know, but I think the benefit worth it
>
> It's worth it because it makes review easier. It's well documented in
> the process documentation that patches should do one thing and not a
> whole lump of changes.
>
No problem it will be split into two different patches.
> >> > + if (!error) {
> >> > + if (clock_b == CLOCK_MONOTONIC_RAW) {
> >> > + ts_b = ktime_to_timespec64(xtstamp_a1.sys_monoraw);
> >> > + ts_a1 = ktime_to_timespec64(xtstamp_a1.device);
> >> > + goto out;
> >> > + } else if (clock_b == CLOCK_REALTIME) {
> >> > + ts_b = ktime_to_timespec64(xtstamp_a1.sys_realtime);
> >> > + ts_a1 = ktime_to_timespec64(xtstamp_a1.device);
> >> > + goto out;
> >> > + } else {
> >> > + crosstime_support_a = true;
> >>
> >> Right. If clock_b is anything else than CLOCK_MONOTONIC_RAW or
> >> CLOCK_REALTIME then this is true.
> >>
> >> > + }
> >> > + }
> >>
> >> So in case of an error, this just keeps going with an uninitialized
> >> xtstamp_a1 and if the clock_b part succeeds it continues and operates on
> >> garbage.
> >>
> > On error xtstamp_a1 will be taken again using clock_get_crosstimespec
> > so no one will be operating on garbage.
>
> It will not, because crosstime_support_a == false. It will therefore
> fall back to kc_a->clock_get_timespec(), no?
>
> Sorry, I misread the code vs. using the uninitialized value, but this is
> just unneccesary hard to follow.
>
> >> > + if (crosstime_support_a)
> >> > + error = kc_a->clock_get_crosstimespec(clock_a, &xtstamp_a2);
> >> > + else
> >> > + error = kc_a->clock_get_timespec(clock_a, &ts_a2);
> >> > +
> >> > + if (error)
> >> > + return error;
> >>
> >> The logic and the code flow here are unreadable garbage and there are
> >> zero comments what this is supposed to do.
> >>
> > I will add comments.
> > please no need to use negative words like "garbage" (not the first time),
> > please keep it professional and civilized.
>
> Let me rephrase:
>
> The code and the logic is incomprehensible unless I waste an unjustified
> amount of time to decode it. Sorry, I don't have that time.
>
> >> > + if (crosstime_support_a) {
> >> > + ktime_a = ktime_sub(xtstamp_a2.device, xtstamp_a1.device);
> >> > + ts_offs_err = ktime_divns(ktime_a, 2);
> >> > + ktime_a = ktime_add_ns(xtstamp_a1.device, (u64)ts_offs_err);
> >> > + ts_a1 = ktime_to_timespec64(ktime_a);
> >>
> >> This is just wrong.
> >>
> >> read(a1);
> >> read(b);
> >> read(a2);
> >>
> >> You _CANNOT_ assume that (a1 + ((a2 - a1) / 2) is anywhere close to the
> >> point in time where 'b' is read. This code is preemtible and
> >> interruptible. I explained this to you before.
> >>
> >> Your explanation in the comment above the function is just wishful
> >> thinking.
> >>
> > you explained it before, but still it is better then two consecutive
> > user space calls which are also preemptible
> > and the userspace to kernel context switch time is added.
>
> It might be marginally better, but it is still just _pretending_ that it
> does the right thing, is correct and better than the existing IOCTLs.
>
> If your user space implementation has the same algorithm, then I'm
> absolutely not surprised that the results are not useful. Why?
>
> You simply cannot use the midpoint of the outer samples if you want to
> have precise results if there is no guarantee that b was sampled exactly
> in the midpoint of a1 and a2. A hardware implementation might give that
> guarantee, but the kernel cannot.
>
> But why using the midpoint in the first place?
>
> There is absolutely no reason to do so because the sampling points a1, b
> and a2 can be precisely determined with the precision of the cross time
> stamp mechanism, which is best with a hardware based cross time stamp
> obviously.
>
> The whole point of ptp::info::getcrosststamp() is to get properly
> correlated clock samples of
>
> 1) PHC clock
> 2) CLOCK_MONOTONIC_RAW
> 3) CLOCK_REALTIME
>
> So if you take 3 samples:
>
> get_cross_timestamp(a1);
> get_cross_timestamp(b);
> get_cross_timestamp(a2);
>
> then each of them provides:
>
> - device time
> - correlated CLOCK_MONOTONIC_RAW
> - correlated CLOCK_REALTIME
>
> Ergo the obvious thing to do is:
>
> d1 = b.sys_monoraw - a1.sys_monoraw;
> d2 = a2.sys_monoraw - a1.sys_monoraw;
>
> tsa = a1.device + ((a2.device - a1.device) * d1) / d2;
>
> Which is maximaly precise under the assumption that in the time between
> the sample points a1 and a2 neither the system clock nor the PCH clocks
> are changing their frequency significantly. That is a valid assumption
> when you put a reasonable upper bound on d2.
>
You are right.
In fact, we are running this calculation on a user space application.
We use the new system call to get pairs of mono and PHC and then run
that calculation in user space.
That is why the system call returns pairs of clock samples and not the
diff between them.
> Even when the device does not implement getcrosststamp() then loop based
> sampling like it is implemented in the PTP_SYS_OFFSET[_EXTENDED] IOTCLs
> is providing reasonably accurate results to the extent possible.
>
> Your algorithm is imprecise by definition and you can apply as much
> testing as you want, it won't become magically correct. It's still a
> guesstimate, i.e. an estimate made without using adequate or complete
> information.
>
> Now why a new syscall?
>
> This can be done from user space with existing interfaces and the very
> same precision today:
>
> ioctl(fda, PTP_SYS_OFFSET*, &a1);
> ioctl(fdb, PTP_SYS_OFFSET*, &b);
> ioctl(fda, PTP_SYS_OFFSET*, &a2);
>
> u64 d1 = timespec_delta_ns(b.sys_monoraw, a1.sys_monoraw);
> u64 d2 = timespec_delta_ns(a2.sys_monoraw, a1.sys_monoraw);
> u64 td = (timespec_delta_ns(a2.device, a1.device) * d1) / d2
>
> tsa = timespec_add_ns(a1.device, td);
> tsb = b.device;
>
> with the extra benefit of:
>
> 1) The correct CLOCK_REALTIME at that sample point,
> i.e. b.sys_realtime
>
> 2) The correct CLOCK_MONOTONIC_RAW at that sample point,
> i.e. b.sys_monoraw
>
If PTP_SYS_OFFSET IOCTL returns sys_monoraw, then you are right, but
unfortunately the only IOCTL that returns sys_monoraw is
PTP_SYS_OFFSET_PRECISE (getcrosststamp)
And most of the drivers does not support it.
> It works with PTP_SYS_OFFSET_PRECISE and PTP_SYS_OFFSET[_EXTENDED], with
> the obvious limitations of PTP_SYS_OFFSET[_EXTENDED], which are still
> vastly superior to your proposed (a2 - a1) / 2 guestimate which is just
> reading the PCH clocks with clock_get_timespec().
>
It only works with PTP_SYS_OFFSET_PRECISE (which most of the NIC
drivers does not support and I take it under consideration in my
system call),
PTP_SYS_OFFSET_EXTENDED ioctl returns system time before, PHC, system
time after , and no monotic raw.
> It is completely independent of the load, the syscall overhead and the
> actual time delta between the sample points when you apply a reasonable
> upper bound for d2, i.e. the time delta between the sample points a1 and
> a2 to eliminate the issue that system clock and/or the PCH clocks change
> their frequency significantly during that time. You'd need to do that in
> the kernel too.
>
> The actual frequency difference between PCH A and system clock is
> completely irrelevant when the frequencies of both are stable accross
> the sample period.
>
> You might still argue that the time delta between the sample points a1
> and a2 matters and is slightly shorter in the kernel, but that is a
> non-argument because:
>
> 1) The kernel implementation does not guarantee atomicity of the
> consecutive samples either. The time delta is just statistically
> better, which is obviously useless when you want to make
> guarantees.
>
> 2) It does not matter when the time delta is slightly larger because
> you need a large frequency change of the involved clocks in the
> sample interval between the sample points a1 and a2 to make an
> actual difference in the resulting accuracy.
>
> A typical temperature drift of a high quality cyrstal is less than
> 1ppm per degree Celsius and even if you assume that the overall
> system drift is 10ppm per degree Celsius then still the actual
> error for a bound time delta between the sample points a1 and a2 is
> just somewhere in the irrelevant noise, unless you manage to blow
> torch or ice spray your crystals during the sample interval.
>
> If your clocks are not stable enough then nothing can cure it and
> you cannot do high precision timekeeping with them.
>
You are right in case of long preemption (which still the system call
is better), but in other cases it is relevant.
> So what is your new syscall solving that can't be done with the existing
> IOCTLs other than providing worse precision results based on
> guesstimates and some handwavy future use for random clock ids?
>
> Nothing as far as I can tell, but I might be missing something important
> here.
>
Few points to consider:
1) Most of the PHCs do not support cross time stamping.
2) Users can implement your suggested code in user space while using
the new system call to get pairs of mono and PHC
. This is what we did already in user space.
3) User with less tight requirement will benefit high accuracy with
the new system call
> Thanks,
>
> tglx
> ---
> arch/x86/kernel/tsc.c:119: "Math is hard, let's go shopping." - John Stultz
^ permalink raw reply
* Re: [RFC v3 0/3] move_phys_pages syscall - migrate page contents given
From: Huang, Ying @ 2024-03-20 6:01 UTC (permalink / raw)
To: Gregory Price
Cc: Gregory Price, linux-mm, linux-api, linux-arch, linux-kselftest,
linux-kernel, dan.j.williams, honggyu.kim, corbet, arnd, luto,
akpm, shuah
In-Reply-To: <Zfpohg3EGxxOEcWg@memverge.com>
Gregory Price <gregory.price@memverge.com> writes:
> On Wed, Mar 20, 2024 at 10:48:44AM +0800, Huang, Ying wrote:
>> Gregory Price <gourry.memverge@gmail.com> writes:
>>
>> > Doing this reverse-translation outside of the kernel requires considerable
>> > space and compute, and it will have to be performed again by the existing
>> > system calls. Much of this work can be avoided if the pages can be
>> > migrated directly with physical memory addressing.
>>
>> One difficulty of the idea of the physical address is that we lacks some
>> user space specified policy information to make decision. For example,
>> users may want to pin some pages in DRAM to improve latency, or pin some
>> pages in CXL memory to do some best effort work. To make the correct
>> decision, we need PID and virtual address.
>>
>
> I think of this as a second or third order problem. The core problem
> right now isn't the practicality of how userland would actually use this
> interface - the core problem is whether the data generated by offloaded
> monitoring is even worth collecting and operating on in the first place.
>
> So this is a quick hack to do some research about whether it's even
> worth developing the whole abstraction described by Willy.
>
> This is why it's labeled RFC. I upped a v3 because I know of two groups
> actively looking at using it for research, and because the folio updates
> broke the old version. It's also easier for me to engage through the
> list than via private channels for this particular work.
>
>
> Do I suggest we merge this interface as-is? No, too many concerns about
> side channels. However, it's a clean reuse of move_pages code to
> bootstrap the investigation, and it at least gets the gears turning.
Got it! Thanks for detailed explanation.
I think that one of the difficulties of offloaded monitoring is that
it's hard to obey these user specified policies. The policies may
become more complex in the future, for example, allocate DRAM among
workloads.
> Example notes from a sidebar earlier today:
>
> * An interesting proposal from Dan Williams would be to provide some
> sort of `/sys/.../memory_tiering/tierN/promote_hot` interface, with
> a callback mechanism into the relevant hardware drivers that allows
> for this to be abstracted. This could be done on some interval and
> some threshhold (# pages, hotness threshhold, etc).
>
>
> The code to execute promotions ends up looking like what I have now
>
> 1) Validate the page is elgibile to be promoted by walking the vmas
> 2) invoking the existing move_pages code
>
> The above idea can be implemented trivially in userland without
> having to plumb through a whole brand new callback system.
>
>
> Sometimes you have to post stupid ideas to get to the good ones :]
>
--
Best Regards,
Huang, Ying
^ permalink raw reply
* Re: [RFC v3 0/3] move_phys_pages syscall - migrate page contents given
From: Gregory Price @ 2024-03-20 4:39 UTC (permalink / raw)
To: Huang, Ying
Cc: Gregory Price, linux-mm, linux-api, linux-arch, linux-kselftest,
linux-kernel, dan.j.williams, honggyu.kim, corbet, arnd, luto,
akpm, shuah
In-Reply-To: <87v85hsjn7.fsf@yhuang6-desk2.ccr.corp.intel.com>
On Wed, Mar 20, 2024 at 10:48:44AM +0800, Huang, Ying wrote:
> Gregory Price <gourry.memverge@gmail.com> writes:
>
> > Doing this reverse-translation outside of the kernel requires considerable
> > space and compute, and it will have to be performed again by the existing
> > system calls. Much of this work can be avoided if the pages can be
> > migrated directly with physical memory addressing.
>
> One difficulty of the idea of the physical address is that we lacks some
> user space specified policy information to make decision. For example,
> users may want to pin some pages in DRAM to improve latency, or pin some
> pages in CXL memory to do some best effort work. To make the correct
> decision, we need PID and virtual address.
>
I think of this as a second or third order problem. The core problem
right now isn't the practicality of how userland would actually use this
interface - the core problem is whether the data generated by offloaded
monitoring is even worth collecting and operating on in the first place.
So this is a quick hack to do some research about whether it's even
worth developing the whole abstraction described by Willy.
This is why it's labeled RFC. I upped a v3 because I know of two groups
actively looking at using it for research, and because the folio updates
broke the old version. It's also easier for me to engage through the
list than via private channels for this particular work.
Do I suggest we merge this interface as-is? No, too many concerns about
side channels. However, it's a clean reuse of move_pages code to
bootstrap the investigation, and it at least gets the gears turning.
Example notes from a sidebar earlier today:
* An interesting proposal from Dan Williams would be to provide some
sort of `/sys/.../memory_tiering/tierN/promote_hot` interface, with
a callback mechanism into the relevant hardware drivers that allows
for this to be abstracted. This could be done on some interval and
some threshhold (# pages, hotness threshhold, etc).
The code to execute promotions ends up looking like what I have now
1) Validate the page is elgibile to be promoted by walking the vmas
2) invoking the existing move_pages code
The above idea can be implemented trivially in userland without
having to plumb through a whole brand new callback system.
Sometimes you have to post stupid ideas to get to the good ones :]
~Gregory
^ permalink raw reply
* Re: [RFC v3 0/3] move_phys_pages syscall - migrate page contents given
From: Huang, Ying @ 2024-03-20 2:48 UTC (permalink / raw)
To: Gregory Price
Cc: linux-mm, linux-api, linux-arch, linux-kselftest, linux-kernel,
dan.j.williams, honggyu.kim, corbet, arnd, luto, akpm, shuah,
Gregory Price
In-Reply-To: <20240319172609.332900-1-gregory.price@memverge.com>
Gregory Price <gourry.memverge@gmail.com> writes:
> v3:
> - pull forward to v6.8
> - style and small fixups recommended by jcameron
> - update syscall number (will do all archs when RFC tag drops)
> - update for new folio code
> - added OCP link to device-tracked address hotness proposal
> - kept void* over __u64 simply because it integrates cleanly with
> existing migration code. If there's strong opinions, I can refactor.
>
> This patch set is a proposal for a syscall analogous to move_pages,
> that migrates pages between NUMA nodes using physical addressing.
>
> The intent is to better enable user-land system-wide memory tiering
> as CXL devices begin to provide memory resources on the PCIe bus.
>
> For example, user-land software which is making decisions based on
> data sources which expose physical address information no longer
> must convert that information to virtual addressing to act upon it
> (see background for info on how physical addresses are acquired).
>
> The syscall requires CAP_SYS_ADMIN, since physical address source
> information is typically protected by the same (or CAP_SYS_NICE).
>
> This patch set broken into 3 patches:
> 1) refactor of existing migration code for code reuse
> 2) The sys_move_phys_pages system call.
> 3) ktest of the syscall
>
> The sys_move_phys_pages system call validates the page may be
> migrated by checking migratable-status of each vma mapping the page,
> and the intersection of cpuset policies each vma's task.
>
>
> Background:
>
> Userspace job schedulers, memory managers, and tiering software
> solutions depend on page migration syscalls to reallocate resources
> across NUMA nodes. Currently, these calls enable movement of memory
> associated with a specific PID. Moves can be requested in coarse,
> process-sized strokes (as with migrate_pages), and on specific virtual
> pages (via move_pages).
>
> However, a number of profiling mechanisms provide system-wide information
> that would benefit from a physical-addressing version move_pages.
>
> There are presently at least 4 ways userland can acquire physical
> address information for use with this interface, and 1 hardware offload
> mechanism being proposed by opencompute.
>
> 1) /proc/pid/pagemap: can be used to do page table translations.
> This is only really useful for testing, and the ktest was
> written using this functionality.
>
> 2) X86: IBS (AMD) and PEBS (Intel) can be configured to return physical
> and/or vitual address information.
>
> 3) zoneinfo: /proc/zoneinfo exposes the start PFN of zones
>
> 4) /sys/kernel/mm/page_idle: A way to query whether a PFN is idle.
> So long as the page size is known, this can be used to identify
> system-wide idle pages that could be migrated to lower tiers.
>
> https://docs.kernel.org/admin-guide/mm/idle_page_tracking.html
>
> 5) CXL Offloaded Hotness Monitoring (Proposed): a CXL memory device
> may provide hot/cold information about its memory. For example,
> it may report the hottest device addresses (0-based) or a physical
> address (if it has access to decoders for convert bases).
>
> DPA can be cheaply converted to HPA by combining it with data
> exposed by /sys/bus/cxl/ information (region address bases).
>
> See: https://www.opencompute.org/documents/ocp-cms-hotness-tracking-requirements-white-paper-pdf-1
>
>
> Information from these sources facilitates systemwide resource management,
> but with the limitations of migrate_pages and move_pages applying to
> individual tasks, their outputs must be converted back to virtual addresses
> and re-associated with specific PIDs.
>
> Doing this reverse-translation outside of the kernel requires considerable
> space and compute, and it will have to be performed again by the existing
> system calls. Much of this work can be avoided if the pages can be
> migrated directly with physical memory addressing.
One difficulty of the idea of the physical address is that we lacks some
user space specified policy information to make decision. For example,
users may want to pin some pages in DRAM to improve latency, or pin some
pages in CXL memory to do some best effort work. To make the correct
decision, we need PID and virtual address.
Yes, I found that you have tried to avoid to break the existing policy
in the code. But it seems better to consider the policy beforehand to
avoid to make the wrong decision in the first place.
--
Best Regards,
Huang, Ying
^ permalink raw reply
* Re: [RFC v3 3/3] ktest: sys_move_phys_pages ktest
From: Gregory Price @ 2024-03-19 18:50 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Gregory Price, linux-mm, linux-api, linux-arch, linux-kselftest,
linux-kernel, ying.huang, dan.j.williams, honggyu.kim, corbet,
arnd, luto, akpm, shuah
In-Reply-To: <Zfnbn8H4O9neZhcm@casper.infradead.org>
On Tue, Mar 19, 2024 at 06:38:23PM +0000, Matthew Wilcox wrote:
> > The syscall design is mostly being posted right now to collaborate via
> > public channels, but if the idea is so fundamentally offensive then i'll
> > drop it and relay the opinion accordingly.
>
> The syscall design is wrong. Exposing physical addresses to userspace
> is never the right answer. Think rowhammer.
>
1) The syscall does not expose physical addresses information, it
consumes it.
2) The syscall does not allow the user to select target physical address
only the target node. Now, that said, if source-pages are zeroed on
migration, that's definitely a concern. I did not see this to be the
case, however, and the frequency of write required to make use of
that for rowhammer seems to be a mitigating factor.
3) there exist 4 interfaces which do expose physical address information
- /proc/pid/pagemap
- perf / IBS and PEBs
- zoneinfo
- /sys/kerne/mm/page_idle (PFNs)
4) The syscall requires CAP_SYS_ADMIN because these other sources
require the same, though as v1/v2 discussed there could be an
argument for CAP_SYS_NIDE.
> I'm vehemently opposed to all of the bullshit around CXL. However, if you
> are going to propose something, it should be based around an abstraction.
> Say "We have 8 pools of memory. This VMA is backed by memory from pools
> 3 & 6. The relative hotness of the 8 pools are <vector>. The quantities
> of memory in the 8 ppols are <vector>". And then you can say "migrate
> this range of memory to pool 2".
>
> That's just an initial response to the idea. I refuse to invest a
> serious amount of time in a dead-end idea like CXL memory pooling.
Who said anything about pools? Local memory expanders are capable of
hosting hotness tracking offload.
~Gregory
^ permalink raw reply
* Re: [RFC v3 3/3] ktest: sys_move_phys_pages ktest
From: Matthew Wilcox @ 2024-03-19 18:38 UTC (permalink / raw)
To: Gregory Price
Cc: Gregory Price, linux-mm, linux-api, linux-arch, linux-kselftest,
linux-kernel, ying.huang, dan.j.williams, honggyu.kim, corbet,
arnd, luto, akpm, shuah
In-Reply-To: <ZfnaMa6x/O68ENsP@memverge.com>
On Tue, Mar 19, 2024 at 02:32:17PM -0400, Gregory Price wrote:
> On Tue, Mar 19, 2024 at 06:20:33PM +0000, Matthew Wilcox wrote:
> > On Tue, Mar 19, 2024 at 02:14:33PM -0400, Gregory Price wrote:
> > > On Tue, Mar 19, 2024 at 05:52:46PM +0000, Matthew Wilcox wrote:
> > > > On Tue, Mar 19, 2024 at 01:26:09PM -0400, Gregory Price wrote:
> > > > > Implement simple ktest that looks up the physical address via
> > > > > /proc/self/pagemap and migrates the page based on that information.
> > > >
> > > > What? LOL. No.
> > > >
> > >
> > > Certainly the test is stupid and requires admin, but I could not
> > > come up an easier test to demonstrate the concept - and the docs
> > > say to include a test with all syscall proposals.
> > >
> > > Am I missing something else important?
> > > (stupid question: of course I am, but alas I must ask it)
> >
> > It's not that the test is stupid. It's the concept that's stupid.
>
> Ok i'll bite.
>
> The 2 major ways page-hotness is detected right now is page-faults
> (induced or otherwise) and things like IBS/PEBS.
>
> page-faults cause overhead, and IBS/PEBS actually miss upwards of ~66%
> of all traffic (if you want the details i can dig up the presentation,
> but TL;DR: prefetcher traffic is missed entirely).
>
> so OCP folks have been proposing hotness-tracking offloaded to the
> memory devices themselves:
>
> https://www.opencompute.org/documents/ocp-cms-hotness-tracking-requirements-white-paper-pdf-1
>
> (it's come along further than this white paper, but i need to dig up
> the new information).
>
> These devices are incapable of providing virtual addressing information,
> and doing reverse lookups of addresses is inordinately expensive from
> user space. This leaves: Do it all in a kernel task, or give user space
> an an interface to operate on data provided by the device.
>
> The syscall design is mostly being posted right now to collaborate via
> public channels, but if the idea is so fundamentally offensive then i'll
> drop it and relay the opinion accordingly.
The syscall design is wrong. Exposing physical addresses to userspace
is never the right answer. Think rowhammer.
I'm vehemently opposed to all of the bullshit around CXL. However, if you
are going to propose something, it should be based around an abstraction.
Say "We have 8 pools of memory. This VMA is backed by memory from pools
3 & 6. The relative hotness of the 8 pools are <vector>. The quantities
of memory in the 8 ppols are <vector>". And then you can say "migrate
this range of memory to pool 2".
That's just an initial response to the idea. I refuse to invest a
serious amount of time in a dead-end idea like CXL memory pooling.
^ permalink raw reply
* Re: [RFC v3 3/3] ktest: sys_move_phys_pages ktest
From: Gregory Price @ 2024-03-19 18:32 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Gregory Price, linux-mm, linux-api, linux-arch, linux-kselftest,
linux-kernel, ying.huang, dan.j.williams, honggyu.kim, corbet,
arnd, luto, akpm, shuah
In-Reply-To: <ZfnXcbd3h3Rj4IIS@casper.infradead.org>
On Tue, Mar 19, 2024 at 06:20:33PM +0000, Matthew Wilcox wrote:
> On Tue, Mar 19, 2024 at 02:14:33PM -0400, Gregory Price wrote:
> > On Tue, Mar 19, 2024 at 05:52:46PM +0000, Matthew Wilcox wrote:
> > > On Tue, Mar 19, 2024 at 01:26:09PM -0400, Gregory Price wrote:
> > > > Implement simple ktest that looks up the physical address via
> > > > /proc/self/pagemap and migrates the page based on that information.
> > >
> > > What? LOL. No.
> > >
> >
> > Certainly the test is stupid and requires admin, but I could not
> > come up an easier test to demonstrate the concept - and the docs
> > say to include a test with all syscall proposals.
> >
> > Am I missing something else important?
> > (stupid question: of course I am, but alas I must ask it)
>
> It's not that the test is stupid. It's the concept that's stupid.
Ok i'll bite.
The 2 major ways page-hotness is detected right now is page-faults
(induced or otherwise) and things like IBS/PEBS.
page-faults cause overhead, and IBS/PEBS actually miss upwards of ~66%
of all traffic (if you want the details i can dig up the presentation,
but TL;DR: prefetcher traffic is missed entirely).
so OCP folks have been proposing hotness-tracking offloaded to the
memory devices themselves:
https://www.opencompute.org/documents/ocp-cms-hotness-tracking-requirements-white-paper-pdf-1
(it's come along further than this white paper, but i need to dig up
the new information).
These devices are incapable of providing virtual addressing information,
and doing reverse lookups of addresses is inordinately expensive from
user space. This leaves: Do it all in a kernel task, or give user space
an an interface to operate on data provided by the device.
The syscall design is mostly being posted right now to collaborate via
public channels, but if the idea is so fundamentally offensive then i'll
drop it and relay the opinion accordingly.
~Gregory
^ permalink raw reply
* Re: [RFC v3 3/3] ktest: sys_move_phys_pages ktest
From: Matthew Wilcox @ 2024-03-19 18:20 UTC (permalink / raw)
To: Gregory Price
Cc: Gregory Price, linux-mm, linux-api, linux-arch, linux-kselftest,
linux-kernel, ying.huang, dan.j.williams, honggyu.kim, corbet,
arnd, luto, akpm, shuah
In-Reply-To: <ZfnWCRwcZJ4KBmSH@memverge.com>
On Tue, Mar 19, 2024 at 02:14:33PM -0400, Gregory Price wrote:
> On Tue, Mar 19, 2024 at 05:52:46PM +0000, Matthew Wilcox wrote:
> > On Tue, Mar 19, 2024 at 01:26:09PM -0400, Gregory Price wrote:
> > > Implement simple ktest that looks up the physical address via
> > > /proc/self/pagemap and migrates the page based on that information.
> >
> > What? LOL. No.
> >
>
> Certainly the test is stupid and requires admin, but I could not
> come up an easier test to demonstrate the concept - and the docs
> say to include a test with all syscall proposals.
>
> Am I missing something else important?
> (stupid question: of course I am, but alas I must ask it)
It's not that the test is stupid. It's the concept that's stupid.
^ permalink raw reply
* Re: [RFC v3 3/3] ktest: sys_move_phys_pages ktesty
From: Gregory Price @ 2024-03-19 18:18 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Gregory Price, linux-mm, linux-api, linux-arch, linux-kselftest,
linux-kernel, ying.huang, dan.j.williams, honggyu.kim, corbet,
arnd, luto, akpm, shuah
In-Reply-To: <ZfnWYcFLLVyP2Ng8@memverge.com>
On Tue, Mar 19, 2024 at 02:16:01PM -0400, Gregory Price wrote:
> On Tue, Mar 19, 2024 at 06:08:39PM +0000, Matthew Wilcox wrote:
> > On Tue, Mar 19, 2024 at 05:52:46PM +0000, Matthew Wilcox wrote:
> > > On Tue, Mar 19, 2024 at 01:26:09PM -0400, Gregory Price wrote:
> > > > Implement simple ktest that looks up the physical address via
> > > > /proc/self/pagemap and migrates the page based on that information.
> > >
> > > What? LOL. No.
> >
> > Also, how is this v3 and the first one to land on linux-mm?
> >
> > https://lore.kernel.org/linux-mm/?q=move_phys_pages
> >
> > Also, where is the syscall itself? The only thing here is the ktest.
> >
>
> OH, I see the confusion now.
>
> There were two other versions, and I have experienced this delivery
> failure before, i'm not sure why the other commits have not been delivered.
>
> Let me look into this.
>
> ~Gregory
>
Full set of patches:
https://lore.kernel.org/all/20240319172609.332900-1-gregory.price@memverge.com/
I've experienced silent linux-mm delivery failures like this before, I
still do not understand this issue.
v1:
https://lore.kernel.org/all/20230907075453.350554-1-gregory.price@memverge.com/
v2:
https://lore.kernel.org/all/20230919230909.530174-1-gregory.price@memverge.com/
~Gregory
^ permalink raw reply
* Re: [RFC v3 3/3] ktest: sys_move_phys_pages ktesty
From: Gregory Price @ 2024-03-19 18:16 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Gregory Price, linux-mm, linux-api, linux-arch, linux-kselftest,
linux-kernel, ying.huang, dan.j.williams, honggyu.kim, corbet,
arnd, luto, akpm, shuah
In-Reply-To: <ZfnUp7OAr6PFGAq_@casper.infradead.org>
On Tue, Mar 19, 2024 at 06:08:39PM +0000, Matthew Wilcox wrote:
> On Tue, Mar 19, 2024 at 05:52:46PM +0000, Matthew Wilcox wrote:
> > On Tue, Mar 19, 2024 at 01:26:09PM -0400, Gregory Price wrote:
> > > Implement simple ktest that looks up the physical address via
> > > /proc/self/pagemap and migrates the page based on that information.
> >
> > What? LOL. No.
>
> Also, how is this v3 and the first one to land on linux-mm?
>
> https://lore.kernel.org/linux-mm/?q=move_phys_pages
>
> Also, where is the syscall itself? The only thing here is the ktest.
>
OH, I see the confusion now.
There were two other versions, and I have experienced this delivery
failure before, i'm not sure why the other commits have not been delivered.
Let me look into this.
~Gregory
^ permalink raw reply
* Re: [RFC v3 3/3] ktest: sys_move_phys_pages ktest
From: Gregory Price @ 2024-03-19 18:14 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Gregory Price, linux-mm, linux-api, linux-arch, linux-kselftest,
linux-kernel, ying.huang, dan.j.williams, honggyu.kim, corbet,
arnd, luto, akpm, shuah
In-Reply-To: <ZfnQ7n_7cZvk9BkC@casper.infradead.org>
On Tue, Mar 19, 2024 at 05:52:46PM +0000, Matthew Wilcox wrote:
> On Tue, Mar 19, 2024 at 01:26:09PM -0400, Gregory Price wrote:
> > Implement simple ktest that looks up the physical address via
> > /proc/self/pagemap and migrates the page based on that information.
>
> What? LOL. No.
>
Certainly the test is stupid and requires admin, but I could not
come up an easier test to demonstrate the concept - and the docs
say to include a test with all syscall proposals.
Am I missing something else important?
(stupid question: of course I am, but alas I must ask it)
~Gregory
^ permalink raw reply
* Re: [RFC v3 3/3] ktest: sys_move_phys_pages ktest
From: Matthew Wilcox @ 2024-03-19 18:08 UTC (permalink / raw)
To: Gregory Price
Cc: linux-mm, linux-api, linux-arch, linux-kselftest, linux-kernel,
ying.huang, dan.j.williams, honggyu.kim, corbet, arnd, luto, akpm,
shuah, Gregory Price
In-Reply-To: <ZfnQ7n_7cZvk9BkC@casper.infradead.org>
On Tue, Mar 19, 2024 at 05:52:46PM +0000, Matthew Wilcox wrote:
> On Tue, Mar 19, 2024 at 01:26:09PM -0400, Gregory Price wrote:
> > Implement simple ktest that looks up the physical address via
> > /proc/self/pagemap and migrates the page based on that information.
>
> What? LOL. No.
Also, how is this v3 and the first one to land on linux-mm?
https://lore.kernel.org/linux-mm/?q=move_phys_pages
Also, where is the syscall itself? The only thing here is the ktest.
^ permalink raw reply
* Re: [RFC v3 3/3] ktest: sys_move_phys_pages ktest
From: Matthew Wilcox @ 2024-03-19 17:52 UTC (permalink / raw)
To: Gregory Price
Cc: linux-mm, linux-api, linux-arch, linux-kselftest, linux-kernel,
ying.huang, dan.j.williams, honggyu.kim, corbet, arnd, luto, akpm,
shuah, Gregory Price
In-Reply-To: <20240319172609.332900-4-gregory.price@memverge.com>
On Tue, Mar 19, 2024 at 01:26:09PM -0400, Gregory Price wrote:
> Implement simple ktest that looks up the physical address via
> /proc/self/pagemap and migrates the page based on that information.
What? LOL. No.
^ permalink raw reply
* [RFC v3 3/3] ktest: sys_move_phys_pages ktest
From: Gregory Price @ 2024-03-19 17:26 UTC (permalink / raw)
To: linux-mm
Cc: linux-api, linux-arch, linux-kselftest, linux-kernel, ying.huang,
dan.j.williams, honggyu.kim, corbet, arnd, luto, akpm, shuah,
Gregory Price
In-Reply-To: <20240319172609.332900-1-gregory.price@memverge.com>
Implement simple ktest that looks up the physical address via
/proc/self/pagemap and migrates the page based on that information.
Signed-off-by: Gregory Price <gregory.price@memverge.com>
---
tools/testing/selftests/mm/migration.c | 99 ++++++++++++++++++++++++++
1 file changed, 99 insertions(+)
diff --git a/tools/testing/selftests/mm/migration.c b/tools/testing/selftests/mm/migration.c
index 6908569ef406..c005c98dbdc1 100644
--- a/tools/testing/selftests/mm/migration.c
+++ b/tools/testing/selftests/mm/migration.c
@@ -5,6 +5,8 @@
*/
#include "../kselftest_harness.h"
+#include <stdint.h>
+#include <stdio.h>
#include <strings.h>
#include <pthread.h>
#include <numa.h>
@@ -14,11 +16,17 @@
#include <sys/types.h>
#include <signal.h>
#include <time.h>
+#include <unistd.h>
#define TWOMEG (2<<20)
#define RUNTIME (20)
+#define GET_BIT(X, Y) ((X & ((uint64_t)1<<Y)) >> Y)
+#define GET_PFN(X) (X & 0x7FFFFFFFFFFFFFull)
#define ALIGN(x, a) (((x) + (a - 1)) & (~((a) - 1)))
+#define PAGEMAP_ENTRY 8
+const int __endian_bit = 1;
+#define is_bigendian() ((*(char *)&__endian_bit) == 0)
FIXTURE(migration)
{
@@ -94,6 +102,45 @@ int migrate(uint64_t *ptr, int n1, int n2)
return 0;
}
+int migrate_phys(uint64_t paddr, int n1, int n2)
+{
+ int ret, tmp;
+ int status = 0;
+ struct timespec ts1, ts2;
+
+ if (clock_gettime(CLOCK_MONOTONIC, &ts1))
+ return -1;
+
+ while (1) {
+ if (clock_gettime(CLOCK_MONOTONIC, &ts2))
+ return -1;
+
+ if (ts2.tv_sec - ts1.tv_sec >= RUNTIME)
+ return 0;
+
+ /*
+ * FIXME: move_phys_pages was syscall 462 during RFC.
+ * Update this when an official syscall number is adopted
+ * and the libnuma interface is implemented.
+ */
+ ret = syscall(462, 1, (void **) &paddr, &n2, &status,
+ MPOL_MF_MOVE_ALL);
+ if (ret) {
+ if (ret > 0)
+ printf("Didn't migrate %d pages\n", ret);
+ else
+ perror("Couldn't migrate pages");
+ return -2;
+ }
+
+ tmp = n2;
+ n2 = n1;
+ n1 = tmp;
+ }
+
+ return 0;
+}
+
void *access_mem(void *ptr)
{
volatile uint64_t y = 0;
@@ -199,4 +246,56 @@ TEST_F_TIMEOUT(migration, private_anon_thp, 2*RUNTIME)
ASSERT_EQ(pthread_cancel(self->threads[i]), 0);
}
+/*
+ * Same as the basic migration, but test move_phys_pages.
+ */
+TEST_F_TIMEOUT(migration, phys_addr, 2*RUNTIME)
+{
+ uint64_t *ptr;
+ uint64_t pagemap_val, paddr, file_offset;
+ unsigned char c_buf[PAGEMAP_ENTRY];
+ int i, c, status;
+ FILE *f;
+
+ if (self->nthreads < 2 || self->n1 < 0 || self->n2 < 0)
+ SKIP(return, "Not enough threads or NUMA nodes available");
+
+ ptr = mmap(NULL, TWOMEG, PROT_READ | PROT_WRITE,
+ MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+ ASSERT_NE(ptr, MAP_FAILED);
+
+ memset(ptr, 0xde, TWOMEG);
+
+ /* PFN of ptr from /proc/self/pagemap */
+ f = fopen("/proc/self/pagemap", "rb");
+ file_offset = ((uint64_t)ptr) / getpagesize() * PAGEMAP_ENTRY;
+ status = fseek(f, file_offset, SEEK_SET);
+ ASSERT_EQ(status, 0);
+ for (i = 0; i < PAGEMAP_ENTRY; i++) {
+ c = getc(f);
+ ASSERT_NE(c, EOF);
+ /* handle endiand differences */
+ if (is_bigendian())
+ c_buf[i] = c;
+ else
+ c_buf[PAGEMAP_ENTRY - i - 1] = c;
+ }
+ fclose(f);
+
+ for (i = 0; i < PAGEMAP_ENTRY; i++)
+ pagemap_val = (pagemap_val << 8) + c_buf[i];
+
+ ASSERT_TRUE(GET_BIT(pagemap_val, 63));
+ /* This reports a pfn, we need to shift this by page size */
+ paddr = GET_PFN(pagemap_val) << __builtin_ctz(getpagesize());
+
+ for (i = 0; i < self->nthreads - 1; i++)
+ if (pthread_create(&self->threads[i], NULL, access_mem, ptr))
+ perror("Couldn't create thread");
+
+ ASSERT_EQ(migrate_phys(paddr, self->n1, self->n2), 0);
+ for (i = 0; i < self->nthreads - 1; i++)
+ ASSERT_EQ(pthread_cancel(self->threads[i]), 0);
+}
+
TEST_HARNESS_MAIN
--
2.39.1
^ permalink raw reply related
* [RFC v3 2/3] mm/migrate: Create move_phys_pages syscall
From: Gregory Price @ 2024-03-19 17:26 UTC (permalink / raw)
To: linux-mm
Cc: linux-api, linux-arch, linux-kselftest, linux-kernel, ying.huang,
dan.j.williams, honggyu.kim, corbet, arnd, luto, akpm, shuah,
Gregory Price
In-Reply-To: <20240319172609.332900-1-gregory.price@memverge.com>
Similar to the move_pages system call, instead of taking a pid and
list of virtual addresses, this system call takes a list of physical
addresses.
Because there is no task to validate the memory policy against, each
page needs to be interrogated to determine whether the migration is
valid, and all tasks that map it need to be interrogated.
This is accomplished in via a rmap_walk on the folio containing
the page, and an interrogation of all tasks that map the page (by
way of each task's vma).
Each page must be interrogated individually, which should be
considered when using this to migrate shared regions.
The remaining logic is the same as the move_pages syscall. One
change to do_pages_move is made (to check whether an mm_struct is
passed) in order to re-use the existing migration code.
Signed-off-by: Gregory Price <gregory.price@memverge.com>
---
arch/x86/entry/syscalls/syscall_32.tbl | 1 +
arch/x86/entry/syscalls/syscall_64.tbl | 1 +
include/linux/syscalls.h | 5 +
include/uapi/asm-generic/unistd.h | 8 +-
kernel/sys_ni.c | 1 +
mm/migrate.c | 206 +++++++++++++++++++++++-
tools/include/uapi/asm-generic/unistd.h | 8 +-
7 files changed, 222 insertions(+), 8 deletions(-)
diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
index 5f8591ce7f25..250c00281029 100644
--- a/arch/x86/entry/syscalls/syscall_32.tbl
+++ b/arch/x86/entry/syscalls/syscall_32.tbl
@@ -466,3 +466,4 @@
459 i386 lsm_get_self_attr sys_lsm_get_self_attr
460 i386 lsm_set_self_attr sys_lsm_set_self_attr
461 i386 lsm_list_modules sys_lsm_list_modules
+462 i386 move_phys_pages sys_move_phys_pages
diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
index 7e8d46f4147f..a928df7c6f52 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -383,6 +383,7 @@
459 common lsm_get_self_attr sys_lsm_get_self_attr
460 common lsm_set_self_attr sys_lsm_set_self_attr
461 common lsm_list_modules sys_lsm_list_modules
+462 common move_phys_pages sys_move_phys_pages
#
# Due to a historical design error, certain syscalls are numbered differently
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 77eb9b0e7685..575ba9d26e30 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -840,6 +840,11 @@ asmlinkage long sys_move_pages(pid_t pid, unsigned long nr_pages,
const int __user *nodes,
int __user *status,
int flags);
+asmlinkage long sys_move_phys_pages(unsigned long nr_pages,
+ const void __user * __user *pages,
+ const int __user *nodes,
+ int __user *status,
+ int flags);
asmlinkage long sys_rt_tgsigqueueinfo(pid_t tgid, pid_t pid, int sig,
siginfo_t __user *uinfo);
asmlinkage long sys_perf_event_open(
diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
index 75f00965ab15..13bc8dd16d6b 100644
--- a/include/uapi/asm-generic/unistd.h
+++ b/include/uapi/asm-generic/unistd.h
@@ -842,8 +842,14 @@ __SYSCALL(__NR_lsm_set_self_attr, sys_lsm_set_self_attr)
#define __NR_lsm_list_modules 461
__SYSCALL(__NR_lsm_list_modules, sys_lsm_list_modules)
+/* CONFIG_MMU only */
+#ifndef __ARCH_NOMMU
+#define __NR_move_phys_pages 462
+__SYSCALL(__NR_move_phys_pages, sys_move_phys_pages)
+#endif
+
#undef __NR_syscalls
-#define __NR_syscalls 462
+#define __NR_syscalls 463
/*
* 32 bit systems traditionally used different
diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
index faad00cce269..254915fd1e2c 100644
--- a/kernel/sys_ni.c
+++ b/kernel/sys_ni.c
@@ -196,6 +196,7 @@ COND_SYSCALL(migrate_pages);
COND_SYSCALL(move_pages);
COND_SYSCALL(set_mempolicy_home_node);
COND_SYSCALL(cachestat);
+COND_SYSCALL(move_phys_pages);
COND_SYSCALL(perf_event_open);
COND_SYSCALL(accept4);
diff --git a/mm/migrate.c b/mm/migrate.c
index 27071a07ffbb..7213703441f8 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -2182,9 +2182,119 @@ static int move_pages_and_store_status(int node,
return store_status(status, start, node, i - start);
}
+struct rmap_page_ctxt {
+ bool found;
+ bool migratable;
+ bool node_allowed;
+ int node;
+};
+
+/*
+ * Walks each vma mapping a given page and determines if those
+ * vma's are both migratable, and that the target node is within
+ * the allowed cpuset of the owning task.
+ */
+static bool phys_page_migratable(struct folio *folio,
+ struct vm_area_struct *vma,
+ unsigned long address,
+ void *arg)
+{
+ struct rmap_page_ctxt *ctxt = arg;
+#ifdef CONFIG_MEMCG
+ struct task_struct *owner = vma->vm_mm->owner;
+ nodemask_t task_nodes = cpuset_mems_allowed(owner);
+#else
+ nodemask_t task_nodes = node_possible_map;
+#endif
+
+ ctxt->found = true;
+ ctxt->migratable &= vma_migratable(vma);
+ ctxt->node_allowed &= node_isset(ctxt->node, task_nodes);
+
+ return ctxt->migratable && ctxt->node_allowed;
+}
+
+static struct folio *phys_migrate_get_folio(struct page *page)
+{
+ struct folio *folio;
+
+ folio = page_folio(page);
+ if (!folio_test_lru(folio) || !folio_try_get(folio))
+ return NULL;
+ if (unlikely(page_folio(page) != folio || !folio_test_lru(folio))) {
+ folio_put(folio);
+ folio = NULL;
+ }
+ return folio;
+}
+
+/*
+ * Validates the physical address is online and migratable. Walks the folio
+ * containing the page to validate the vma is migratable and the cpuset node
+ * restrictions. Then calls add_page_for_migration to isolate it from the
+ * LRU and place it into the given pagelist.
+ * Returns:
+ * errno - if the page is not online, migratable, or can't be isolated
+ * 0 - when it doesn't have to be migrated because it is already on the
+ * target node
+ * 1 - when it has been queued
+ */
+static int add_phys_page_for_migration(const void __user *p, int node,
+ struct list_head *pagelist,
+ bool migrate_all)
+{
+ unsigned long pfn;
+ struct page *page;
+ struct folio *folio;
+ int err;
+ struct rmap_page_ctxt rmctxt = {
+ .found = false,
+ .migratable = true,
+ .node_allowed = true,
+ .node = node
+ };
+ struct rmap_walk_control rwc = {
+ .rmap_one = phys_page_migratable,
+ .arg = &rmctxt
+ };
+
+ pfn = ((unsigned long)p) >> PAGE_SHIFT;
+ page = pfn_to_online_page(pfn);
+ if (!page || PageTail(page))
+ return -ENOENT;
+
+ folio = phys_migrate_get_folio(page);
+ if (!folio)
+ return -ENOENT;
+
+ rmap_walk(folio, &rwc);
+
+ if (!rmctxt.found)
+ err = -ENOENT;
+ else if (!rmctxt.migratable)
+ err = -EFAULT;
+ else if (!rmctxt.node_allowed)
+ err = -EACCES;
+ else
+ err = add_page_for_migration(page, folio, node, pagelist,
+ migrate_all);
+
+ folio_put(folio);
+
+ return err;
+}
+
/*
* Migrate an array of page address onto an array of nodes and fill
* the corresponding array of status.
+ *
+ * When the mm argument is not NULL, task_nodes is expected to be the
+ * cpuset nodemask for the task which owns the mm_struct, and the
+ * values located in (*pages) are expected to be virtual addresses.
+ *
+ * When the mm argument is NULL, the values located at (*pages) are
+ * expected to be physical addresses, and task_nodes is expected to
+ * be empty.
*/
static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
unsigned long nr_pages,
@@ -2226,7 +2336,14 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
goto out_flush;
err = -EACCES;
- if (!node_isset(node, task_nodes))
+ /*
+ * if mm is NULL, then the pages are addressed via physical
+ * address and the task_nodes structure is empty. Validation
+ * of migratability is deferred to add_phys_page_for_migration
+ * where vma's that map the address will have their node_mask
+ * checked to ensure the requested node bit is set.
+ */
+ if (mm && !node_isset(node, task_nodes))
goto out_flush;
if (current_node == NUMA_NO_NODE) {
@@ -2243,10 +2360,17 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
/*
* Errors in the page lookup or isolation are not fatal and we simply
- * report them via status
+ * report them via status.
+ *
+ * If mm is NULL, then p treated as is a physical address.
*/
- err = add_virt_page_for_migration(mm, p, current_node, &pagelist,
- flags & MPOL_MF_MOVE_ALL);
+ if (mm)
+ err = add_virt_page_for_migration(mm, p, current_node, &pagelist,
+ flags & MPOL_MF_MOVE_ALL);
+ else
+ err = add_phys_page_for_migration(p, current_node, &pagelist,
+ flags & MPOL_MF_MOVE_ALL);
+
if (err > 0) {
/* The page is successfully queued for migration */
@@ -2334,6 +2458,37 @@ static void do_pages_stat_array(struct mm_struct *mm, unsigned long nr_pages,
mmap_read_unlock(mm);
}
+/*
+ * Determine the nodes pages pointed to by the physical addresses in the
+ * pages array, and store those node values in the status array
+ */
+static void do_phys_pages_stat_array(unsigned long nr_pages,
+ const void __user **pages, int *status)
+{
+ unsigned long i;
+
+ for (i = 0; i < nr_pages; i++) {
+ unsigned long pfn = (unsigned long)(*pages) >> PAGE_SHIFT;
+ struct page *page = pfn_to_online_page(pfn);
+ int err = -ENOENT;
+
+ if (!page)
+ goto set_status;
+
+ get_page(page);
+
+ if (!is_zone_device_page(page))
+ err = page_to_nid(page);
+
+ put_page(page);
+set_status:
+ *status = err;
+
+ pages++;
+ status++;
+ }
+}
+
static int get_compat_pages_array(const void __user *chunk_pages[],
const void __user * __user *pages,
unsigned long chunk_nr)
@@ -2376,7 +2531,10 @@ static int do_pages_stat(struct mm_struct *mm, unsigned long nr_pages,
break;
}
- do_pages_stat_array(mm, chunk_nr, chunk_pages, chunk_status);
+ if (mm)
+ do_pages_stat_array(mm, chunk_nr, chunk_pages, chunk_status);
+ else
+ do_phys_pages_stat_array(chunk_nr, chunk_pages, chunk_status);
if (copy_to_user(status, chunk_status, chunk_nr * sizeof(*status)))
break;
@@ -2449,7 +2607,7 @@ static int kernel_move_pages(pid_t pid, unsigned long nr_pages,
nodemask_t task_nodes;
/* Check flags */
- if (flags & ~(MPOL_MF_MOVE|MPOL_MF_MOVE_ALL))
+ if (flags & ~(MPOL_MF_MOVE | MPOL_MF_MOVE_ALL))
return -EINVAL;
if ((flags & MPOL_MF_MOVE_ALL) && !capable(CAP_SYS_NICE))
@@ -2477,6 +2635,42 @@ SYSCALL_DEFINE6(move_pages, pid_t, pid, unsigned long, nr_pages,
return kernel_move_pages(pid, nr_pages, pages, nodes, status, flags);
}
+/*
+ * Move a list of physically-addressed pages to the list of target nodes
+ */
+static int kernel_move_phys_pages(unsigned long nr_pages,
+ const void __user * __user *pages,
+ const int __user *nodes,
+ int __user *status, int flags)
+{
+ nodemask_t dummy_nodes;
+
+ if (flags & ~(MPOL_MF_MOVE|MPOL_MF_MOVE_ALL))
+ return -EINVAL;
+
+ if (!capable(CAP_SYS_ADMIN))
+ return -EPERM;
+
+ if (!nodes)
+ return do_pages_stat(NULL, nr_pages, pages, status);
+
+ /*
+ * When the mm argument to do_pages_move is null, the task_nodes
+ * argument is ignored, so pass in an empty nodemask as a dummy.
+ */
+ nodes_clear(dummy_nodes);
+ return do_pages_move(NULL, dummy_nodes, nr_pages, pages, nodes, status,
+ flags);
+}
+
+SYSCALL_DEFINE5(move_phys_pages, unsigned long, nr_pages,
+ const void __user * __user *, pages,
+ const int __user *, nodes,
+ int __user *, status, int, flags)
+{
+ return kernel_move_phys_pages(nr_pages, pages, nodes, status, flags);
+}
+
#ifdef CONFIG_NUMA_BALANCING
/*
* Returns true if this is a safe migration target node for misplaced NUMA
diff --git a/tools/include/uapi/asm-generic/unistd.h b/tools/include/uapi/asm-generic/unistd.h
index 75f00965ab15..13bc8dd16d6b 100644
--- a/tools/include/uapi/asm-generic/unistd.h
+++ b/tools/include/uapi/asm-generic/unistd.h
@@ -842,8 +842,14 @@ __SYSCALL(__NR_lsm_set_self_attr, sys_lsm_set_self_attr)
#define __NR_lsm_list_modules 461
__SYSCALL(__NR_lsm_list_modules, sys_lsm_list_modules)
+/* CONFIG_MMU only */
+#ifndef __ARCH_NOMMU
+#define __NR_move_phys_pages 462
+__SYSCALL(__NR_move_phys_pages, sys_move_phys_pages)
+#endif
+
#undef __NR_syscalls
-#define __NR_syscalls 462
+#define __NR_syscalls 463
/*
* 32 bit systems traditionally used different
--
2.39.1
^ permalink raw reply related
* [RFC v3 1/3] mm/migrate: refactor add_page_for_migration for code re-use
From: Gregory Price @ 2024-03-19 17:26 UTC (permalink / raw)
To: linux-mm
Cc: linux-api, linux-arch, linux-kselftest, linux-kernel, ying.huang,
dan.j.williams, honggyu.kim, corbet, arnd, luto, akpm, shuah,
Gregory Price
In-Reply-To: <20240319172609.332900-1-gregory.price@memverge.com>
add_page_for_migration presently does two actions:
1) validates the page is present and migratable
2) isolates the page from LRU and puts it into the migration list
Break add_page_for_migration into 2 functions:
add_page_for_migration - isolate the page from LUR and add to list
add_virt_page_for_migration - validate the page and call the above
add_page_for_migration does not require the mm_struct and so can be
re-used for a physical addressing version of move_pages
Signed-off-by: Gregory Price <gregory.price@memverge.com>
---
mm/migrate.c | 84 +++++++++++++++++++++++++++++++---------------------
1 file changed, 50 insertions(+), 34 deletions(-)
diff --git a/mm/migrate.c b/mm/migrate.c
index c27b1f8097d4..27071a07ffbb 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -2066,6 +2066,46 @@ static int do_move_pages_to_node(struct list_head *pagelist, int node)
return err;
}
+/*
+ * Isolates the page from the LRU and puts it into the given pagelist
+ * Returns:
+ * errno - if the page cannot be isolated
+ * 0 - when it doesn't have to be migrated because it is already on the
+ * target node
+ * 1 - when it has been queued
+ */
+static int add_page_for_migration(struct page *page,
+ struct folio *folio,
+ int node,
+ struct list_head *pagelist,
+ bool migrate_all)
+{
+ if (folio_is_zone_device(folio))
+ return -ENOENT;
+
+ if (folio_nid(folio) == node)
+ return 0;
+
+ if (page_mapcount(page) > 1 && !migrate_all)
+ return -EACCES;
+
+ if (folio_test_hugetlb(folio)) {
+ if (isolate_hugetlb(folio, pagelist))
+ return 1;
+ return -EBUSY;
+ }
+
+ if (!folio_isolate_lru(folio))
+ return -EBUSY;
+
+ list_add_tail(&folio->lru, pagelist);
+ node_stat_mod_folio(folio,
+ NR_ISOLATED_ANON + folio_is_file_lru(folio),
+ folio_nr_pages(folio));
+
+ return 1;
+}
+
/*
* Resolves the given address to a struct page, isolates it from the LRU and
* puts it to the given pagelist.
@@ -2075,19 +2115,19 @@ static int do_move_pages_to_node(struct list_head *pagelist, int node)
* target node
* 1 - when it has been queued
*/
-static int add_page_for_migration(struct mm_struct *mm, const void __user *p,
- int node, struct list_head *pagelist, bool migrate_all)
+static int add_virt_page_for_migration(struct mm_struct *mm,
+ const void __user *p, int node, struct list_head *pagelist,
+ bool migrate_all)
{
struct vm_area_struct *vma;
unsigned long addr;
struct page *page;
struct folio *folio;
- int err;
+ int err = -EFAULT;
mmap_read_lock(mm);
addr = (unsigned long)untagged_addr_remote(mm, p);
- err = -EFAULT;
vma = vma_lookup(mm, addr);
if (!vma || !vma_migratable(vma))
goto out;
@@ -2095,41 +2135,17 @@ static int add_page_for_migration(struct mm_struct *mm, const void __user *p,
/* FOLL_DUMP to ignore special (like zero) pages */
page = follow_page(vma, addr, FOLL_GET | FOLL_DUMP);
- err = PTR_ERR(page);
- if (IS_ERR(page))
- goto out;
-
err = -ENOENT;
if (!page)
goto out;
- folio = page_folio(page);
- if (folio_is_zone_device(folio))
- goto out_putfolio;
-
- err = 0;
- if (folio_nid(folio) == node)
- goto out_putfolio;
+ err = PTR_ERR(page);
+ if (IS_ERR(page))
+ goto out;
- err = -EACCES;
- if (page_mapcount(page) > 1 && !migrate_all)
- goto out_putfolio;
+ folio = page_folio(page);
+ err = add_page_for_migration(page, folio, node, pagelist, migrate_all);
- err = -EBUSY;
- if (folio_test_hugetlb(folio)) {
- if (isolate_hugetlb(folio, pagelist))
- err = 1;
- } else {
- if (!folio_isolate_lru(folio))
- goto out_putfolio;
-
- err = 1;
- list_add_tail(&folio->lru, pagelist);
- node_stat_mod_folio(folio,
- NR_ISOLATED_ANON + folio_is_file_lru(folio),
- folio_nr_pages(folio));
- }
-out_putfolio:
/*
* Either remove the duplicate refcount from folio_isolate_lru()
* or drop the folio ref if it was not isolated.
@@ -2229,7 +2245,7 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
* Errors in the page lookup or isolation are not fatal and we simply
* report them via status
*/
- err = add_page_for_migration(mm, p, current_node, &pagelist,
+ err = add_virt_page_for_migration(mm, p, current_node, &pagelist,
flags & MPOL_MF_MOVE_ALL);
if (err > 0) {
--
2.39.1
^ permalink raw reply related
* [RFC v3 0/3] move_phys_pages syscall - migrate page contents given
From: Gregory Price @ 2024-03-19 17:26 UTC (permalink / raw)
To: linux-mm
Cc: linux-api, linux-arch, linux-kselftest, linux-kernel, ying.huang,
dan.j.williams, honggyu.kim, corbet, arnd, luto, akpm, shuah,
Gregory Price
v3:
- pull forward to v6.8
- style and small fixups recommended by jcameron
- update syscall number (will do all archs when RFC tag drops)
- update for new folio code
- added OCP link to device-tracked address hotness proposal
- kept void* over __u64 simply because it integrates cleanly with
existing migration code. If there's strong opinions, I can refactor.
This patch set is a proposal for a syscall analogous to move_pages,
that migrates pages between NUMA nodes using physical addressing.
The intent is to better enable user-land system-wide memory tiering
as CXL devices begin to provide memory resources on the PCIe bus.
For example, user-land software which is making decisions based on
data sources which expose physical address information no longer
must convert that information to virtual addressing to act upon it
(see background for info on how physical addresses are acquired).
The syscall requires CAP_SYS_ADMIN, since physical address source
information is typically protected by the same (or CAP_SYS_NICE).
This patch set broken into 3 patches:
1) refactor of existing migration code for code reuse
2) The sys_move_phys_pages system call.
3) ktest of the syscall
The sys_move_phys_pages system call validates the page may be
migrated by checking migratable-status of each vma mapping the page,
and the intersection of cpuset policies each vma's task.
Background:
Userspace job schedulers, memory managers, and tiering software
solutions depend on page migration syscalls to reallocate resources
across NUMA nodes. Currently, these calls enable movement of memory
associated with a specific PID. Moves can be requested in coarse,
process-sized strokes (as with migrate_pages), and on specific virtual
pages (via move_pages).
However, a number of profiling mechanisms provide system-wide information
that would benefit from a physical-addressing version move_pages.
There are presently at least 4 ways userland can acquire physical
address information for use with this interface, and 1 hardware offload
mechanism being proposed by opencompute.
1) /proc/pid/pagemap: can be used to do page table translations.
This is only really useful for testing, and the ktest was
written using this functionality.
2) X86: IBS (AMD) and PEBS (Intel) can be configured to return physical
and/or vitual address information.
3) zoneinfo: /proc/zoneinfo exposes the start PFN of zones
4) /sys/kernel/mm/page_idle: A way to query whether a PFN is idle.
So long as the page size is known, this can be used to identify
system-wide idle pages that could be migrated to lower tiers.
https://docs.kernel.org/admin-guide/mm/idle_page_tracking.html
5) CXL Offloaded Hotness Monitoring (Proposed): a CXL memory device
may provide hot/cold information about its memory. For example,
it may report the hottest device addresses (0-based) or a physical
address (if it has access to decoders for convert bases).
DPA can be cheaply converted to HPA by combining it with data
exposed by /sys/bus/cxl/ information (region address bases).
See: https://www.opencompute.org/documents/ocp-cms-hotness-tracking-requirements-white-paper-pdf-1
Information from these sources facilitates systemwide resource management,
but with the limitations of migrate_pages and move_pages applying to
individual tasks, their outputs must be converted back to virtual addresses
and re-associated with specific PIDs.
Doing this reverse-translation outside of the kernel requires considerable
space and compute, and it will have to be performed again by the existing
system calls. Much of this work can be avoided if the pages can be
migrated directly with physical memory addressing.
Gregory Price (3):
mm/migrate: refactor add_page_for_migration for code re-use
mm/migrate: Create move_phys_pages syscall
ktest: sys_move_phys_pages ktest
arch/x86/entry/syscalls/syscall_32.tbl | 1 +
arch/x86/entry/syscalls/syscall_64.tbl | 1 +
include/linux/syscalls.h | 5 +
include/uapi/asm-generic/unistd.h | 8 +-
kernel/sys_ni.c | 1 +
mm/migrate.c | 288 ++++++++++++++++++++----
tools/include/uapi/asm-generic/unistd.h | 8 +-
tools/testing/selftests/mm/migration.c | 99 ++++++++
8 files changed, 370 insertions(+), 41 deletions(-)
--
2.39.1
^ permalink raw reply
* Process synchronize with CAS
From: qiqi tang @ 2024-03-18 14:17 UTC (permalink / raw)
To: linux-api
Hi, everyone
I am interested in the interface of linux. As we all know, there
is CAS way to accomplish the lockless ring buffer with the kernel .
The kernel api can realize the datastruct Such as
__sync_fetch_and_add type __sync_fetch_and_sub (type * ptr, type
value, ...) and so on
But I find the limitation that this CAS is work good for the thread
model rather than Process model . I want to create a shared Memory
and use mmap API to mount this shared memory into producter and
consumer process. What should I do to realize the synchronization
using the CAS algorithm . Are there have any API can over cross
Multi-Process just like the thread model? I would appreciate that
someone can provide the information for this email. Thank you!
Regards,
Jack
^ permalink raw reply
* Re: [PATCH v3 2/2] VT: Allow to get max font width and height
From: Helge Deller @ 2024-03-15 9:16 UTC (permalink / raw)
To: legion, Greg Kroah-Hartman, Jiri Slaby
Cc: LKML, kbd, linux-api, linux-fbdev, linux-serial
In-Reply-To: <78fcb9ad77b88edee8768806ce6a4d23f6d33118.1710252966.git.legion@kernel.org>
On 3/12/24 15:23, legion@kernel.org wrote:
> From: Alexey Gladkov <legion@kernel.org>
>
> The Console drivers has more restrictive font size limits than vt_ioctl.
> This leads to errors that are difficult to handle. If a font whose size
> is not supported is used, an EINVAL error will be returned, which is
> also returned in case of errors in the font itself. At the moment there
> is no way to understand what font sizes the current console driver
> supports.
>
> To solve this problem, we need to transfer information about the
> supported font to userspace from the console driver.
>
> Signed-off-by: Alexey Gladkov <legion@kernel.org>
Acked-by: Helge Deller <deller@gmx.de>
Helge
> ---
> drivers/video/console/newport_con.c | 21 +++++++++++++++++----
> drivers/video/console/sticon.c | 25 +++++++++++++++++++++++--
> drivers/video/console/vgacon.c | 21 ++++++++++++++++++++-
> drivers/video/fbdev/core/fbcon.c | 22 +++++++++++++++++++++-
> 4 files changed, 81 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/video/console/newport_con.c b/drivers/video/console/newport_con.c
> index e8e4f82cd4a1..87f174a95fa8 100644
> --- a/drivers/video/console/newport_con.c
> +++ b/drivers/video/console/newport_con.c
> @@ -33,6 +33,9 @@
>
> #define NEWPORT_LEN 0x10000
>
> +#define NEWPORT_MAX_FONT_WIDTH 8
> +#define NEWPORT_MAX_FONT_HEIGHT 16
> +
> #define FONT_DATA ((unsigned char *)font_vga_8x16.data)
>
> static unsigned char *font_data[MAX_NR_CONSOLES];
> @@ -328,8 +331,8 @@ static void newport_init(struct vc_data *vc, int init)
> {
> int cols, rows;
>
> - cols = newport_xsize / 8;
> - rows = newport_ysize / 16;
> + cols = newport_xsize / NEWPORT_MAX_FONT_WIDTH;
> + rows = newport_ysize / NEWPORT_MAX_FONT_HEIGHT;
> vc->vc_can_do_color = 1;
> if (init) {
> vc->vc_cols = cols;
> @@ -507,8 +510,8 @@ static int newport_set_font(int unit, struct console_font *op, unsigned int vpit
>
> /* ladis: when I grow up, there will be a day... and more sizes will
> * be supported ;-) */
> - if ((w != 8) || (h != 16) || (vpitch != 32)
> - || (op->charcount != 256 && op->charcount != 512))
> + if ((w != NEWPORT_MAX_FONT_WIDTH) || (h != NEWPORT_MAX_FONT_HEIGHT) ||
> + (vpitch != 32) || (op->charcount != 256 && op->charcount != 512))
> return -EINVAL;
>
> if (!(new_data = kmalloc(FONT_EXTRA_WORDS * sizeof(int) + size,
> @@ -569,6 +572,15 @@ static int newport_font_default(struct vc_data *vc, struct console_font *op, cha
> return newport_set_def_font(vc->vc_num, op);
> }
>
> +static int newport_font_info(struct vc_data *vc, struct console_font_info *info)
> +{
> + info->min_width = info->max_width = NEWPORT_MAX_FONT_WIDTH;
> + info->min_height = info->max_height = NEWPORT_MAX_FONT_HEIGHT;
> + info->flags = KD_FONT_INFO_FLAG_LOW_SIZE | KD_FONT_INFO_FLAG_HIGH_SIZE;
> +
> + return 0;
> +}
> +
> static int newport_font_set(struct vc_data *vc, struct console_font *font,
> unsigned int vpitch, unsigned int flags)
> {
> @@ -688,6 +700,7 @@ const struct consw newport_con = {
> .con_scroll = newport_scroll,
> .con_switch = newport_switch,
> .con_blank = newport_blank,
> + .con_font_info = newport_font_info,
> .con_font_set = newport_font_set,
> .con_font_default = newport_font_default,
> .con_save_screen = newport_save_screen
> diff --git a/drivers/video/console/sticon.c b/drivers/video/console/sticon.c
> index 992a4fa431aa..d32ca458eb77 100644
> --- a/drivers/video/console/sticon.c
> +++ b/drivers/video/console/sticon.c
> @@ -56,6 +56,11 @@
> #define BLANK 0
> static int vga_is_gfx;
>
> +#define STICON_MIN_FONT_WIDTH 6
> +#define STICON_MIN_FONT_HEIGHT 6
> +#define STICON_MAX_FONT_WIDTH 32
> +#define STICON_MAX_FONT_HEIGHT 32
> +
> #define STI_DEF_FONT sticon_sti->font
>
> /* borrowed from fbcon.c */
> @@ -180,8 +185,10 @@ static int sticon_set_font(struct vc_data *vc, struct console_font *op,
> struct sti_cooked_font *cooked_font;
> unsigned char *data = op->data, *p;
>
> - if ((w < 6) || (h < 6) || (w > 32) || (h > 32) || (vpitch != 32)
> - || (op->charcount != 256 && op->charcount != 512))
> + if (!in_range(w, STICON_MIN_FONT_WIDTH, STICON_MAX_FONT_WIDTH) ||
> + !in_range(h, STICON_MIN_FONT_HEIGHT, STICON_MAX_FONT_HEIGHT) ||
> + (vpitch != 32) ||
> + (op->charcount != 256 && op->charcount != 512))
> return -EINVAL;
> pitch = ALIGN(w, 8) / 8;
> bpc = pitch * h;
> @@ -273,6 +280,19 @@ static int sticon_font_set(struct vc_data *vc, struct console_font *font,
> return sticon_set_font(vc, font, vpitch);
> }
>
> +static int sticon_font_info(struct vc_data *vc, struct console_font_info *info)
> +{
> + info->min_width = STICON_MIN_FONT_WIDTH;
> + info->min_height = STICON_MIN_FONT_HEIGHT;
> +
> + info->max_width = STICON_MAX_FONT_WIDTH;
> + info->max_height = STICON_MAX_FONT_HEIGHT;
> +
> + info->flags = KD_FONT_INFO_FLAG_LOW_SIZE | KD_FONT_INFO_FLAG_HIGH_SIZE;
> +
> + return 0;
> +}
> +
> static void sticon_init(struct vc_data *c, int init)
> {
> struct sti_struct *sti = sticon_sti;
> @@ -371,6 +391,7 @@ static const struct consw sti_con = {
> .con_scroll = sticon_scroll,
> .con_switch = sticon_switch,
> .con_blank = sticon_blank,
> + .con_font_info = sticon_font_info,
> .con_font_set = sticon_font_set,
> .con_font_default = sticon_font_default,
> .con_build_attr = sticon_build_attr,
> diff --git a/drivers/video/console/vgacon.c b/drivers/video/console/vgacon.c
> index 8ef1579fa57f..b75d31ef3353 100644
> --- a/drivers/video/console/vgacon.c
> +++ b/drivers/video/console/vgacon.c
> @@ -61,6 +61,10 @@ static struct vgastate vgastate;
> #define BLANK 0x0020
>
> #define VGA_FONTWIDTH 8 /* VGA does not support fontwidths != 8 */
> +
> +#define VGACON_MAX_FONT_WIDTH VGA_FONTWIDTH
> +#define VGACON_MAX_FONT_HEIGHT 32
> +
> /*
> * Interface used by the world
> */
> @@ -1013,6 +1017,19 @@ static int vgacon_adjust_height(struct vc_data *vc, unsigned fontheight)
> return 0;
> }
>
> +static int vgacon_font_info(struct vc_data *vc, struct console_font_info *info)
> +{
> + info->min_width = VGACON_MAX_FONT_WIDTH;
> + info->min_height = 0;
> +
> + info->max_width = VGACON_MAX_FONT_WIDTH;
> + info->max_height = VGACON_MAX_FONT_HEIGHT;
> +
> + info->flags = KD_FONT_INFO_FLAG_LOW_SIZE | KD_FONT_INFO_FLAG_HIGH_SIZE;
> +
> + return 0;
> +}
> +
> static int vgacon_font_set(struct vc_data *c, struct console_font *font,
> unsigned int vpitch, unsigned int flags)
> {
> @@ -1022,7 +1039,8 @@ static int vgacon_font_set(struct vc_data *c, struct console_font *font,
> if (vga_video_type < VIDEO_TYPE_EGAM)
> return -EINVAL;
>
> - if (font->width != VGA_FONTWIDTH || font->height > 32 || vpitch != 32 ||
> + if (font->width != VGACON_MAX_FONT_WIDTH ||
> + font->height > VGACON_MAX_FONT_HEIGHT || vpitch != 32 ||
> (charcount != 256 && charcount != 512))
> return -EINVAL;
>
> @@ -1177,6 +1195,7 @@ const struct consw vga_con = {
> .con_scroll = vgacon_scroll,
> .con_switch = vgacon_switch,
> .con_blank = vgacon_blank,
> + .con_font_info = vgacon_font_info,
> .con_font_set = vgacon_font_set,
> .con_font_get = vgacon_font_get,
> .con_resize = vgacon_resize,
> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> index 46823c2e2ba1..e10abe416159 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -101,6 +101,9 @@ enum {
> FBCON_LOGO_DONTSHOW = -3 /* do not show the logo */
> };
>
> +#define FBCON_MAX_FONT_WIDTH 32
> +#define FBCON_MAX_FONT_HEIGHT 32
> +
> static struct fbcon_display fb_display[MAX_NR_CONSOLES];
>
> static struct fb_info *fbcon_registered_fb[FB_MAX];
> @@ -2456,6 +2459,21 @@ static int fbcon_do_set_font(struct vc_data *vc, int w, int h, int charcount,
> return ret;
> }
>
> +
> +static int fbcon_font_info(struct vc_data *vc, struct console_font_info *info)
> +{
> + info->min_width = 0;
> + info->min_height = 0;
> +
> + info->max_width = FBCON_MAX_FONT_WIDTH;
> + info->max_height = FBCON_MAX_FONT_HEIGHT;
> +
> + info->flags = KD_FONT_INFO_FLAG_LOW_SIZE | KD_FONT_INFO_FLAG_HIGH_SIZE;
> +
> + return 0;
> +}
> +
> +
> /*
> * User asked to set font; we are guaranteed that charcount does not exceed 512
> * but lets not assume that, since charcount of 512 is small for unicode support.
> @@ -2483,7 +2501,8 @@ static int fbcon_set_font(struct vc_data *vc, struct console_font *font,
> h > FBCON_SWAP(info->var.rotate, info->var.yres, info->var.xres))
> return -EINVAL;
>
> - if (font->width > 32 || font->height > 32)
> + if (font->width > FBCON_MAX_FONT_WIDTH ||
> + font->height > FBCON_MAX_FONT_HEIGHT)
> return -EINVAL;
>
> /* Make sure drawing engine can handle the font */
> @@ -3158,6 +3177,7 @@ static const struct consw fb_con = {
> .con_scroll = fbcon_scroll,
> .con_switch = fbcon_switch,
> .con_blank = fbcon_blank,
> + .con_font_info = fbcon_font_info,
> .con_font_set = fbcon_set_font,
> .con_font_get = fbcon_get_font,
> .con_font_default = fbcon_set_def_font,
^ permalink raw reply
* Re: [PATCH v3 1/2] VT: Add KDFONTINFO ioctl
From: Helge Deller @ 2024-03-15 9:15 UTC (permalink / raw)
To: legion, Greg Kroah-Hartman, Jiri Slaby
Cc: LKML, kbd, linux-api, linux-fbdev, linux-serial
In-Reply-To: <ed056326540f04b72c97a276fbcc316e1b2f6371.1710252966.git.legion@kernel.org>
On 3/12/24 15:23, legion@kernel.org wrote:
> From: Alexey Gladkov <legion@kernel.org>
>
> Each driver has its own restrictions on font size. There is currently no
> way to understand what the requirements are. The new ioctl allows
> userspace to get the minmum and maximum font size values.
>
> Signed-off-by: Alexey Gladkov <legion@kernel.org>
Acked-by: Helge Deller <deller@gmx.de>
Helge
> ---
> drivers/tty/vt/vt.c | 24 ++++++++++++++++++++++++
> drivers/tty/vt/vt_ioctl.c | 13 +++++++++++++
> include/linux/console.h | 2 ++
> include/linux/vt_kern.h | 1 +
> include/uapi/linux/kd.h | 13 ++++++++++++-
> 5 files changed, 52 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index 156efda7c80d..8c2a3d98b5ec 100644
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -4680,6 +4680,30 @@ int con_font_op(struct vc_data *vc, struct console_font_op *op)
> return -ENOSYS;
> }
>
> +int con_font_info(struct vc_data *vc, struct console_font_info *info)
> +{
> + int rc = -EINVAL;
> +
> + info->min_height = 0;
> + info->max_height = max_font_height;
> +
> + info->min_width = 0;
> + info->max_width = max_font_width;
> +
> + info->flags = KD_FONT_INFO_FLAG_LOW_SIZE | KD_FONT_INFO_FLAG_HIGH_SIZE;
> +
> + console_lock();
> + if (vc->vc_mode != KD_TEXT)
> + rc = -EINVAL;
> + else if (vc->vc_sw->con_font_info)
> + rc = vc->vc_sw->con_font_info(vc, info);
> + else
> + rc = -ENOSYS;
> + console_unlock();
> +
> + return rc;
> +}
> +
> /*
> * Interface exported to selection and vcs.
> */
> diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c
> index 8c685b501404..b3b4e4b69366 100644
> --- a/drivers/tty/vt/vt_ioctl.c
> +++ b/drivers/tty/vt/vt_ioctl.c
> @@ -479,6 +479,19 @@ static int vt_k_ioctl(struct tty_struct *tty, unsigned int cmd,
> break;
> }
>
> + case KDFONTINFO: {
> + struct console_font_info fnt_info;
> +
> + if (copy_from_user(&fnt_info, up, sizeof(fnt_info)))
> + return -EFAULT;
> + ret = con_font_info(vc, &fnt_info);
> + if (ret)
> + return ret;
> + if (copy_to_user(up, &fnt_info, sizeof(fnt_info)))
> + return -EFAULT;
> + break;
> + }
> +
> default:
> return -ENOIOCTLCMD;
> }
> diff --git a/include/linux/console.h b/include/linux/console.h
> index 779d388af8a0..5bea6f6c2042 100644
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -20,6 +20,7 @@
> #include <linux/types.h>
>
> struct vc_data;
> +struct console_font_info;
> struct console_font_op;
> struct console_font;
> struct module;
> @@ -59,6 +60,7 @@ struct consw {
> unsigned int lines);
> int (*con_switch)(struct vc_data *vc);
> int (*con_blank)(struct vc_data *vc, int blank, int mode_switch);
> + int (*con_font_info)(struct vc_data *vc, struct console_font_info *info);
> int (*con_font_set)(struct vc_data *vc, struct console_font *font,
> unsigned int vpitch, unsigned int flags);
> int (*con_font_get)(struct vc_data *vc, struct console_font *font,
> diff --git a/include/linux/vt_kern.h b/include/linux/vt_kern.h
> index c1f5aebef170..6bda4cc1fe6f 100644
> --- a/include/linux/vt_kern.h
> +++ b/include/linux/vt_kern.h
> @@ -32,6 +32,7 @@ void do_blank_screen(int entering_gfx);
> void do_unblank_screen(int leaving_gfx);
> void poke_blanked_console(void);
> int con_font_op(struct vc_data *vc, struct console_font_op *op);
> +int con_font_info(struct vc_data *vc, struct console_font_info *info);
> int con_set_cmap(unsigned char __user *cmap);
> int con_get_cmap(unsigned char __user *cmap);
> void scrollback(struct vc_data *vc);
> diff --git a/include/uapi/linux/kd.h b/include/uapi/linux/kd.h
> index 6b384065c013..781e086e55bf 100644
> --- a/include/uapi/linux/kd.h
> +++ b/include/uapi/linux/kd.h
> @@ -183,8 +183,19 @@ struct console_font {
>
> #define KD_FONT_FLAG_DONT_RECALC 1 /* Don't recalculate hw charcell size [compat] */
>
> +#define KDFONTINFO 0x4B73 /* font information */
> +
> +#define KD_FONT_INFO_FLAG_LOW_SIZE (1U << 0) /* 256 */
> +#define KD_FONT_INFO_FLAG_HIGH_SIZE (1U << 1) /* 512 */
> +
> +struct console_font_info {
> + unsigned int min_width, min_height; /* minimal font size */
> + unsigned int max_width, max_height; /* maximum font size */
> + unsigned int flags; /* KD_FONT_INFO_FLAG_* */
> +};
> +
> /* note: 0x4B00-0x4B4E all have had a value at some time;
> don't reuse for the time being */
> -/* note: 0x4B60-0x4B6D, 0x4B70-0x4B72 used above */
> +/* note: 0x4B60-0x4B6D, 0x4B70-0x4B73 used above */
>
> #endif /* _UAPI_LINUX_KD_H */
^ permalink raw reply
* Re: [PATCH v7] posix-timers: add clock_compare system call
From: Thomas Gleixner @ 2024-03-14 18:42 UTC (permalink / raw)
To: Sagi Maimon
Cc: richardcochran, luto, datglx, mingo, bp, dave.hansen, x86, hpa,
arnd, geert, peterz, hannes, sohil.mehta, rick.p.edgecombe,
nphamcs, palmer, keescook, legion, mark.rutland, mszeredi, casey,
reibax, davem, brauner, linux-kernel, linux-api, linux-arch,
netdev
In-Reply-To: <CAMuE1bHOm2Y1bOpggStMOjZhN5TaxoC1gJea5Mdrc+mormQg0g@mail.gmail.com>
On Thu, Mar 14 2024 at 17:46, Sagi Maimon wrote:
Can you please trim your replies? I really have better things to do than
doing detective work to find 10 new lines within 200+ irrelevant ones.
> On Thu, Mar 14, 2024 at 1:12 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>> Please read and follow the documentation provided at:
>>
>> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html
>>
> I have missed this part on prviews reply.
> I have read the documentation above and I think that the variable
> declarations at the beginning of a function is in reverse fir tree
> order meaning from big to small, but I guess that I am missing something,
> can you please explain what is wrong with the variable declaration,
> so I can fix it.
>> > + struct timespec64 ts_a, ts_a1, ts_b, ts_a2;
>> > + struct system_device_crosststamp xtstamp_a1, xtstamp_a2, xtstamp_b;
>> > + const struct k_clock *kc_a, *kc_b;
>> > + ktime_t ktime_a;
>> > + s64 ts_offs_err = 0;
>> > + int error = 0;
>> > + bool crosstime_support_a = false;
>> > + bool crosstime_support_b = false;
It's not about the data type. Look at the three layouts and figure out
which one is better to parse.
^ permalink raw reply
* Re: [PATCH v3] LSM: use 32 bit compatible data types in LSM syscalls.
From: Paul Moore @ 2024-03-14 18:18 UTC (permalink / raw)
To: Dmitry V. Levin
Cc: Casey Schaufler, LSM List, Linux kernel mailing list, linux-api,
Mickaël Salaün, James Morris, Serge Hallyn,
John Johansen, Tetsuo Handa, Stephen Smalley
In-Reply-To: <20240314180143.GA26431@altlinux.org>
On Thu, Mar 14, 2024 at 2:01 PM Dmitry V. Levin <ldv@strace.io> wrote:
> On Thu, Mar 14, 2024 at 11:30:53AM -0400, Paul Moore wrote:
> > On Wed, Mar 13, 2024 at 4:07 PM Paul Moore <paul@paul-moore.com> wrote:
> > > On Mar 13, 2024 Casey Schaufler <casey@schaufler-ca.com> wrote:
> > > >
> > > > LSM: use 32 bit compatible data types in LSM syscalls.
> > > >
> > > > Change the size parameters in lsm_list_modules(), lsm_set_self_attr()
> > > > and lsm_get_self_attr() from size_t to u32. This avoids the need to
> > > > have different interfaces for 32 and 64 bit systems.
> > > >
> > > > Cc: stable@vger.kernel.org
> > > > Fixes: a04a1198088a: ("LSM: syscalls for current process attributes")
> > > > Fixes: ad4aff9ec25f: ("LSM: Create lsm_list_modules system call")
> > > > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> > > > Reported-and-reviewed-by: Dmitry V. Levin <ldv@strace.io>
> > > > ---
> > > > include/linux/lsm_hook_defs.h | 4 ++--
> > > > include/linux/security.h | 8 ++++----
> > > > security/apparmor/lsm.c | 4 ++--
> > > > security/lsm_syscalls.c | 10 +++++-----
> > > > security/security.c | 12 ++++++------
> > > > security/selinux/hooks.c | 4 ++--
> > > > security/smack/smack_lsm.c | 4 ++--
> > > > tools/testing/selftests/lsm/common.h | 6 +++---
> > > > tools/testing/selftests/lsm/lsm_get_self_attr_test.c | 10 +++++-----
> > > > tools/testing/selftests/lsm/lsm_list_modules_test.c | 8 ++++----
> > > > tools/testing/selftests/lsm/lsm_set_self_attr_test.c | 6 +++---
> > > > 11 files changed, 38 insertions(+), 38 deletions(-)
> > >
> > > Okay, this looks better, I'm going to merge this into lsm/stable-6.9
> > > and put it through the usual automated testing as well as a kselftest
> > > run to make sure everything there is still okay. Assuming all goes
> > > well and no one raises any objections, I'll likely send this up to
> > > Linus tomorrow.
> >
> > I had to squash the code snippet below into the patch to address a
> > build problem identified by the kernel build robot. I'm going to keep
> > Casey's sign-off and Dmitry's reported-reviewed tag as I feel this
> > change is minor, but if anyone has any objections please let me know
> > soon.
> >
> > [NOTE: cut-n-paste'd into email, likely whitespace damage, but you get the idea]
> >
> > diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> > index 77eb9b0e7685..e619ac10cd23 100644
> > --- a/include/linux/syscalls.h
> > +++ b/include/linux/syscalls.h
> > @@ -960,10 +960,10 @@ asmlinkage long sys_cachestat(unsigned int fd,
> > struct cachestat __user *cstat, unsigned int flags);
> > asmlinkage long sys_map_shadow_stack(unsigned long addr, unsigned long size, un
> > signed int flags);
> > asmlinkage long sys_lsm_get_self_attr(unsigned int attr, struct lsm_ctx *ctx,
> > - size_t *size, __u32 flags);
> > + u32 *size, u32 flags);
> > asmlinkage long sys_lsm_set_self_attr(unsigned int attr, struct lsm_ctx *ctx,
> > - size_t size, __u32 flags);
> > -asmlinkage long sys_lsm_list_modules(u64 *ids, size_t *size, u32 flags);
> > + u32 size, u32 flags);
> > +asmlinkage long sys_lsm_list_modules(u64 *ids, u32 *size, u32 flags);
>
> Fine with me, thanks.
>
> btw, with the change above, u32 will become about twice more popular
> in include/linux/syscalls.h than __u32.
I was looking at that when I was putting the patch together this
morning, trying to decide which was the "correct" choice between 'u32'
and '__u32' and wasn't able to find a good explanation of which is the
"right" option in this file. Ultimately I went with 'u32' as I tend
to follow some old guidance of: '__u32' for userspace headers, 'u32'
for kernel headers.
If it should be the other way, please let me know. I just want to
keep it consistent across the LSM syscalls.
--
paul-moore.com
^ permalink raw reply
* Re: [PATCH v7] posix-timers: add clock_compare system call
From: Thomas Gleixner @ 2024-03-14 18:08 UTC (permalink / raw)
To: Sagi Maimon
Cc: richardcochran, luto, tglx, mingo, bp, dave.hansen, x86, hpa,
arnd, geert, peterz, hannes, sohil.mehta, rick.p.edgecombe,
nphamcs, palmer, keescook, legion, mark.rutland, mszeredi, casey,
reibax, davem, brauner, linux-kernel, linux-api, linux-arch,
netdev
In-Reply-To: <CAMuE1bH_H9E+Zx365G9AtmWSmhW-kPPB+-=8s2rH4hpxqE+dHQ@mail.gmail.com>
Sagi!
On Thu, Mar 14 2024 at 14:19, Sagi Maimon wrote:
> On Thu, Mar 14, 2024 at 1:12 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>> On Thu, Mar 14 2024 at 11:05, Sagi Maimon wrote:
>> > Some user space applications need to read a couple of different clocks.
>> > Each read requires moving from user space to kernel space.
>> > Reading each clock separately (syscall) introduces extra
>> > unpredictable/unmeasurable delay. Minimizing this delay contributes to user
>> > space actions on these clocks (e.g. synchronization etc).
>>
>> I asked for a proper description of the actual problem several times now
>> and you still provide some handwaving blurb. Feel free to ignore me, but
>> then please don't be surprised if I ignore you too.
>>
> Nobody is ignoring your notes, and I address any notes given by any
> maintainer in the most serious way.
> As far as explaining the actual problem this is the best that I can,
> but let me try to explain better:
> We did many tests with different CPU loading and compared sampling the
> same clock twice,
> once in user space and once by using the system call.
> We have noticed an improvement up to hundreds of nanoseconds while
> using the system call.
> Those results improved our ability to sync different PHCs
So let me express how I understand the problem - as far as I decoded it
from your writeups:
Synchronizing two PHCs requires to read timestamps from both and
correlate them. Currently this requires several seperate system calls.
This is subject to unmeasurable delays due to system call overhead,
preemption and interrupts which makes the correlation imprecise.
Therefore you want a system call, which samples two clocks at once, to
make the correlation more precise.
Right? For the further comments I assume this is what you are trying to
say and to solve.
So far so good, except that I do not agree with that reasoning at all:
1. The delays are measurable and as precise as the cross time stamp
mechanism (hardware or software based) allows.
2. The system call overhead is completely irrelevant.
3. The time deltas between the sample points are irrelevant within a
reasonable upper bound to the time delta between the two outer
sample points.
4. The alledged higher precision is based on a guesstimate and not on
correctness. Just because it behaves slightly better in testing
does not make it any more correct.
5. The problem can be solved with maximal possible accuracy by using
the existing PTP IOCTLs.
See below.
>> Also why does reading two random clocks make any sense at all? Your code
>> allows to read CLOCK_TAI and CLOCK_THREAD_CPUTIME_ID. What for?
>>
> Initially we needed to sync some different PHCs for our user space
> application, that is why we came with this idea.
> The first idea was an IOCTL that returned samples of several PHCs for
> the need of synchronization.
> Richard Cochran suggested a system call instead, which will add the
> ability to get various system clocks, while this
> implementation is more complex then IOCTL, I think that he was right,
> for future usage.
Which future usage? We are not introducing swiss army knife interfaces
just because there might be an illusional use case somewhere in the
unspecified future.
Adding a system call needs a proper design and justification. Handwaving
future usage is not enough.
Documentation/process/adding-syscalls.rst is very clear about what is
required for a new system call.
>> This needs to be split up into:
>>
>> 1) Infrastructure in posix-timers.c
>> 2) Wire up the syscall in x86
>> 3) Infrastructure in posix-clock.c
>> 4) Usage in ptp_clock.c
>>
>> and not as a big lump of everything.
>>
> I know, but I think the benefit worth it
It's worth it because it makes review easier. It's well documented in
the process documentation that patches should do one thing and not a
whole lump of changes.
>> > + if (!error) {
>> > + if (clock_b == CLOCK_MONOTONIC_RAW) {
>> > + ts_b = ktime_to_timespec64(xtstamp_a1.sys_monoraw);
>> > + ts_a1 = ktime_to_timespec64(xtstamp_a1.device);
>> > + goto out;
>> > + } else if (clock_b == CLOCK_REALTIME) {
>> > + ts_b = ktime_to_timespec64(xtstamp_a1.sys_realtime);
>> > + ts_a1 = ktime_to_timespec64(xtstamp_a1.device);
>> > + goto out;
>> > + } else {
>> > + crosstime_support_a = true;
>>
>> Right. If clock_b is anything else than CLOCK_MONOTONIC_RAW or
>> CLOCK_REALTIME then this is true.
>>
>> > + }
>> > + }
>>
>> So in case of an error, this just keeps going with an uninitialized
>> xtstamp_a1 and if the clock_b part succeeds it continues and operates on
>> garbage.
>>
> On error xtstamp_a1 will be taken again using clock_get_crosstimespec
> so no one will be operating on garbage.
It will not, because crosstime_support_a == false. It will therefore
fall back to kc_a->clock_get_timespec(), no?
Sorry, I misread the code vs. using the uninitialized value, but this is
just unneccesary hard to follow.
>> > + if (crosstime_support_a)
>> > + error = kc_a->clock_get_crosstimespec(clock_a, &xtstamp_a2);
>> > + else
>> > + error = kc_a->clock_get_timespec(clock_a, &ts_a2);
>> > +
>> > + if (error)
>> > + return error;
>>
>> The logic and the code flow here are unreadable garbage and there are
>> zero comments what this is supposed to do.
>>
> I will add comments.
> please no need to use negative words like "garbage" (not the first time),
> please keep it professional and civilized.
Let me rephrase:
The code and the logic is incomprehensible unless I waste an unjustified
amount of time to decode it. Sorry, I don't have that time.
>> > + if (crosstime_support_a) {
>> > + ktime_a = ktime_sub(xtstamp_a2.device, xtstamp_a1.device);
>> > + ts_offs_err = ktime_divns(ktime_a, 2);
>> > + ktime_a = ktime_add_ns(xtstamp_a1.device, (u64)ts_offs_err);
>> > + ts_a1 = ktime_to_timespec64(ktime_a);
>>
>> This is just wrong.
>>
>> read(a1);
>> read(b);
>> read(a2);
>>
>> You _CANNOT_ assume that (a1 + ((a2 - a1) / 2) is anywhere close to the
>> point in time where 'b' is read. This code is preemtible and
>> interruptible. I explained this to you before.
>>
>> Your explanation in the comment above the function is just wishful
>> thinking.
>>
> you explained it before, but still it is better then two consecutive
> user space calls which are also preemptible
> and the userspace to kernel context switch time is added.
It might be marginally better, but it is still just _pretending_ that it
does the right thing, is correct and better than the existing IOCTLs.
If your user space implementation has the same algorithm, then I'm
absolutely not surprised that the results are not useful. Why?
You simply cannot use the midpoint of the outer samples if you want to
have precise results if there is no guarantee that b was sampled exactly
in the midpoint of a1 and a2. A hardware implementation might give that
guarantee, but the kernel cannot.
But why using the midpoint in the first place?
There is absolutely no reason to do so because the sampling points a1, b
and a2 can be precisely determined with the precision of the cross time
stamp mechanism, which is best with a hardware based cross time stamp
obviously.
The whole point of ptp::info::getcrosststamp() is to get properly
correlated clock samples of
1) PHC clock
2) CLOCK_MONOTONIC_RAW
3) CLOCK_REALTIME
So if you take 3 samples:
get_cross_timestamp(a1);
get_cross_timestamp(b);
get_cross_timestamp(a2);
then each of them provides:
- device time
- correlated CLOCK_MONOTONIC_RAW
- correlated CLOCK_REALTIME
Ergo the obvious thing to do is:
d1 = b.sys_monoraw - a1.sys_monoraw;
d2 = a2.sys_monoraw - a1.sys_monoraw;
tsa = a1.device + ((a2.device - a1.device) * d1) / d2;
Which is maximaly precise under the assumption that in the time between
the sample points a1 and a2 neither the system clock nor the PCH clocks
are changing their frequency significantly. That is a valid assumption
when you put a reasonable upper bound on d2.
Even when the device does not implement getcrosststamp() then loop based
sampling like it is implemented in the PTP_SYS_OFFSET[_EXTENDED] IOTCLs
is providing reasonably accurate results to the extent possible.
Your algorithm is imprecise by definition and you can apply as much
testing as you want, it won't become magically correct. It's still a
guesstimate, i.e. an estimate made without using adequate or complete
information.
Now why a new syscall?
This can be done from user space with existing interfaces and the very
same precision today:
ioctl(fda, PTP_SYS_OFFSET*, &a1);
ioctl(fdb, PTP_SYS_OFFSET*, &b);
ioctl(fda, PTP_SYS_OFFSET*, &a2);
u64 d1 = timespec_delta_ns(b.sys_monoraw, a1.sys_monoraw);
u64 d2 = timespec_delta_ns(a2.sys_monoraw, a1.sys_monoraw);
u64 td = (timespec_delta_ns(a2.device, a1.device) * d1) / d2
tsa = timespec_add_ns(a1.device, td);
tsb = b.device;
with the extra benefit of:
1) The correct CLOCK_REALTIME at that sample point,
i.e. b.sys_realtime
2) The correct CLOCK_MONOTONIC_RAW at that sample point,
i.e. b.sys_monoraw
It works with PTP_SYS_OFFSET_PRECISE and PTP_SYS_OFFSET[_EXTENDED], with
the obvious limitations of PTP_SYS_OFFSET[_EXTENDED], which are still
vastly superior to your proposed (a2 - a1) / 2 guestimate which is just
reading the PCH clocks with clock_get_timespec().
It is completely independent of the load, the syscall overhead and the
actual time delta between the sample points when you apply a reasonable
upper bound for d2, i.e. the time delta between the sample points a1 and
a2 to eliminate the issue that system clock and/or the PCH clocks change
their frequency significantly during that time. You'd need to do that in
the kernel too.
The actual frequency difference between PCH A and system clock is
completely irrelevant when the frequencies of both are stable accross
the sample period.
You might still argue that the time delta between the sample points a1
and a2 matters and is slightly shorter in the kernel, but that is a
non-argument because:
1) The kernel implementation does not guarantee atomicity of the
consecutive samples either. The time delta is just statistically
better, which is obviously useless when you want to make
guarantees.
2) It does not matter when the time delta is slightly larger because
you need a large frequency change of the involved clocks in the
sample interval between the sample points a1 and a2 to make an
actual difference in the resulting accuracy.
A typical temperature drift of a high quality cyrstal is less than
1ppm per degree Celsius and even if you assume that the overall
system drift is 10ppm per degree Celsius then still the actual
error for a bound time delta between the sample points a1 and a2 is
just somewhere in the irrelevant noise, unless you manage to blow
torch or ice spray your crystals during the sample interval.
If your clocks are not stable enough then nothing can cure it and
you cannot do high precision timekeeping with them.
So what is your new syscall solving that can't be done with the existing
IOCTLs other than providing worse precision results based on
guesstimates and some handwavy future use for random clock ids?
Nothing as far as I can tell, but I might be missing something important
here.
Thanks,
tglx
---
arch/x86/kernel/tsc.c:119: "Math is hard, let's go shopping." - John Stultz
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox