* Re: [PATCHv8 07/10] acpi/hmat: Register processor domain to its memory
From: Rafael J. Wysocki @ 2019-03-13 23:22 UTC (permalink / raw)
To: Keith Busch
Cc: Linux Kernel Mailing List, ACPI Devel Maling List,
Linux Memory Management List, Linux API, Greg Kroah-Hartman,
Rafael Wysocki, Dave Hansen, Dan Williams, Jonathan Cameron,
Brice Goglin
In-Reply-To: <20190311205606.11228-8-keith.busch@intel.com>
On Mon, Mar 11, 2019 at 9:55 PM Keith Busch <keith.busch@intel.com> wrote:
>
> If the HMAT Subsystem Address Range provides a valid processor proximity
> domain for a memory domain, or a processor domain matches the performance
> access of the valid processor proximity domain, register the memory
> target with that initiator so this relationship will be visible under
> the node's sysfs directory.
>
> Since HMAT requires valid address ranges have an equivalent SRAT entry,
> verify each memory target satisfies this requirement.
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Keith Busch <keith.busch@intel.com>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> drivers/acpi/hmat/Kconfig | 3 +-
> drivers/acpi/hmat/hmat.c | 392 +++++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 393 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/hmat/Kconfig b/drivers/acpi/hmat/Kconfig
> index 2f7111b7af62..13cddd612a52 100644
> --- a/drivers/acpi/hmat/Kconfig
> +++ b/drivers/acpi/hmat/Kconfig
> @@ -4,4 +4,5 @@ config ACPI_HMAT
> depends on ACPI_NUMA
> help
> If set, this option has the kernel parse and report the
> - platform's ACPI HMAT (Heterogeneous Memory Attributes Table).
> + platform's ACPI HMAT (Heterogeneous Memory Attributes Table),
> + and register memory initiators with their targets.
> diff --git a/drivers/acpi/hmat/hmat.c b/drivers/acpi/hmat/hmat.c
> index 4758beb3b2c1..01a6eddac6f7 100644
> --- a/drivers/acpi/hmat/hmat.c
> +++ b/drivers/acpi/hmat/hmat.c
> @@ -13,11 +13,105 @@
> #include <linux/device.h>
> #include <linux/init.h>
> #include <linux/list.h>
> +#include <linux/list_sort.h>
> #include <linux/node.h>
> #include <linux/sysfs.h>
>
> static __initdata u8 hmat_revision;
>
> +static __initdata LIST_HEAD(targets);
> +static __initdata LIST_HEAD(initiators);
> +static __initdata LIST_HEAD(localities);
> +
> +/*
> + * The defined enum order is used to prioritize attributes to break ties when
> + * selecting the best performing node.
> + */
> +enum locality_types {
> + WRITE_LATENCY,
> + READ_LATENCY,
> + WRITE_BANDWIDTH,
> + READ_BANDWIDTH,
> +};
> +
> +static struct memory_locality *localities_types[4];
> +
> +struct memory_target {
> + struct list_head node;
> + unsigned int memory_pxm;
> + unsigned int processor_pxm;
> + struct node_hmem_attrs hmem_attrs;
> +};
> +
> +struct memory_initiator {
> + struct list_head node;
> + unsigned int processor_pxm;
> +};
> +
> +struct memory_locality {
> + struct list_head node;
> + struct acpi_hmat_locality *hmat_loc;
> +};
> +
> +static __init struct memory_initiator *find_mem_initiator(unsigned int cpu_pxm)
> +{
> + struct memory_initiator *initiator;
> +
> + list_for_each_entry(initiator, &initiators, node)
> + if (initiator->processor_pxm == cpu_pxm)
> + return initiator;
> + return NULL;
> +}
> +
> +static __init struct memory_target *find_mem_target(unsigned int mem_pxm)
> +{
> + struct memory_target *target;
> +
> + list_for_each_entry(target, &targets, node)
> + if (target->memory_pxm == mem_pxm)
> + return target;
> + return NULL;
> +}
> +
> +static __init void alloc_memory_initiator(unsigned int cpu_pxm)
> +{
> + struct memory_initiator *initiator;
> +
> + if (pxm_to_node(cpu_pxm) == NUMA_NO_NODE)
> + return;
> +
> + initiator = find_mem_initiator(cpu_pxm);
> + if (initiator)
> + return;
> +
> + initiator = kzalloc(sizeof(*initiator), GFP_KERNEL);
> + if (!initiator)
> + return;
> +
> + initiator->processor_pxm = cpu_pxm;
> + list_add_tail(&initiator->node, &initiators);
> +}
> +
> +static __init void alloc_memory_target(unsigned int mem_pxm)
> +{
> + struct memory_target *target;
> +
> + if (pxm_to_node(mem_pxm) == NUMA_NO_NODE)
> + return;
> +
> + target = find_mem_target(mem_pxm);
> + if (target)
> + return;
> +
> + target = kzalloc(sizeof(*target), GFP_KERNEL);
> + if (!target)
> + return;
> +
> + target->memory_pxm = mem_pxm;
> + target->processor_pxm = PXM_INVAL;
> + list_add_tail(&target->node, &targets);
> +}
> +
> static __init const char *hmat_data_type(u8 type)
> {
> switch (type) {
> @@ -89,14 +183,83 @@ static __init u32 hmat_normalize(u16 entry, u64 base, u8 type)
> return value;
> }
>
> +static __init void hmat_update_target_access(struct memory_target *target,
> + u8 type, u32 value)
> +{
> + switch (type) {
> + case ACPI_HMAT_ACCESS_LATENCY:
> + target->hmem_attrs.read_latency = value;
> + target->hmem_attrs.write_latency = value;
> + break;
> + case ACPI_HMAT_READ_LATENCY:
> + target->hmem_attrs.read_latency = value;
> + break;
> + case ACPI_HMAT_WRITE_LATENCY:
> + target->hmem_attrs.write_latency = value;
> + break;
> + case ACPI_HMAT_ACCESS_BANDWIDTH:
> + target->hmem_attrs.read_bandwidth = value;
> + target->hmem_attrs.write_bandwidth = value;
> + break;
> + case ACPI_HMAT_READ_BANDWIDTH:
> + target->hmem_attrs.read_bandwidth = value;
> + break;
> + case ACPI_HMAT_WRITE_BANDWIDTH:
> + target->hmem_attrs.write_bandwidth = value;
> + break;
> + default:
> + break;
> + }
> +}
> +
> +static __init void hmat_add_locality(struct acpi_hmat_locality *hmat_loc)
> +{
> + struct memory_locality *loc;
> +
> + loc = kzalloc(sizeof(*loc), GFP_KERNEL);
> + if (!loc) {
> + pr_notice_once("Failed to allocate HMAT locality\n");
> + return;
> + }
> +
> + loc->hmat_loc = hmat_loc;
> + list_add_tail(&loc->node, &localities);
> +
> + switch (hmat_loc->data_type) {
> + case ACPI_HMAT_ACCESS_LATENCY:
> + localities_types[READ_LATENCY] = loc;
> + localities_types[WRITE_LATENCY] = loc;
> + break;
> + case ACPI_HMAT_READ_LATENCY:
> + localities_types[READ_LATENCY] = loc;
> + break;
> + case ACPI_HMAT_WRITE_LATENCY:
> + localities_types[WRITE_LATENCY] = loc;
> + break;
> + case ACPI_HMAT_ACCESS_BANDWIDTH:
> + localities_types[READ_BANDWIDTH] = loc;
> + localities_types[WRITE_BANDWIDTH] = loc;
> + break;
> + case ACPI_HMAT_READ_BANDWIDTH:
> + localities_types[READ_BANDWIDTH] = loc;
> + break;
> + case ACPI_HMAT_WRITE_BANDWIDTH:
> + localities_types[WRITE_BANDWIDTH] = loc;
> + break;
> + default:
> + break;
> + }
> +}
> +
> static __init int hmat_parse_locality(union acpi_subtable_headers *header,
> const unsigned long end)
> {
> struct acpi_hmat_locality *hmat_loc = (void *)header;
> + struct memory_target *target;
> unsigned int init, targ, total_size, ipds, tpds;
> u32 *inits, *targs, value;
> u16 *entries;
> - u8 type;
> + u8 type, mem_hier;
>
> if (hmat_loc->header.length < sizeof(*hmat_loc)) {
> pr_notice("HMAT: Unexpected locality header length: %d\n",
> @@ -105,6 +268,7 @@ static __init int hmat_parse_locality(union acpi_subtable_headers *header,
> }
>
> type = hmat_loc->data_type;
> + mem_hier = hmat_loc->flags & ACPI_HMAT_MEMORY_HIERARCHY;
> ipds = hmat_loc->number_of_initiator_Pds;
> tpds = hmat_loc->number_of_target_Pds;
> total_size = sizeof(*hmat_loc) + sizeof(*entries) * ipds * tpds +
> @@ -123,6 +287,7 @@ static __init int hmat_parse_locality(union acpi_subtable_headers *header,
> targs = inits + ipds;
> entries = (u16 *)(targs + tpds);
> for (init = 0; init < ipds; init++) {
> + alloc_memory_initiator(inits[init]);
> for (targ = 0; targ < tpds; targ++) {
> value = hmat_normalize(entries[init * tpds + targ],
> hmat_loc->entry_base_unit,
> @@ -130,9 +295,18 @@ static __init int hmat_parse_locality(union acpi_subtable_headers *header,
> pr_info(" Initiator-Target[%d-%d]:%d%s\n",
> inits[init], targs[targ], value,
> hmat_data_type_suffix(type));
> +
> + if (mem_hier == ACPI_HMAT_MEMORY) {
> + target = find_mem_target(targs[targ]);
> + if (target && target->processor_pxm == inits[init])
> + hmat_update_target_access(target, type, value);
> + }
> }
> }
>
> + if (mem_hier == ACPI_HMAT_MEMORY)
> + hmat_add_locality(hmat_loc);
> +
> return 0;
> }
>
> @@ -160,6 +334,7 @@ static int __init hmat_parse_proximity_domain(union acpi_subtable_headers *heade
> const unsigned long end)
> {
> struct acpi_hmat_proximity_domain *p = (void *)header;
> + struct memory_target *target;
>
> if (p->header.length != sizeof(*p)) {
> pr_notice("HMAT: Unexpected address range header length: %d\n",
> @@ -175,6 +350,23 @@ static int __init hmat_parse_proximity_domain(union acpi_subtable_headers *heade
> pr_info("HMAT: Memory Flags:%04x Processor Domain:%d Memory Domain:%d\n",
> p->flags, p->processor_PD, p->memory_PD);
>
> + if (p->flags & ACPI_HMAT_MEMORY_PD_VALID) {
> + target = find_mem_target(p->memory_PD);
> + if (!target) {
> + pr_debug("HMAT: Memory Domain missing from SRAT\n");
> + return -EINVAL;
> + }
> + }
> + if (target && p->flags & ACPI_HMAT_PROCESSOR_PD_VALID) {
> + int p_node = pxm_to_node(p->processor_PD);
> +
> + if (p_node == NUMA_NO_NODE) {
> + pr_debug("HMAT: Invalid Processor Domain\n");
> + return -EINVAL;
> + }
> + target->processor_pxm = p_node;
> + }
> +
> return 0;
> }
>
> @@ -198,6 +390,191 @@ static int __init hmat_parse_subtable(union acpi_subtable_headers *header,
> }
> }
>
> +static __init int srat_parse_mem_affinity(union acpi_subtable_headers *header,
> + const unsigned long end)
> +{
> + struct acpi_srat_mem_affinity *ma = (void *)header;
> +
> + if (!ma)
> + return -EINVAL;
> + if (!(ma->flags & ACPI_SRAT_MEM_ENABLED))
> + return 0;
> + alloc_memory_target(ma->proximity_domain);
> + return 0;
> +}
> +
> +static __init u32 hmat_initiator_perf(struct memory_target *target,
> + struct memory_initiator *initiator,
> + struct acpi_hmat_locality *hmat_loc)
> +{
> + unsigned int ipds, tpds, i, idx = 0, tdx = 0;
> + u32 *inits, *targs;
> + u16 *entries;
> +
> + ipds = hmat_loc->number_of_initiator_Pds;
> + tpds = hmat_loc->number_of_target_Pds;
> + inits = (u32 *)(hmat_loc + 1);
> + targs = inits + ipds;
> + entries = (u16 *)(targs + tpds);
> +
> + for (i = 0; i < ipds; i++) {
> + if (inits[i] == initiator->processor_pxm) {
> + idx = i;
> + break;
> + }
> + }
> +
> + if (i == ipds)
> + return 0;
> +
> + for (i = 0; i < tpds; i++) {
> + if (targs[i] == target->memory_pxm) {
> + tdx = i;
> + break;
> + }
> + }
> + if (i == tpds)
> + return 0;
> +
> + return hmat_normalize(entries[idx * tpds + tdx],
> + hmat_loc->entry_base_unit,
> + hmat_loc->data_type);
> +}
> +
> +static __init bool hmat_update_best(u8 type, u32 value, u32 *best)
> +{
> + bool updated = false;
> +
> + if (!value)
> + return false;
> +
> + switch (type) {
> + case ACPI_HMAT_ACCESS_LATENCY:
> + case ACPI_HMAT_READ_LATENCY:
> + case ACPI_HMAT_WRITE_LATENCY:
> + if (!*best || *best > value) {
> + *best = value;
> + updated = true;
> + }
> + break;
> + case ACPI_HMAT_ACCESS_BANDWIDTH:
> + case ACPI_HMAT_READ_BANDWIDTH:
> + case ACPI_HMAT_WRITE_BANDWIDTH:
> + if (!*best || *best < value) {
> + *best = value;
> + updated = true;
> + }
> + break;
> + }
> +
> + return updated;
> +}
> +
> +static int initiator_cmp(void *priv, struct list_head *a, struct list_head *b)
> +{
> + struct memory_initiator *ia;
> + struct memory_initiator *ib;
> + unsigned long *p_nodes = priv;
> +
> + ia = list_entry(a, struct memory_initiator, node);
> + ib = list_entry(b, struct memory_initiator, node);
> +
> + set_bit(ia->processor_pxm, p_nodes);
> + set_bit(ib->processor_pxm, p_nodes);
> +
> + return ia->processor_pxm - ib->processor_pxm;
> +}
> +
> +static __init void hmat_register_target_initiators(struct memory_target *target)
> +{
> + static DECLARE_BITMAP(p_nodes, MAX_NUMNODES);
> + struct memory_initiator *initiator;
> + unsigned int mem_nid, cpu_nid;
> + struct memory_locality *loc = NULL;
> + u32 best = 0;
> + int i;
> +
> + mem_nid = pxm_to_node(target->memory_pxm);
> + /*
> + * If the Address Range Structure provides a local processor pxm, link
> + * only that one. Otherwise, find the best performance attributes and
> + * register all initiators that match.
> + */
> + if (target->processor_pxm != PXM_INVAL) {
> + cpu_nid = pxm_to_node(target->processor_pxm);
> + register_memory_node_under_compute_node(mem_nid, cpu_nid, 0);
> + return;
> + }
> +
> + if (list_empty(&localities))
> + return;
> +
> + /*
> + * We need the initiator list sorted so we can use bitmap_clear for
> + * previously set initiators when we find a better memory accessor.
> + * We'll also use the sorting to prime the candidate nodes with known
> + * initiators.
> + */
> + bitmap_zero(p_nodes, MAX_NUMNODES);
> + list_sort(p_nodes, &initiators, initiator_cmp);
> + for (i = WRITE_LATENCY; i <= READ_BANDWIDTH; i++) {
> + loc = localities_types[i];
> + if (!loc)
> + continue;
> +
> + best = 0;
> + list_for_each_entry(initiator, &initiators, node) {
> + u32 value;
> +
> + if (!test_bit(initiator->processor_pxm, p_nodes))
> + continue;
> +
> + value = hmat_initiator_perf(target, initiator, loc->hmat_loc);
> + if (hmat_update_best(loc->hmat_loc->data_type, value, &best))
> + bitmap_clear(p_nodes, 0, initiator->processor_pxm);
> + if (value != best)
> + clear_bit(initiator->processor_pxm, p_nodes);
> + }
> + if (best)
> + hmat_update_target_access(target, loc->hmat_loc->data_type, best);
> + }
> +
> + for_each_set_bit(i, p_nodes, MAX_NUMNODES) {
> + cpu_nid = pxm_to_node(i);
> + register_memory_node_under_compute_node(mem_nid, cpu_nid, 0);
> + }
> +}
> +
> +static __init void hmat_register_targets(void)
> +{
> + struct memory_target *target;
> +
> + list_for_each_entry(target, &targets, node)
> + hmat_register_target_initiators(target);
> +}
> +
> +static __init void hmat_free_structures(void)
> +{
> + struct memory_target *target, *tnext;
> + struct memory_locality *loc, *lnext;
> + struct memory_initiator *initiator, *inext;
> +
> + list_for_each_entry_safe(target, tnext, &targets, node) {
> + list_del(&target->node);
> + kfree(target);
> + }
> +
> + list_for_each_entry_safe(initiator, inext, &initiators, node) {
> + list_del(&initiator->node);
> + kfree(initiator);
> + }
> +
> + list_for_each_entry_safe(loc, lnext, &localities, node) {
> + list_del(&loc->node);
> + kfree(loc);
> + }
> +}
> +
> static __init int hmat_init(void)
> {
> struct acpi_table_header *tbl;
> @@ -207,6 +584,17 @@ static __init int hmat_init(void)
> if (srat_disabled())
> return 0;
>
> + status = acpi_get_table(ACPI_SIG_SRAT, 0, &tbl);
> + if (ACPI_FAILURE(status))
> + return 0;
> +
> + if (acpi_table_parse_entries(ACPI_SIG_SRAT,
> + sizeof(struct acpi_table_srat),
> + ACPI_SRAT_TYPE_MEMORY_AFFINITY,
> + srat_parse_mem_affinity, 0) < 0)
> + goto out_put;
> + acpi_put_table(tbl);
> +
> status = acpi_get_table(ACPI_SIG_HMAT, 0, &tbl);
> if (ACPI_FAILURE(status))
> return 0;
> @@ -229,7 +617,9 @@ static __init int hmat_init(void)
> goto out_put;
> }
> }
> + hmat_register_targets();
> out_put:
> + hmat_free_structures();
> acpi_put_table(tbl);
> return 0;
> }
> --
> 2.14.4
>
^ permalink raw reply
* Re: [PATCHv8 06/10] node: Add memory-side caching attributes
From: Rafael J. Wysocki @ 2019-03-13 23:18 UTC (permalink / raw)
To: Keith Busch
Cc: Linux Kernel Mailing List, ACPI Devel Maling List,
Linux Memory Management List, Linux API, Greg Kroah-Hartman,
Rafael Wysocki, Dave Hansen, Dan Williams, Jonathan Cameron,
Brice Goglin
In-Reply-To: <20190311205606.11228-7-keith.busch@intel.com>
On Mon, Mar 11, 2019 at 9:55 PM Keith Busch <keith.busch@intel.com> wrote:
>
> System memory may have caches to help improve access speed to frequently
> requested address ranges. While the system provided cache is transparent
> to the software accessing these memory ranges, applications can optimize
> their own access based on cache attributes.
>
> Provide a new API for the kernel to register these memory-side caches
> under the memory node that provides it.
>
> The new sysfs representation is modeled from the existing cpu cacheinfo
> attributes, as seen from /sys/devices/system/cpu/<cpu>/cache/. Unlike CPU
> cacheinfo though, the node cache level is reported from the view of the
> memory. A higher level number is nearer to the CPU, while lower levels
> are closer to the last level memory.
>
> The exported attributes are the cache size, the line size, associativity
> indexing, and write back policy, and add the attributes for the system
> memory caches to sysfs stable documentation.
>
> Signed-off-by: Keith Busch <keith.busch@intel.com>
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> Documentation/ABI/stable/sysfs-devices-node | 34 +++++++
> drivers/base/node.c | 151 ++++++++++++++++++++++++++++
> include/linux/node.h | 39 +++++++
> 3 files changed, 224 insertions(+)
>
> diff --git a/Documentation/ABI/stable/sysfs-devices-node b/Documentation/ABI/stable/sysfs-devices-node
> index 735a40a3f9b2..f7ce68fbd4b9 100644
> --- a/Documentation/ABI/stable/sysfs-devices-node
> +++ b/Documentation/ABI/stable/sysfs-devices-node
> @@ -142,3 +142,37 @@ Contact: Keith Busch <keith.busch@intel.com>
> Description:
> This node's write latency in nanoseconds when access
> from nodes found in this class's linked initiators.
> +
> +What: /sys/devices/system/node/nodeX/memory_side_cache/indexY/
> +Date: December 2018
> +Contact: Keith Busch <keith.busch@intel.com>
> +Description:
> + The directory containing attributes for the memory-side cache
> + level 'Y'.
> +
> +What: /sys/devices/system/node/nodeX/memory_side_cache/indexY/indexing
> +Date: December 2018
> +Contact: Keith Busch <keith.busch@intel.com>
> +Description:
> + The caches associativity indexing: 0 for direct mapped,
> + non-zero if indexed.
> +
> +What: /sys/devices/system/node/nodeX/memory_side_cache/indexY/line_size
> +Date: December 2018
> +Contact: Keith Busch <keith.busch@intel.com>
> +Description:
> + The number of bytes accessed from the next cache level on a
> + cache miss.
> +
> +What: /sys/devices/system/node/nodeX/memory_side_cache/indexY/size
> +Date: December 2018
> +Contact: Keith Busch <keith.busch@intel.com>
> +Description:
> + The size of this memory side cache in bytes.
> +
> +What: /sys/devices/system/node/nodeX/memory_side_cache/indexY/write_policy
> +Date: December 2018
> +Contact: Keith Busch <keith.busch@intel.com>
> +Description:
> + The cache write policy: 0 for write-back, 1 for write-through,
> + other or unknown.
> diff --git a/drivers/base/node.c b/drivers/base/node.c
> index 2de546a040a5..8598fcbd2a17 100644
> --- a/drivers/base/node.c
> +++ b/drivers/base/node.c
> @@ -205,6 +205,155 @@ void node_set_perf_attrs(unsigned int nid, struct node_hmem_attrs *hmem_attrs,
> }
> }
> }
> +
> +/**
> + * struct node_cache_info - Internal tracking for memory node caches
> + * @dev: Device represeting the cache level
> + * @node: List element for tracking in the node
> + * @cache_attrs:Attributes for this cache level
> + */
> +struct node_cache_info {
> + struct device dev;
> + struct list_head node;
> + struct node_cache_attrs cache_attrs;
> +};
> +#define to_cache_info(device) container_of(device, struct node_cache_info, dev)
> +
> +#define CACHE_ATTR(name, fmt) \
> +static ssize_t name##_show(struct device *dev, \
> + struct device_attribute *attr, \
> + char *buf) \
> +{ \
> + return sprintf(buf, fmt "\n", to_cache_info(dev)->cache_attrs.name);\
> +} \
> +DEVICE_ATTR_RO(name);
> +
> +CACHE_ATTR(size, "%llu")
> +CACHE_ATTR(line_size, "%u")
> +CACHE_ATTR(indexing, "%u")
> +CACHE_ATTR(write_policy, "%u")
> +
> +static struct attribute *cache_attrs[] = {
> + &dev_attr_indexing.attr,
> + &dev_attr_size.attr,
> + &dev_attr_line_size.attr,
> + &dev_attr_write_policy.attr,
> + NULL,
> +};
> +ATTRIBUTE_GROUPS(cache);
> +
> +static void node_cache_release(struct device *dev)
> +{
> + kfree(dev);
> +}
> +
> +static void node_cacheinfo_release(struct device *dev)
> +{
> + struct node_cache_info *info = to_cache_info(dev);
> + kfree(info);
> +}
> +
> +static void node_init_cache_dev(struct node *node)
> +{
> + struct device *dev;
> +
> + dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> + if (!dev)
> + return;
> +
> + dev->parent = &node->dev;
> + dev->release = node_cache_release;
> + if (dev_set_name(dev, "memory_side_cache"))
> + goto free_dev;
> +
> + if (device_register(dev))
> + goto free_name;
> +
> + pm_runtime_no_callbacks(dev);
> + node->cache_dev = dev;
> + return;
> +free_name:
> + kfree_const(dev->kobj.name);
> +free_dev:
> + kfree(dev);
> +}
> +
> +/**
> + * node_add_cache() - add cache attribute to a memory node
> + * @nid: Node identifier that has new cache attributes
> + * @cache_attrs: Attributes for the cache being added
> + */
> +void node_add_cache(unsigned int nid, struct node_cache_attrs *cache_attrs)
> +{
> + struct node_cache_info *info;
> + struct device *dev;
> + struct node *node;
> +
> + if (!node_online(nid) || !node_devices[nid])
> + return;
> +
> + node = node_devices[nid];
> + list_for_each_entry(info, &node->cache_attrs, node) {
> + if (info->cache_attrs.level == cache_attrs->level) {
> + dev_warn(&node->dev,
> + "attempt to add duplicate cache level:%d\n",
> + cache_attrs->level);
> + return;
> + }
> + }
> +
> + if (!node->cache_dev)
> + node_init_cache_dev(node);
> + if (!node->cache_dev)
> + return;
> +
> + info = kzalloc(sizeof(*info), GFP_KERNEL);
> + if (!info)
> + return;
> +
> + dev = &info->dev;
> + dev->parent = node->cache_dev;
> + dev->release = node_cacheinfo_release;
> + dev->groups = cache_groups;
> + if (dev_set_name(dev, "index%d", cache_attrs->level))
> + goto free_cache;
> +
> + info->cache_attrs = *cache_attrs;
> + if (device_register(dev)) {
> + dev_warn(&node->dev, "failed to add cache level:%d\n",
> + cache_attrs->level);
> + goto free_name;
> + }
> + pm_runtime_no_callbacks(dev);
> + list_add_tail(&info->node, &node->cache_attrs);
> + return;
> +free_name:
> + kfree_const(dev->kobj.name);
> +free_cache:
> + kfree(info);
> +}
> +
> +static void node_remove_caches(struct node *node)
> +{
> + struct node_cache_info *info, *next;
> +
> + if (!node->cache_dev)
> + return;
> +
> + list_for_each_entry_safe(info, next, &node->cache_attrs, node) {
> + list_del(&info->node);
> + device_unregister(&info->dev);
> + }
> + device_unregister(node->cache_dev);
> +}
> +
> +static void node_init_caches(unsigned int nid)
> +{
> + INIT_LIST_HEAD(&node_devices[nid]->cache_attrs);
> +}
> +#else
> +static void node_init_caches(unsigned int nid) { }
> +static void node_remove_caches(struct node *node) { }
> #endif
>
> #define K(x) ((x) << (PAGE_SHIFT - 10))
> @@ -489,6 +638,7 @@ void unregister_node(struct node *node)
> {
> hugetlb_unregister_node(node); /* no-op, if memoryless node */
> node_remove_accesses(node);
> + node_remove_caches(node);
> device_unregister(&node->dev);
> }
>
> @@ -781,6 +931,7 @@ int __register_one_node(int nid)
> INIT_LIST_HEAD(&node_devices[nid]->access_list);
> /* initialize work queue for memory hot plug */
> init_node_hugetlb_work(nid);
> + node_init_caches(nid);
>
> return error;
> }
> diff --git a/include/linux/node.h b/include/linux/node.h
> index 4139d728f8b3..1a557c589ecb 100644
> --- a/include/linux/node.h
> +++ b/include/linux/node.h
> @@ -35,10 +35,45 @@ struct node_hmem_attrs {
> unsigned int write_latency;
> };
>
> +enum cache_indexing {
> + NODE_CACHE_DIRECT_MAP,
> + NODE_CACHE_INDEXED,
> + NODE_CACHE_OTHER,
> +};
> +
> +enum cache_write_policy {
> + NODE_CACHE_WRITE_BACK,
> + NODE_CACHE_WRITE_THROUGH,
> + NODE_CACHE_WRITE_OTHER,
> +};
> +
> +/**
> + * struct node_cache_attrs - system memory caching attributes
> + *
> + * @indexing: The ways memory blocks may be placed in cache
> + * @write_policy: Write back or write through policy
> + * @size: Total size of cache in bytes
> + * @line_size: Number of bytes fetched on a cache miss
> + * @level: The cache hierarchy level
> + */
> +struct node_cache_attrs {
> + enum cache_indexing indexing;
> + enum cache_write_policy write_policy;
> + u64 size;
> + u16 line_size;
> + u8 level;
> +};
> +
> #ifdef CONFIG_HMEM_REPORTING
> +void node_add_cache(unsigned int nid, struct node_cache_attrs *cache_attrs);
> void node_set_perf_attrs(unsigned int nid, struct node_hmem_attrs *hmem_attrs,
> unsigned access);
> #else
> +static inline void node_add_cache(unsigned int nid,
> + struct node_cache_attrs *cache_attrs)
> +{
> +}
> +
> static inline void node_set_perf_attrs(unsigned int nid,
> struct node_hmem_attrs *hmem_attrs,
> unsigned access)
> @@ -53,6 +88,10 @@ struct node {
> #if defined(CONFIG_MEMORY_HOTPLUG_SPARSE) && defined(CONFIG_HUGETLBFS)
> struct work_struct node_work;
> #endif
> +#ifdef CONFIG_HMEM_REPORTING
> + struct list_head cache_attrs;
> + struct device *cache_dev;
> +#endif
> };
>
> struct memory_block;
> --
> 2.14.4
>
^ permalink raw reply
* Re: [PATCHv8 05/10] node: Add heterogenous memory access attributes
From: Rafael J. Wysocki @ 2019-03-13 23:15 UTC (permalink / raw)
To: Keith Busch
Cc: Linux Kernel Mailing List, ACPI Devel Maling List,
Linux Memory Management List, Linux API, Greg Kroah-Hartman,
Rafael Wysocki, Dave Hansen, Dan Williams, Jonathan Cameron,
Brice Goglin
In-Reply-To: <20190311205606.11228-6-keith.busch@intel.com>
On Mon, Mar 11, 2019 at 9:55 PM Keith Busch <keith.busch@intel.com> wrote:
>
> Heterogeneous memory systems provide memory nodes with different latency
> and bandwidth performance attributes. Provide a new kernel interface
> for subsystems to register the attributes under the memory target
> node's initiator access class. If the system provides this information,
> applications may query these attributes when deciding which node to
> request memory.
>
> The following example shows the new sysfs hierarchy for a node exporting
> performance attributes:
>
> # tree -P "read*|write*"/sys/devices/system/node/nodeY/accessZ/initiators/
> /sys/devices/system/node/nodeY/accessZ/initiators/
> |-- read_bandwidth
> |-- read_latency
> |-- write_bandwidth
> `-- write_latency
>
> The bandwidth is exported as MB/s and latency is reported in
> nanoseconds. The values are taken from the platform as reported by the
> manufacturer.
>
> Memory accesses from an initiator node that is not one of the memory's
> access "Z" initiator nodes linked in the same directory may observe
> different performance than reported here. When a subsystem makes use
> of this interface, initiators of a different access number may not have
> the same performance relative to initiators in other access numbers, or
> omitted from the any access class' initiators.
>
> Descriptions for memory access initiator performance access attributes
> are added to sysfs stable documentation.
>
> Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Tested-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Keith Busch <keith.busch@intel.com>
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> Documentation/ABI/stable/sysfs-devices-node | 28 ++++++++++++++
> drivers/base/Kconfig | 8 ++++
> drivers/base/node.c | 59 +++++++++++++++++++++++++++++
> include/linux/node.h | 26 +++++++++++++
> 4 files changed, 121 insertions(+)
>
> diff --git a/Documentation/ABI/stable/sysfs-devices-node b/Documentation/ABI/stable/sysfs-devices-node
> index 433bcc04e542..735a40a3f9b2 100644
> --- a/Documentation/ABI/stable/sysfs-devices-node
> +++ b/Documentation/ABI/stable/sysfs-devices-node
> @@ -114,3 +114,31 @@ Contact: Keith Busch <keith.busch@intel.com>
> Description:
> The directory containing symlinks to memory targets that
> this initiator node has class "Y" access.
> +
> +What: /sys/devices/system/node/nodeX/accessY/initiators/read_bandwidth
> +Date: December 2018
> +Contact: Keith Busch <keith.busch@intel.com>
> +Description:
> + This node's read bandwidth in MB/s when accessed from
> + nodes found in this access class's linked initiators.
> +
> +What: /sys/devices/system/node/nodeX/accessY/initiators/read_latency
> +Date: December 2018
> +Contact: Keith Busch <keith.busch@intel.com>
> +Description:
> + This node's read latency in nanoseconds when accessed
> + from nodes found in this access class's linked initiators.
> +
> +What: /sys/devices/system/node/nodeX/accessY/initiators/write_bandwidth
> +Date: December 2018
> +Contact: Keith Busch <keith.busch@intel.com>
> +Description:
> + This node's write bandwidth in MB/s when accessed from
> + found in this access class's linked initiators.
> +
> +What: /sys/devices/system/node/nodeX/accessY/initiators/write_latency
> +Date: December 2018
> +Contact: Keith Busch <keith.busch@intel.com>
> +Description:
> + This node's write latency in nanoseconds when access
> + from nodes found in this class's linked initiators.
> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> index 059700ea3521..a7438a58c250 100644
> --- a/drivers/base/Kconfig
> +++ b/drivers/base/Kconfig
> @@ -149,6 +149,14 @@ config DEBUG_TEST_DRIVER_REMOVE
> unusable. You should say N here unless you are explicitly looking to
> test this functionality.
>
> +config HMEM_REPORTING
> + bool
> + default n
> + depends on NUMA
> + help
> + Enable reporting for heterogenous memory access attributes under
> + their non-uniform memory nodes.
> +
> source "drivers/base/test/Kconfig"
>
> config SYS_HYPERVISOR
> diff --git a/drivers/base/node.c b/drivers/base/node.c
> index 6f4097680580..2de546a040a5 100644
> --- a/drivers/base/node.c
> +++ b/drivers/base/node.c
> @@ -71,6 +71,9 @@ struct node_access_nodes {
> struct device dev;
> struct list_head list_node;
> unsigned access;
> +#ifdef CONFIG_HMEM_REPORTING
> + struct node_hmem_attrs hmem_attrs;
> +#endif
> };
> #define to_access_nodes(dev) container_of(dev, struct node_access_nodes, dev)
>
> @@ -148,6 +151,62 @@ static struct node_access_nodes *node_init_node_access(struct node *node,
> return NULL;
> }
>
> +#ifdef CONFIG_HMEM_REPORTING
> +#define ACCESS_ATTR(name) \
> +static ssize_t name##_show(struct device *dev, \
> + struct device_attribute *attr, \
> + char *buf) \
> +{ \
> + return sprintf(buf, "%u\n", to_access_nodes(dev)->hmem_attrs.name); \
> +} \
> +static DEVICE_ATTR_RO(name);
> +
> +ACCESS_ATTR(read_bandwidth)
> +ACCESS_ATTR(read_latency)
> +ACCESS_ATTR(write_bandwidth)
> +ACCESS_ATTR(write_latency)
> +
> +static struct attribute *access_attrs[] = {
> + &dev_attr_read_bandwidth.attr,
> + &dev_attr_read_latency.attr,
> + &dev_attr_write_bandwidth.attr,
> + &dev_attr_write_latency.attr,
> + NULL,
> +};
> +
> +/**
> + * node_set_perf_attrs - Set the performance values for given access class
> + * @nid: Node identifier to be set
> + * @hmem_attrs: Heterogeneous memory performance attributes
> + * @access: The access class the for the given attributes
> + */
> +void node_set_perf_attrs(unsigned int nid, struct node_hmem_attrs *hmem_attrs,
> + unsigned access)
> +{
> + struct node_access_nodes *c;
> + struct node *node;
> + int i;
> +
> + if (WARN_ON_ONCE(!node_online(nid)))
> + return;
> +
> + node = node_devices[nid];
> + c = node_init_node_access(node, access);
> + if (!c)
> + return;
> +
> + c->hmem_attrs = *hmem_attrs;
> + for (i = 0; access_attrs[i] != NULL; i++) {
> + if (sysfs_add_file_to_group(&c->dev.kobj, access_attrs[i],
> + "initiators")) {
> + pr_info("failed to add performance attribute to node %d\n",
> + nid);
> + break;
> + }
> + }
> +}
> +#endif
> +
> #define K(x) ((x) << (PAGE_SHIFT - 10))
> static ssize_t node_read_meminfo(struct device *dev,
> struct device_attribute *attr, char *buf)
> diff --git a/include/linux/node.h b/include/linux/node.h
> index bb288817ed33..4139d728f8b3 100644
> --- a/include/linux/node.h
> +++ b/include/linux/node.h
> @@ -20,6 +20,32 @@
> #include <linux/list.h>
> #include <linux/workqueue.h>
>
> +/**
> + * struct node_hmem_attrs - heterogeneous memory performance attributes
> + *
> + * @read_bandwidth: Read bandwidth in MB/s
> + * @write_bandwidth: Write bandwidth in MB/s
> + * @read_latency: Read latency in nanoseconds
> + * @write_latency: Write latency in nanoseconds
> + */
> +struct node_hmem_attrs {
> + unsigned int read_bandwidth;
> + unsigned int write_bandwidth;
> + unsigned int read_latency;
> + unsigned int write_latency;
> +};
> +
> +#ifdef CONFIG_HMEM_REPORTING
> +void node_set_perf_attrs(unsigned int nid, struct node_hmem_attrs *hmem_attrs,
> + unsigned access);
> +#else
> +static inline void node_set_perf_attrs(unsigned int nid,
> + struct node_hmem_attrs *hmem_attrs,
> + unsigned access)
> +{
> +}
> +#endif
> +
> struct node {
> struct device dev;
> struct list_head access_list;
> --
> 2.14.4
>
^ permalink raw reply
* Re: [PATCHv8 04/10] node: Link memory nodes to their compute nodes
From: Rafael J. Wysocki @ 2019-03-13 23:13 UTC (permalink / raw)
To: Keith Busch
Cc: Linux Kernel Mailing List, ACPI Devel Maling List,
Linux Memory Management List, Linux API, Greg Kroah-Hartman,
Rafael Wysocki, Dave Hansen, Dan Williams, Jonathan Cameron,
Brice Goglin
In-Reply-To: <20190311205606.11228-5-keith.busch@intel.com>
On Mon, Mar 11, 2019 at 9:55 PM Keith Busch <keith.busch@intel.com> wrote:
>
> Systems may be constructed with various specialized nodes. Some nodes
> may provide memory, some provide compute devices that access and use
> that memory, and others may provide both. Nodes that provide memory are
> referred to as memory targets, and nodes that can initiate memory access
> are referred to as memory initiators.
>
> Memory targets will often have varying access characteristics from
> different initiators, and platforms may have ways to express those
> relationships. In preparation for these systems, provide interfaces for
> the kernel to export the memory relationship among different nodes memory
> targets and their initiators with symlinks to each other.
>
> If a system provides access locality for each initiator-target pair, nodes
> may be grouped into ranked access classes relative to other nodes. The
> new interface allows a subsystem to register relationships of varying
> classes if available and desired to be exported.
>
> A memory initiator may have multiple memory targets in the same access
> class. The target memory's initiators in a given class indicate the
> nodes access characteristics share the same performance relative to other
> linked initiator nodes. Each target within an initiator's access class,
> though, do not necessarily perform the same as each other.
>
> A memory target node may have multiple memory initiators. All linked
> initiators in a target's class have the same access characteristics to
> that target.
>
> The following example show the nodes' new sysfs hierarchy for a memory
> target node 'Y' with access class 0 from initiator node 'X':
>
> # symlinks -v /sys/devices/system/node/nodeX/access0/
> relative: /sys/devices/system/node/nodeX/access0/targets/nodeY -> ../../nodeY
>
> # symlinks -v /sys/devices/system/node/nodeY/access0/
> relative: /sys/devices/system/node/nodeY/access0/initiators/nodeX -> ../../nodeX
>
> The new attributes are added to the sysfs stable documentation.
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Keith Busch <keith.busch@intel.com>
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> Documentation/ABI/stable/sysfs-devices-node | 25 ++++-
> drivers/base/node.c | 142 +++++++++++++++++++++++++++-
> include/linux/node.h | 6 ++
> 3 files changed, 171 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/ABI/stable/sysfs-devices-node b/Documentation/ABI/stable/sysfs-devices-node
> index 3e90e1f3bf0a..433bcc04e542 100644
> --- a/Documentation/ABI/stable/sysfs-devices-node
> +++ b/Documentation/ABI/stable/sysfs-devices-node
> @@ -90,4 +90,27 @@ Date: December 2009
> Contact: Lee Schermerhorn <lee.schermerhorn@hp.com>
> Description:
> The node's huge page size control/query attributes.
> - See Documentation/admin-guide/mm/hugetlbpage.rst
> \ No newline at end of file
> + See Documentation/admin-guide/mm/hugetlbpage.rst
> +
> +What: /sys/devices/system/node/nodeX/accessY/
> +Date: December 2018
> +Contact: Keith Busch <keith.busch@intel.com>
> +Description:
> + The node's relationship to other nodes for access class "Y".
> +
> +What: /sys/devices/system/node/nodeX/accessY/initiators/
> +Date: December 2018
> +Contact: Keith Busch <keith.busch@intel.com>
> +Description:
> + The directory containing symlinks to memory initiator
> + nodes that have class "Y" access to this target node's
> + memory. CPUs and other memory initiators in nodes not in
> + the list accessing this node's memory may have different
> + performance.
> +
> +What: /sys/devices/system/node/nodeX/accessY/targets/
> +Date: December 2018
> +Contact: Keith Busch <keith.busch@intel.com>
> +Description:
> + The directory containing symlinks to memory targets that
> + this initiator node has class "Y" access.
> diff --git a/drivers/base/node.c b/drivers/base/node.c
> index 86d6cd92ce3d..6f4097680580 100644
> --- a/drivers/base/node.c
> +++ b/drivers/base/node.c
> @@ -17,6 +17,7 @@
> #include <linux/nodemask.h>
> #include <linux/cpu.h>
> #include <linux/device.h>
> +#include <linux/pm_runtime.h>
> #include <linux/swap.h>
> #include <linux/slab.h>
>
> @@ -59,6 +60,94 @@ static inline ssize_t node_read_cpulist(struct device *dev,
> static DEVICE_ATTR(cpumap, S_IRUGO, node_read_cpumask, NULL);
> static DEVICE_ATTR(cpulist, S_IRUGO, node_read_cpulist, NULL);
>
> +/**
> + * struct node_access_nodes - Access class device to hold user visible
> + * relationships to other nodes.
> + * @dev: Device for this memory access class
> + * @list_node: List element in the node's access list
> + * @access: The access class rank
> + */
> +struct node_access_nodes {
> + struct device dev;
> + struct list_head list_node;
> + unsigned access;
> +};
> +#define to_access_nodes(dev) container_of(dev, struct node_access_nodes, dev)
> +
> +static struct attribute *node_init_access_node_attrs[] = {
> + NULL,
> +};
> +
> +static struct attribute *node_targ_access_node_attrs[] = {
> + NULL,
> +};
> +
> +static const struct attribute_group initiators = {
> + .name = "initiators",
> + .attrs = node_init_access_node_attrs,
> +};
> +
> +static const struct attribute_group targets = {
> + .name = "targets",
> + .attrs = node_targ_access_node_attrs,
> +};
> +
> +static const struct attribute_group *node_access_node_groups[] = {
> + &initiators,
> + &targets,
> + NULL,
> +};
> +
> +static void node_remove_accesses(struct node *node)
> +{
> + struct node_access_nodes *c, *cnext;
> +
> + list_for_each_entry_safe(c, cnext, &node->access_list, list_node) {
> + list_del(&c->list_node);
> + device_unregister(&c->dev);
> + }
> +}
> +
> +static void node_access_release(struct device *dev)
> +{
> + kfree(to_access_nodes(dev));
> +}
> +
> +static struct node_access_nodes *node_init_node_access(struct node *node,
> + unsigned access)
> +{
> + struct node_access_nodes *access_node;
> + struct device *dev;
> +
> + list_for_each_entry(access_node, &node->access_list, list_node)
> + if (access_node->access == access)
> + return access_node;
> +
> + access_node = kzalloc(sizeof(*access_node), GFP_KERNEL);
> + if (!access_node)
> + return NULL;
> +
> + access_node->access = access;
> + dev = &access_node->dev;
> + dev->parent = &node->dev;
> + dev->release = node_access_release;
> + dev->groups = node_access_node_groups;
> + if (dev_set_name(dev, "access%u", access))
> + goto free;
> +
> + if (device_register(dev))
> + goto free_name;
> +
> + pm_runtime_no_callbacks(dev);
> + list_add_tail(&access_node->list_node, &node->access_list);
> + return access_node;
> +free_name:
> + kfree_const(dev->kobj.name);
> +free:
> + kfree(access_node);
> + return NULL;
> +}
> +
> #define K(x) ((x) << (PAGE_SHIFT - 10))
> static ssize_t node_read_meminfo(struct device *dev,
> struct device_attribute *attr, char *buf)
> @@ -340,7 +429,7 @@ static int register_node(struct node *node, int num)
> void unregister_node(struct node *node)
> {
> hugetlb_unregister_node(node); /* no-op, if memoryless node */
> -
> + node_remove_accesses(node);
> device_unregister(&node->dev);
> }
>
> @@ -372,6 +461,56 @@ int register_cpu_under_node(unsigned int cpu, unsigned int nid)
> kobject_name(&node_devices[nid]->dev.kobj));
> }
>
> +/**
> + * register_memory_node_under_compute_node - link memory node to its compute
> + * node for a given access class.
> + * @mem_node: Memory node number
> + * @cpu_node: Cpu node number
> + * @access: Access class to register
> + *
> + * Description:
> + * For use with platforms that may have separate memory and compute nodes.
> + * This function will export node relationships linking which memory
> + * initiator nodes can access memory targets at a given ranked access
> + * class.
> + */
> +int register_memory_node_under_compute_node(unsigned int mem_nid,
> + unsigned int cpu_nid,
> + unsigned access)
> +{
> + struct node *init_node, *targ_node;
> + struct node_access_nodes *initiator, *target;
> + int ret;
> +
> + if (!node_online(cpu_nid) || !node_online(mem_nid))
> + return -ENODEV;
> +
> + init_node = node_devices[cpu_nid];
> + targ_node = node_devices[mem_nid];
> + initiator = node_init_node_access(init_node, access);
> + target = node_init_node_access(targ_node, access);
> + if (!initiator || !target)
> + return -ENOMEM;
> +
> + ret = sysfs_add_link_to_group(&initiator->dev.kobj, "targets",
> + &targ_node->dev.kobj,
> + dev_name(&targ_node->dev));
> + if (ret)
> + return ret;
> +
> + ret = sysfs_add_link_to_group(&target->dev.kobj, "initiators",
> + &init_node->dev.kobj,
> + dev_name(&init_node->dev));
> + if (ret)
> + goto err;
> +
> + return 0;
> + err:
> + sysfs_remove_link_from_group(&initiator->dev.kobj, "targets",
> + dev_name(&targ_node->dev));
> + return ret;
> +}
> +
> int unregister_cpu_under_node(unsigned int cpu, unsigned int nid)
> {
> struct device *obj;
> @@ -580,6 +719,7 @@ int __register_one_node(int nid)
> register_cpu_under_node(cpu, nid);
> }
>
> + INIT_LIST_HEAD(&node_devices[nid]->access_list);
> /* initialize work queue for memory hot plug */
> init_node_hugetlb_work(nid);
>
> diff --git a/include/linux/node.h b/include/linux/node.h
> index 257bb3d6d014..bb288817ed33 100644
> --- a/include/linux/node.h
> +++ b/include/linux/node.h
> @@ -17,10 +17,12 @@
>
> #include <linux/device.h>
> #include <linux/cpumask.h>
> +#include <linux/list.h>
> #include <linux/workqueue.h>
>
> struct node {
> struct device dev;
> + struct list_head access_list;
>
> #if defined(CONFIG_MEMORY_HOTPLUG_SPARSE) && defined(CONFIG_HUGETLBFS)
> struct work_struct node_work;
> @@ -75,6 +77,10 @@ extern int register_mem_sect_under_node(struct memory_block *mem_blk,
> extern int unregister_mem_sect_under_nodes(struct memory_block *mem_blk,
> unsigned long phys_index);
>
> +extern int register_memory_node_under_compute_node(unsigned int mem_nid,
> + unsigned int cpu_nid,
> + unsigned access);
> +
> #ifdef CONFIG_HUGETLBFS
> extern void register_hugetlbfs_with_node(node_registration_func_t doregister,
> node_registration_func_t unregister);
> --
> 2.14.4
>
^ permalink raw reply
* Re: [PATCH v7 01/15] sched/core: uclamp: Add CPU's clamp buckets refcounting
From: Suren Baghdasaryan @ 2019-03-13 21:32 UTC (permalink / raw)
To: Patrick Bellasi
Cc: LKML, linux-pm, linux-api, Ingo Molnar, Peter Zijlstra, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle
In-Reply-To: <20190208100554.32196-2-patrick.bellasi@arm.com>
On Fri, Feb 8, 2019 at 2:06 AM Patrick Bellasi <patrick.bellasi@arm.com> wrote:
>
> Utilization clamping allows to clamp the CPU's utilization within a
> [util_min, util_max] range, depending on the set of RUNNABLE tasks on
> that CPU. Each task references two "clamp buckets" defining its minimum
> and maximum (util_{min,max}) utilization "clamp values". A CPU's clamp
> bucket is active if there is at least one RUNNABLE tasks enqueued on
> that CPU and refcounting that bucket.
>
> When a task is {en,de}queued {on,from} a rq, the set of active clamp
> buckets on that CPU can change. Since each clamp bucket enforces a
> different utilization clamp value, when the set of active clamp buckets
> changes, a new "aggregated" clamp value is computed for that CPU.
>
> Clamp values are always MAX aggregated for both util_min and util_max.
> This ensures that no tasks can affect the performance of other
> co-scheduled tasks which are more boosted (i.e. with higher util_min
> clamp) or less capped (i.e. with higher util_max clamp).
>
> Each task has a:
> task_struct::uclamp[clamp_id]::bucket_id
> to track the "bucket index" of the CPU's clamp bucket it refcounts while
> enqueued, for each clamp index (clamp_id).
>
> Each CPU's rq has a:
> rq::uclamp[clamp_id]::bucket[bucket_id].tasks
> to track how many tasks, currently RUNNABLE on that CPU, refcount each
> clamp bucket (bucket_id) of a clamp index (clamp_id).
>
> Each CPU's rq has also a:
> rq::uclamp[clamp_id]::bucket[bucket_id].value
> to track the clamp value of each clamp bucket (bucket_id) of a clamp
> index (clamp_id).
>
> The rq::uclamp::bucket[clamp_id][] array is scanned every time we need
> to find a new MAX aggregated clamp value for a clamp_id. This operation
> is required only when we dequeue the last task of a clamp bucket
> tracking the current MAX aggregated clamp value. In these cases, the CPU
> is either entering IDLE or going to schedule a less boosted or more
> clamped task.
> The expected number of different clamp values, configured at build time,
> is small enough to fit the full unordered array into a single cache
> line.
I assume you are talking about "struct uclamp_rq uclamp[UCLAMP_CNT]"
here. uclamp_rq size depends on UCLAMP_BUCKETS configurable to be up
to 20. sizeof(long)*20 is already more than 64 bytes. What am I
missing?
> Add the basic data structures required to refcount, in each CPU's rq,
> the number of RUNNABLE tasks for each clamp bucket. Add also the max
> aggregation required to update the rq's clamp value at each
> enqueue/dequeue event.
>
> Use a simple linear mapping of clamp values into clamp buckets.
> Pre-compute and cache bucket_id to avoid integer divisions at
> enqueue/dequeue time.
>
> Signed-off-by: Patrick Bellasi <patrick.bellasi@arm.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
>
> ---
> Changes in v7:
> Message-ID: <20190123191007.GG17749@hirez.programming.kicks-ass.net>
> - removed buckets mapping code
> - use a simpler linear mapping of clamp values into buckets
> Message-ID: <20190124161443.lv2pw5fsspyelckq@e110439-lin>
> - move this patch at the beginning of the series,
> in the attempt to make the overall series easier to digest by moving
> at the very beginning the core bits and main data structures
> Others:
> - update the mapping logic to use exactly and only
> UCLAMP_BUCKETS_COUNT buckets, i.e. no more "special" bucket
> - update uclamp_rq_update() to do top-bottom max search
> ---
> include/linux/log2.h | 37 ++++++++
> include/linux/sched.h | 39 ++++++++
> include/linux/sched/topology.h | 6 --
> init/Kconfig | 53 +++++++++++
> kernel/sched/core.c | 165 +++++++++++++++++++++++++++++++++
> kernel/sched/sched.h | 59 +++++++++++-
> 6 files changed, 350 insertions(+), 9 deletions(-)
>
> diff --git a/include/linux/log2.h b/include/linux/log2.h
> index 2af7f77866d0..e2db25734532 100644
> --- a/include/linux/log2.h
> +++ b/include/linux/log2.h
> @@ -224,4 +224,41 @@ int __order_base_2(unsigned long n)
> ilog2((n) - 1) + 1) : \
> __order_base_2(n) \
> )
> +
> +static inline __attribute__((const))
> +int __bits_per(unsigned long n)
> +{
> + if (n < 2)
> + return 1;
> + if (is_power_of_2(n))
> + return order_base_2(n) + 1;
> + return order_base_2(n);
> +}
> +
> +/**
> + * bits_per - calculate the number of bits required for the argument
> + * @n: parameter
> + *
> + * This is constant-capable and can be used for compile time
> + * initiaizations, e.g bitfields.
> + *
> + * The first few values calculated by this routine:
> + * bf(0) = 1
> + * bf(1) = 1
> + * bf(2) = 2
> + * bf(3) = 2
> + * bf(4) = 3
> + * ... and so on.
> + */
> +#define bits_per(n) \
> +( \
> + __builtin_constant_p(n) ? ( \
> + ((n) == 0 || (n) == 1) ? 1 : ( \
> + ((n) & (n - 1)) == 0 ? \
> + ilog2((n) - 1) + 2 : \
> + ilog2((n) - 1) + 1 \
> + ) \
> + ) : \
> + __bits_per(n) \
> +)
> #endif /* _LINUX_LOG2_H */
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 4112639c2a85..45460e7a3eee 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -281,6 +281,18 @@ struct vtime {
> u64 gtime;
> };
>
> +/*
> + * Utilization clamp constraints.
> + * @UCLAMP_MIN: Minimum utilization
> + * @UCLAMP_MAX: Maximum utilization
> + * @UCLAMP_CNT: Utilization clamp constraints count
> + */
> +enum uclamp_id {
> + UCLAMP_MIN = 0,
> + UCLAMP_MAX,
> + UCLAMP_CNT
> +};
> +
> struct sched_info {
> #ifdef CONFIG_SCHED_INFO
> /* Cumulative counters: */
> @@ -312,6 +324,10 @@ struct sched_info {
> # define SCHED_FIXEDPOINT_SHIFT 10
> # define SCHED_FIXEDPOINT_SCALE (1L << SCHED_FIXEDPOINT_SHIFT)
>
> +/* Increase resolution of cpu_capacity calculations */
> +# define SCHED_CAPACITY_SHIFT SCHED_FIXEDPOINT_SHIFT
> +# define SCHED_CAPACITY_SCALE (1L << SCHED_CAPACITY_SHIFT)
> +
> struct load_weight {
> unsigned long weight;
> u32 inv_weight;
> @@ -560,6 +576,25 @@ struct sched_dl_entity {
> struct hrtimer inactive_timer;
> };
>
> +#ifdef CONFIG_UCLAMP_TASK
> +/* Number of utilization clamp buckets (shorter alias) */
> +#define UCLAMP_BUCKETS CONFIG_UCLAMP_BUCKETS_COUNT
> +
> +/*
> + * Utilization clamp for a scheduling entity
> + * @value: clamp value "requested" by a se
> + * @bucket_id: clamp bucket corresponding to the "requested" value
> + *
> + * The bucket_id is the index of the clamp bucket matching the clamp value
> + * which is pre-computed and stored to avoid expensive integer divisions from
> + * the fast path.
> + */
> +struct uclamp_se {
> + unsigned int value : bits_per(SCHED_CAPACITY_SCALE);
> + unsigned int bucket_id : bits_per(UCLAMP_BUCKETS);
> +};
> +#endif /* CONFIG_UCLAMP_TASK */
> +
> union rcu_special {
> struct {
> u8 blocked;
> @@ -640,6 +675,10 @@ struct task_struct {
> #endif
> struct sched_dl_entity dl;
>
> +#ifdef CONFIG_UCLAMP_TASK
> + struct uclamp_se uclamp[UCLAMP_CNT];
> +#endif
> +
> #ifdef CONFIG_PREEMPT_NOTIFIERS
> /* List of struct preempt_notifier: */
> struct hlist_head preempt_notifiers;
> diff --git a/include/linux/sched/topology.h b/include/linux/sched/topology.h
> index c31d3a47a47c..04beadac6985 100644
> --- a/include/linux/sched/topology.h
> +++ b/include/linux/sched/topology.h
> @@ -6,12 +6,6 @@
>
> #include <linux/sched/idle.h>
>
> -/*
> - * Increase resolution of cpu_capacity calculations
> - */
> -#define SCHED_CAPACITY_SHIFT SCHED_FIXEDPOINT_SHIFT
> -#define SCHED_CAPACITY_SCALE (1L << SCHED_CAPACITY_SHIFT)
> -
> /*
> * sched-domains (multiprocessor balancing) declarations:
> */
> diff --git a/init/Kconfig b/init/Kconfig
> index 513fa544a134..34e23d5d95d1 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -640,6 +640,59 @@ config HAVE_UNSTABLE_SCHED_CLOCK
> config GENERIC_SCHED_CLOCK
> bool
>
> +menu "Scheduler features"
> +
> +config UCLAMP_TASK
> + bool "Enable utilization clamping for RT/FAIR tasks"
> + depends on CPU_FREQ_GOV_SCHEDUTIL
> + help
> + This feature enables the scheduler to track the clamped utilization
> + of each CPU based on RUNNABLE tasks scheduled on that CPU.
> +
> + With this option, the user can specify the min and max CPU
> + utilization allowed for RUNNABLE tasks. The max utilization defines
> + the maximum frequency a task should use while the min utilization
> + defines the minimum frequency it should use.
> +
> + Both min and max utilization clamp values are hints to the scheduler,
> + aiming at improving its frequency selection policy, but they do not
> + enforce or grant any specific bandwidth for tasks.
> +
> + If in doubt, say N.
> +
> +config UCLAMP_BUCKETS_COUNT
> + int "Number of supported utilization clamp buckets"
> + range 5 20
> + default 5
> + depends on UCLAMP_TASK
> + help
> + Defines the number of clamp buckets to use. The range of each bucket
> + will be SCHED_CAPACITY_SCALE/UCLAMP_BUCKETS_COUNT. The higher the
> + number of clamp buckets the finer their granularity and the higher
> + the precision of clamping aggregation and tracking at run-time.
> +
> + For example, with the default configuration we will have 5 clamp
> + buckets tracking 20% utilization each. A 25% boosted tasks will be
> + refcounted in the [20..39]% bucket and will set the bucket clamp
> + effective value to 25%.
> + If a second 30% boosted task should be co-scheduled on the same CPU,
> + that task will be refcounted in the same bucket of the first task and
> + it will boost the bucket clamp effective value to 30%.
> + The clamp effective value of a bucket is reset to its nominal value
> + (20% in the example above) when there are anymore tasks refcounted in
> + that bucket.
> +
> + An additional boost/capping margin can be added to some tasks. In the
> + example above the 25% task will be boosted to 30% until it exits the
> + CPU. If that should be considered not acceptable on certain systems,
> + it's always possible to reduce the margin by increasing the number of
> + clamp buckets to trade off used memory for run-time tracking
> + precision.
> +
> + If in doubt, use the default value.
> +
> +endmenu
> +
> #
> # For architectures that want to enable the support for NUMA-affine scheduler
> # balancing logic:
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index ec1b67a195cc..8ecf5470058c 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -719,6 +719,167 @@ static void set_load_weight(struct task_struct *p, bool update_load)
> }
> }
>
> +#ifdef CONFIG_UCLAMP_TASK
> +
> +/* Integer ceil-rounded range for each bucket */
> +#define UCLAMP_BUCKET_DELTA ((SCHED_CAPACITY_SCALE / UCLAMP_BUCKETS) + 1)
> +
> +static inline unsigned int uclamp_bucket_id(unsigned int clamp_value)
> +{
> + return clamp_value / UCLAMP_BUCKET_DELTA;
> +}
> +
> +static inline unsigned int uclamp_bucket_value(unsigned int clamp_value)
> +{
> + return UCLAMP_BUCKET_DELTA * uclamp_bucket_id(clamp_value);
> +}
> +
> +static inline unsigned int uclamp_none(int clamp_id)
> +{
> + if (clamp_id == UCLAMP_MIN)
> + return 0;
> + return SCHED_CAPACITY_SCALE;
> +}
> +
> +static inline void uclamp_rq_update(struct rq *rq, unsigned int clamp_id)
> +{
> + struct uclamp_bucket *bucket = rq->uclamp[clamp_id].bucket;
> + unsigned int max_value = uclamp_none(clamp_id);
> + unsigned int bucket_id;
> +
> + /*
> + * Both min and max clamps are MAX aggregated, thus the topmost
> + * bucket with some tasks defines the rq's clamp value.
> + */
> + bucket_id = UCLAMP_BUCKETS;
> + do {
> + --bucket_id;
> + if (!rq->uclamp[clamp_id].bucket[bucket_id].tasks)
> + continue;
> + max_value = bucket[bucket_id].value;
> + break;
> + } while (bucket_id);
> +
> + WRITE_ONCE(rq->uclamp[clamp_id].value, max_value);
> +}
> +
> +/*
> + * When a task is enqueued on a rq, the clamp bucket currently defined by the
> + * task's uclamp::bucket_id is reference counted on that rq. This also
> + * immediately updates the rq's clamp value if required.
> + *
> + * Since tasks know their specific value requested from user-space, we track
> + * within each bucket the maximum value for tasks refcounted in that bucket.
> + * This provide a further aggregation (local clamping) which allows to track
> + * within each bucket the exact "requested" clamp value whenever all tasks
> + * RUNNABLE in that bucket require the same clamp.
> + */
> +static inline void uclamp_rq_inc_id(struct task_struct *p, struct rq *rq,
> + unsigned int clamp_id)
> +{
> + unsigned int bucket_id = p->uclamp[clamp_id].bucket_id;
> + unsigned int rq_clamp, bkt_clamp, tsk_clamp;
> +
> + rq->uclamp[clamp_id].bucket[bucket_id].tasks++;
> +
> + /*
> + * Local clamping: rq's buckets always track the max "requested"
> + * clamp value from all RUNNABLE tasks in that bucket.
> + */
> + tsk_clamp = p->uclamp[clamp_id].value;
> + bkt_clamp = rq->uclamp[clamp_id].bucket[bucket_id].value;
> + rq->uclamp[clamp_id].bucket[bucket_id].value = max(bkt_clamp, tsk_clamp);
> +
> + rq_clamp = READ_ONCE(rq->uclamp[clamp_id].value);
> + WRITE_ONCE(rq->uclamp[clamp_id].value, max(rq_clamp, tsk_clamp));
> +}
> +
> +/*
> + * When a task is dequeued from a rq, the clamp bucket reference counted by
> + * the task is released. If this is the last task reference counting the rq's
> + * max active clamp value, then the rq's clamp value is updated.
> + * Both the tasks reference counter and the rq's cached clamp values are
> + * expected to be always valid, if we detect they are not we skip the updates,
> + * enforce a consistent state and warn.
> + */
> +static inline void uclamp_rq_dec_id(struct task_struct *p, struct rq *rq,
> + unsigned int clamp_id)
> +{
> + unsigned int bucket_id = p->uclamp[clamp_id].bucket_id;
> + unsigned int rq_clamp, bkt_clamp;
> +
> + SCHED_WARN_ON(!rq->uclamp[clamp_id].bucket[bucket_id].tasks);
> + if (likely(rq->uclamp[clamp_id].bucket[bucket_id].tasks))
> + rq->uclamp[clamp_id].bucket[bucket_id].tasks--;
> +
> + /*
> + * Keep "local clamping" simple and accept to (possibly) overboost
> + * still RUNNABLE tasks in the same bucket.
> + */
> + if (likely(rq->uclamp[clamp_id].bucket[bucket_id].tasks))
> + return;
> + bkt_clamp = rq->uclamp[clamp_id].bucket[bucket_id].value;
> +
> + /* The rq's clamp value is expected to always track the max */
> + rq_clamp = READ_ONCE(rq->uclamp[clamp_id].value);
> + SCHED_WARN_ON(bkt_clamp > rq_clamp);
> + if (bkt_clamp >= rq_clamp) {
> + /*
> + * Reset rq's clamp bucket value to its nominal value whenever
> + * there are anymore RUNNABLE tasks refcounting it.
> + */
> + rq->uclamp[clamp_id].bucket[bucket_id].value =
> + uclamp_bucket_value(rq_clamp);
> + uclamp_rq_update(rq, clamp_id);
> + }
> +}
> +
> +static inline void uclamp_rq_inc(struct rq *rq, struct task_struct *p)
> +{
> + unsigned int clamp_id;
> +
> + if (unlikely(!p->sched_class->uclamp_enabled))
> + return;
> +
> + for (clamp_id = 0; clamp_id < UCLAMP_CNT; ++clamp_id)
> + uclamp_rq_inc_id(p, rq, clamp_id);
> +}
> +
> +static inline void uclamp_rq_dec(struct rq *rq, struct task_struct *p)
> +{
> + unsigned int clamp_id;
> +
> + if (unlikely(!p->sched_class->uclamp_enabled))
> + return;
> +
> + for (clamp_id = 0; clamp_id < UCLAMP_CNT; ++clamp_id)
> + uclamp_rq_dec_id(p, rq, clamp_id);
> +}
> +
> +static void __init init_uclamp(void)
> +{
> + unsigned int clamp_id;
> + int cpu;
> +
> + for_each_possible_cpu(cpu)
> + memset(&cpu_rq(cpu)->uclamp, 0, sizeof(struct uclamp_rq));
> +
> + for (clamp_id = 0; clamp_id < UCLAMP_CNT; ++clamp_id) {
> + unsigned int clamp_value = uclamp_none(clamp_id);
> + unsigned int bucket_id = uclamp_bucket_id(clamp_value);
> + struct uclamp_se *uc_se = &init_task.uclamp[clamp_id];
> +
> + uc_se->bucket_id = bucket_id;
> + uc_se->value = clamp_value;
> + }
> +}
> +
> +#else /* CONFIG_UCLAMP_TASK */
> +static inline void uclamp_rq_inc(struct rq *rq, struct task_struct *p) { }
> +static inline void uclamp_rq_dec(struct rq *rq, struct task_struct *p) { }
> +static inline void init_uclamp(void) { }
> +#endif /* CONFIG_UCLAMP_TASK */
> +
> static inline void enqueue_task(struct rq *rq, struct task_struct *p, int flags)
> {
> if (!(flags & ENQUEUE_NOCLOCK))
> @@ -729,6 +890,7 @@ static inline void enqueue_task(struct rq *rq, struct task_struct *p, int flags)
> psi_enqueue(p, flags & ENQUEUE_WAKEUP);
> }
>
> + uclamp_rq_inc(rq, p);
> p->sched_class->enqueue_task(rq, p, flags);
> }
>
> @@ -742,6 +904,7 @@ static inline void dequeue_task(struct rq *rq, struct task_struct *p, int flags)
> psi_dequeue(p, flags & DEQUEUE_SLEEP);
> }
>
> + uclamp_rq_dec(rq, p);
> p->sched_class->dequeue_task(rq, p, flags);
> }
>
> @@ -6075,6 +6238,8 @@ void __init sched_init(void)
>
> psi_init();
>
> + init_uclamp();
> +
> scheduler_running = 1;
> }
>
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index c688ef5012e5..ea9e28723946 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -797,6 +797,48 @@ extern void rto_push_irq_work_func(struct irq_work *work);
> #endif
> #endif /* CONFIG_SMP */
>
> +#ifdef CONFIG_UCLAMP_TASK
> +/*
> + * struct uclamp_bucket - Utilization clamp bucket
> + * @value: utilization clamp value for tasks on this clamp bucket
> + * @tasks: number of RUNNABLE tasks on this clamp bucket
> + *
> + * Keep track of how many tasks are RUNNABLE for a given utilization
> + * clamp value.
> + */
> +struct uclamp_bucket {
> + unsigned long value : bits_per(SCHED_CAPACITY_SCALE);
> + unsigned long tasks : BITS_PER_LONG - bits_per(SCHED_CAPACITY_SCALE);
> +};
> +
> +/*
> + * struct uclamp_rq - rq's utilization clamp
> + * @value: currently active clamp values for a rq
> + * @bucket: utilization clamp buckets affecting a rq
> + *
> + * Keep track of RUNNABLE tasks on a rq to aggregate their clamp values.
> + * A clamp value is affecting a rq when there is at least one task RUNNABLE
> + * (or actually running) with that value.
> + *
> + * We have up to UCLAMP_CNT possible different clamp values, which are
> + * currently only two: minmum utilization and maximum utilization.
> + *
> + * All utilization clamping values are MAX aggregated, since:
> + * - for util_min: we want to run the CPU at least at the max of the minimum
> + * utilization required by its currently RUNNABLE tasks.
> + * - for util_max: we want to allow the CPU to run up to the max of the
> + * maximum utilization allowed by its currently RUNNABLE tasks.
> + *
> + * Since on each system we expect only a limited number of different
> + * utilization clamp values (UCLAMP_BUCKETS), we use a simple array to track
> + * the metrics required to compute all the per-rq utilization clamp values.
> + */
> +struct uclamp_rq {
> + unsigned int value;
> + struct uclamp_bucket bucket[UCLAMP_BUCKETS];
> +};
> +#endif /* CONFIG_UCLAMP_TASK */
> +
> /*
> * This is the main, per-CPU runqueue data structure.
> *
> @@ -835,6 +877,11 @@ struct rq {
> unsigned long nr_load_updates;
> u64 nr_switches;
>
> +#ifdef CONFIG_UCLAMP_TASK
> + /* Utilization clamp values based on CPU's RUNNABLE tasks */
> + struct uclamp_rq uclamp[UCLAMP_CNT] ____cacheline_aligned;
> +#endif
> +
> struct cfs_rq cfs;
> struct rt_rq rt;
> struct dl_rq dl;
> @@ -1649,10 +1696,12 @@ extern const u32 sched_prio_to_wmult[40];
> struct sched_class {
> const struct sched_class *next;
>
> +#ifdef CONFIG_UCLAMP_TASK
> + int uclamp_enabled;
> +#endif
> +
> void (*enqueue_task) (struct rq *rq, struct task_struct *p, int flags);
> void (*dequeue_task) (struct rq *rq, struct task_struct *p, int flags);
> - void (*yield_task) (struct rq *rq);
> - bool (*yield_to_task)(struct rq *rq, struct task_struct *p, bool preempt);
>
> void (*check_preempt_curr)(struct rq *rq, struct task_struct *p, int flags);
>
> @@ -1685,7 +1734,6 @@ struct sched_class {
> void (*set_curr_task)(struct rq *rq);
> void (*task_tick)(struct rq *rq, struct task_struct *p, int queued);
> void (*task_fork)(struct task_struct *p);
> - void (*task_dead)(struct task_struct *p);
>
> /*
> * The switched_from() call is allowed to drop rq->lock, therefore we
> @@ -1702,12 +1750,17 @@ struct sched_class {
>
> void (*update_curr)(struct rq *rq);
>
> + void (*yield_task) (struct rq *rq);
> + bool (*yield_to_task)(struct rq *rq, struct task_struct *p, bool preempt);
> +
> #define TASK_SET_GROUP 0
> #define TASK_MOVE_GROUP 1
>
> #ifdef CONFIG_FAIR_GROUP_SCHED
> void (*task_change_group)(struct task_struct *p, int type);
> #endif
> +
> + void (*task_dead)(struct task_struct *p);
> };
>
> static inline void put_prev_task(struct rq *rq, struct task_struct *prev)
> --
> 2.20.1
>
^ permalink raw reply
* Re: [PATCH v7 01/15] sched/core: uclamp: Add CPU's clamp buckets refcounting
From: Suren Baghdasaryan @ 2019-03-13 21:23 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Patrick Bellasi, LKML, linux-pm, linux-api, Ingo Molnar,
Tejun Heo, Rafael J . Wysocki, Vincent Guittot, Viresh Kumar,
Paul Turner, Quentin Perret, Dietmar Eggemann, Morten Rasmussen,
Juri Lelli, Todd Kjos, Joel Fernandes, Steve Muckle
In-Reply-To: <20190313135238.GC5922@hirez.programming.kicks-ass.net>
On Wed, Mar 13, 2019 at 6:52 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Fri, Feb 08, 2019 at 10:05:40AM +0000, Patrick Bellasi wrote:
> > +/*
> > + * When a task is enqueued on a rq, the clamp bucket currently defined by the
> > + * task's uclamp::bucket_id is reference counted on that rq. This also
> > + * immediately updates the rq's clamp value if required.
> > + *
> > + * Since tasks know their specific value requested from user-space, we track
> > + * within each bucket the maximum value for tasks refcounted in that bucket.
> > + * This provide a further aggregation (local clamping) which allows to track
> > + * within each bucket the exact "requested" clamp value whenever all tasks
> > + * RUNNABLE in that bucket require the same clamp.
> > + */
> > +static inline void uclamp_rq_inc_id(struct task_struct *p, struct rq *rq,
> > + unsigned int clamp_id)
> > +{
> > + unsigned int bucket_id = p->uclamp[clamp_id].bucket_id;
> > + unsigned int rq_clamp, bkt_clamp, tsk_clamp;
> > +
> > + rq->uclamp[clamp_id].bucket[bucket_id].tasks++;
> > +
> > + /*
> > + * Local clamping: rq's buckets always track the max "requested"
> > + * clamp value from all RUNNABLE tasks in that bucket.
> > + */
> > + tsk_clamp = p->uclamp[clamp_id].value;
> > + bkt_clamp = rq->uclamp[clamp_id].bucket[bucket_id].value;
> > + rq->uclamp[clamp_id].bucket[bucket_id].value = max(bkt_clamp, tsk_clamp);
>
> So, if I read this correct:
>
> - here we track a max value in a bucket,
>
> > + rq_clamp = READ_ONCE(rq->uclamp[clamp_id].value);
> > + WRITE_ONCE(rq->uclamp[clamp_id].value, max(rq_clamp, tsk_clamp));
> > +}
> > +
> > +/*
> > + * When a task is dequeued from a rq, the clamp bucket reference counted by
> > + * the task is released. If this is the last task reference counting the rq's
> > + * max active clamp value, then the rq's clamp value is updated.
> > + * Both the tasks reference counter and the rq's cached clamp values are
> > + * expected to be always valid, if we detect they are not we skip the updates,
> > + * enforce a consistent state and warn.
> > + */
> > +static inline void uclamp_rq_dec_id(struct task_struct *p, struct rq *rq,
> > + unsigned int clamp_id)
> > +{
> > + unsigned int bucket_id = p->uclamp[clamp_id].bucket_id;
> > + unsigned int rq_clamp, bkt_clamp;
> > +
> > + SCHED_WARN_ON(!rq->uclamp[clamp_id].bucket[bucket_id].tasks);
> > + if (likely(rq->uclamp[clamp_id].bucket[bucket_id].tasks))
> > + rq->uclamp[clamp_id].bucket[bucket_id].tasks--;
> > +
> > + /*
> > + * Keep "local clamping" simple and accept to (possibly) overboost
> > + * still RUNNABLE tasks in the same bucket.
> > + */
> > + if (likely(rq->uclamp[clamp_id].bucket[bucket_id].tasks))
> > + return;
>
> (Oh man, I hope that generates semi sane code; long live CSE passes I
> suppose)
>
> But we never decrement that bkt_clamp value on dequeue.
>
> > + bkt_clamp = rq->uclamp[clamp_id].bucket[bucket_id].value;
> > +
> > + /* The rq's clamp value is expected to always track the max */
> > + rq_clamp = READ_ONCE(rq->uclamp[clamp_id].value);
> > + SCHED_WARN_ON(bkt_clamp > rq_clamp);
> > + if (bkt_clamp >= rq_clamp) {
>
> head hurts, this reads ==, how can this ever not be so?
>
> > + /*
> > + * Reset rq's clamp bucket value to its nominal value whenever
> > + * there are anymore RUNNABLE tasks refcounting it.
>
> -ENOPARSE
>
> > + */
> > + rq->uclamp[clamp_id].bucket[bucket_id].value =
> > + uclamp_bucket_value(rq_clamp);
>
> But basically you decrement the bucket value to the nominal value.
>
> > + uclamp_rq_update(rq, clamp_id);
> > + }
> > +}
>
> Given all that, what is to stop the bucket value to climbing to
> uclamp_bucket_value(+1)-1 and staying there (provided there's someone
> runnable)?
>
> Why are we doing this... ?
I agree with Peter, this part of the patch was the hardest to read.
SCHED_WARN_ON line makes sense to me. The condition that follows and
the following comment are a little baffling. Condition seems to
indicate that the code that follows should be executed only if we are
in the top-most occupied bucket (the bucket which has tasks and has
the highest uclamp value). So this bucket just lost its last task and
we should update rq->uclamp[clamp_id].value. However that's not
exactly what the code does... It also resets
rq->uclamp[clamp_id].bucket[bucket_id].value. So if I understand
correctly, unless the bucket that just lost its last task is the
top-most one its value will not be reset to nominal value. That looks
like a bug to me. Am I missing something?
Side note: some more explanation would be very helpful.
^ permalink raw reply
* Re: [PATCH v7 01/15] sched/core: uclamp: Add CPU's clamp buckets refcounting
From: Suren Baghdasaryan @ 2019-03-13 21:08 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Patrick Bellasi, LKML, linux-pm, linux-api, Ingo Molnar,
Tejun Heo, Rafael J . Wysocki, Vincent Guittot, Viresh Kumar,
Paul Turner, Quentin Perret, Dietmar Eggemann, Morten Rasmussen,
Juri Lelli, Todd Kjos, Joel Fernandes, Steve Muckle
In-Reply-To: <20190313194619.GR2482@worktop.programming.kicks-ass.net>
On Wed, Mar 13, 2019 at 12:46 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Wed, Mar 13, 2019 at 03:23:59PM +0000, Patrick Bellasi wrote:
> > On 13-Mar 15:09, Peter Zijlstra wrote:
> > > On Fri, Feb 08, 2019 at 10:05:40AM +0000, Patrick Bellasi wrote:
>
> > > > +static inline void uclamp_rq_update(struct rq *rq, unsigned int clamp_id)
> > > > +{
> > > > + struct uclamp_bucket *bucket = rq->uclamp[clamp_id].bucket;
> > > > + unsigned int max_value = uclamp_none(clamp_id);
> > >
> > > That's 1024 for uclamp_max
> > >
> > > > + unsigned int bucket_id;
> > > > +
> > > > + /*
> > > > + * Both min and max clamps are MAX aggregated, thus the topmost
> > > > + * bucket with some tasks defines the rq's clamp value.
> > > > + */
> > > > + bucket_id = UCLAMP_BUCKETS;
> > > > + do {
> > > > + --bucket_id;
> > > > + if (!rq->uclamp[clamp_id].bucket[bucket_id].tasks)
> > > > + continue;
> > > > + max_value = bucket[bucket_id].value;
> > >
> > > but this will then _lower_ it. That's not a MAX aggregate.
> >
> > For uclamp_max we want max_value=1024 when there are no active tasks,
> > which means: no max clamp enforced on CFS/RT "idle" cpus.
> >
> > If instead there are active RT/CFS tasks then we want the clamp value
> > of the max group, which means: MAX aggregate active clamps.
> >
> > That's what the code above does and the comment says.
>
> That's (obviously) not how I read it... maybe something like:
>
> static inline void uclamp_rq_update(struct rq *rq, unsigned int clamp_id)
> {
> struct uclamp_bucket *bucket = rq->uclamp[clamp_id].bucket;
> int i;
>
> /*
> * Since both min and max clamps are max aggregated, find the
> * top most bucket with tasks in.
> */
> for (i = UCLMAP_BUCKETS-1; i>=0; i--) {
> if (!bucket[i].tasks)
> continue;
> return bucket[i].value;
> }
>
> /* No tasks -- default clamp values */
> return uclamp_none(clamp_id);
> }
>
> would make it clearer?
This way it's also more readable/obvious when it's used inside
uclamp_rq_dec_id, assuming uclamp_rq_update is renamed into smth like
get_max_rq_uclamp.
^ permalink raw reply
* Re: [PATCH v7 01/15] sched/core: uclamp: Add CPU's clamp buckets refcounting
From: Suren Baghdasaryan @ 2019-03-13 21:01 UTC (permalink / raw)
To: Patrick Bellasi
Cc: Dietmar Eggemann, LKML, linux-pm, linux-api, Ingo Molnar,
Peter Zijlstra, Tejun Heo, Rafael J . Wysocki, Vincent Guittot,
Viresh Kumar, Paul Turner, Quentin Perret, Morten Rasmussen,
Juri Lelli, Todd Kjos, Joel Fernandes, Steve Muckle
In-Reply-To: <20190313151535.q5ivsuywvwkewrk5@e110439-lin>
On Wed, Mar 13, 2019 at 8:15 AM Patrick Bellasi <patrick.bellasi@arm.com> wrote:
>
> On 12-Mar 13:52, Dietmar Eggemann wrote:
> > On 2/8/19 11:05 AM, Patrick Bellasi wrote:
> >
> > [...]
> >
> > > +config UCLAMP_BUCKETS_COUNT
> > > + int "Number of supported utilization clamp buckets"
> > > + range 5 20
> > > + default 5
> > > + depends on UCLAMP_TASK
> > > + help
> > > + Defines the number of clamp buckets to use. The range of each bucket
> > > + will be SCHED_CAPACITY_SCALE/UCLAMP_BUCKETS_COUNT. The higher the
> > > + number of clamp buckets the finer their granularity and the higher
> > > + the precision of clamping aggregation and tracking at run-time.
> > > +
> > > + For example, with the default configuration we will have 5 clamp
> > > + buckets tracking 20% utilization each. A 25% boosted tasks will be
> > > + refcounted in the [20..39]% bucket and will set the bucket clamp
> > > + effective value to 25%.
> > > + If a second 30% boosted task should be co-scheduled on the same CPU,
> > > + that task will be refcounted in the same bucket of the first task and
> > > + it will boost the bucket clamp effective value to 30%.
> > > + The clamp effective value of a bucket is reset to its nominal value
> > > + (20% in the example above) when there are anymore tasks refcounted in
> >
> > this sounds weird.
>
> Why ?
Should probably be "when there are no more tasks refcounted"
> >
> > [...]
> >
> > > +static inline unsigned int uclamp_bucket_value(unsigned int clamp_value)
> > > +{
> > > + return UCLAMP_BUCKET_DELTA * uclamp_bucket_id(clamp_value);
> > > +}
> >
> > Soemthing like uclamp_bucket_nominal_value() should be clearer.
>
> Maybe... can update it in v8
>
uclamp_bucket_base_value is a little shorter, just to consider :)
> > > +static inline void uclamp_rq_update(struct rq *rq, unsigned int clamp_id)
> > > +{
> > > + struct uclamp_bucket *bucket = rq->uclamp[clamp_id].bucket;
> > > + unsigned int max_value = uclamp_none(clamp_id);
> > > + unsigned int bucket_id;
> >
> > unsigned int bucket_id = UCLAMP_BUCKETS;
> >
> > > +
> > > + /*
> > > + * Both min and max clamps are MAX aggregated, thus the topmost
> > > + * bucket with some tasks defines the rq's clamp value.
> > > + */
> > > + bucket_id = UCLAMP_BUCKETS;
> >
> > to get rid of this line?
>
> I put it on a different line as a justfication for the loop variable
> initialization described in the comment above.
>
> >
> > > + do {
> > > + --bucket_id;
> > > + if (!rq->uclamp[clamp_id].bucket[bucket_id].tasks)
> >
> > if (!bucket[bucket_id].tasks)
>
> Right... that's some leftover from the last refactoring!
>
> [...]
>
> > > + * within each bucket the exact "requested" clamp value whenever all tasks
> > > + * RUNNABLE in that bucket require the same clamp.
> > > + */
> > > +static inline void uclamp_rq_inc_id(struct task_struct *p, struct rq *rq,
> > > + unsigned int clamp_id)
> > > +{
> > > + unsigned int bucket_id = p->uclamp[clamp_id].bucket_id;
> > > + unsigned int rq_clamp, bkt_clamp, tsk_clamp;
> >
> > Wouldn't it be easier to have a pointer to the task's and rq's uclamp
> > structure as well to the bucket?
> >
> > - unsigned int bucket_id = p->uclamp[clamp_id].bucket_id;
> > + struct uclamp_se *uc_se = &p->uclamp[clamp_id];
> > + struct uclamp_rq *uc_rq = &rq->uclamp[clamp_id];
> > + struct uclamp_bucket *bucket = &uc_rq->bucket[uc_se->bucket_id];
>
> I think I went back/forth a couple of times in using pointer or the
> extended version, which both have pros and cons.
>
> I personally prefer the pointers as you suggest but I've got the
> impression in the past that since everybody cleared "basic C trainings"
> it's not so difficult to read the code above too.
>
> > The code in uclamp_rq_inc_id() and uclamp_rq_dec_id() for example becomes
> > much more readable.
>
> Agree... let's try to switch once again in v8 and see ;)
>
> > [...]
> >
> > > struct sched_class {
> > > const struct sched_class *next;
> > > +#ifdef CONFIG_UCLAMP_TASK
> > > + int uclamp_enabled;
> > > +#endif
> > > +
> > > void (*enqueue_task) (struct rq *rq, struct task_struct *p, int flags);
> > > void (*dequeue_task) (struct rq *rq, struct task_struct *p, int flags);
> > > - void (*yield_task) (struct rq *rq);
> > > - bool (*yield_to_task)(struct rq *rq, struct task_struct *p, bool preempt);
> > > void (*check_preempt_curr)(struct rq *rq, struct task_struct *p, int flags);
> > > @@ -1685,7 +1734,6 @@ struct sched_class {
> > > void (*set_curr_task)(struct rq *rq);
> > > void (*task_tick)(struct rq *rq, struct task_struct *p, int queued);
> > > void (*task_fork)(struct task_struct *p);
> > > - void (*task_dead)(struct task_struct *p);
> > > /*
> > > * The switched_from() call is allowed to drop rq->lock, therefore we
> > > @@ -1702,12 +1750,17 @@ struct sched_class {
> > > void (*update_curr)(struct rq *rq);
> > > + void (*yield_task) (struct rq *rq);
> > > + bool (*yield_to_task)(struct rq *rq, struct task_struct *p, bool preempt);
> > > +
> > > #define TASK_SET_GROUP 0
> > > #define TASK_MOVE_GROUP 1
> > > #ifdef CONFIG_FAIR_GROUP_SCHED
> > > void (*task_change_group)(struct task_struct *p, int type);
> > > #endif
> > > +
> > > + void (*task_dead)(struct task_struct *p);
> >
> > Why do you move yield_task, yield_to_task and task_dead here?
>
> Since I'm adding a new field at the beginning of the struct, which is
> used at enqueue/dequeue time, this is to ensure that all the
> callbacks used in these paths are grouped together and don't fall
> across a cache line... but yes, that's supposed to be a
> micro-optimization which I can skip in this patch.
>
> --
> #include <best/regards.h>
>
> Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v7 06/15] sched/core: uclamp: Reset uclamp values on RESET_ON_FORK
From: Peter Zijlstra @ 2019-03-13 20:52 UTC (permalink / raw)
To: Patrick Bellasi
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190208100554.32196-7-patrick.bellasi@arm.com>
On Fri, Feb 08, 2019 at 10:05:45AM +0000, Patrick Bellasi wrote:
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 070caa1f72eb..8b282616e9c9 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -1071,7 +1071,7 @@ static void __setscheduler_uclamp(struct task_struct *p,
> }
> }
>
> -static void uclamp_fork(struct task_struct *p)
> +static void uclamp_fork(struct task_struct *p, bool reset)
> {
> unsigned int clamp_id;
>
> @@ -1080,6 +1080,17 @@ static void uclamp_fork(struct task_struct *p)
IIRC there's an early return here if the class doesn't have uclamp
support, which I think is wrong now. You want the reset irrespective of
whether the class supports it, no?
>
> for (clamp_id = 0; clamp_id < UCLAMP_CNT; ++clamp_id)
> p->uclamp[clamp_id].active = false;
> +
> + if (likely(!reset))
> + return;
> +
> + for (clamp_id = 0; clamp_id < UCLAMP_CNT; ++clamp_id) {
> + unsigned int clamp_value = uclamp_none(clamp_id);
> +
> + p->uclamp[clamp_id].user_defined = false;
> + p->uclamp[clamp_id].value = clamp_value;
> + p->uclamp[clamp_id].bucket_id = uclamp_bucket_id(clamp_value);
> + }
> }
^ permalink raw reply
* Re: [PATCH v7 03/15] sched/core: uclamp: Add system default clamps
From: Peter Zijlstra @ 2019-03-13 20:18 UTC (permalink / raw)
To: Patrick Bellasi
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190208100554.32196-4-patrick.bellasi@arm.com>
On Fri, Feb 08, 2019 at 10:05:42AM +0000, Patrick Bellasi wrote:
> +static void uclamp_fork(struct task_struct *p)
> +{
> + unsigned int clamp_id;
> +
> + if (unlikely(!p->sched_class->uclamp_enabled))
> + return;
> +
> + for (clamp_id = 0; clamp_id < UCLAMP_CNT; ++clamp_id)
> + p->uclamp[clamp_id].active = false;
> +}
Because in that case .active == false, and copy_process() will have done
thr right thing?
^ permalink raw reply
* Re: [PATCH v7 03/15] sched/core: uclamp: Add system default clamps
From: Peter Zijlstra @ 2019-03-13 20:13 UTC (permalink / raw)
To: Patrick Bellasi
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190208100554.32196-4-patrick.bellasi@arm.com>
On Fri, Feb 08, 2019 at 10:05:42AM +0000, Patrick Bellasi wrote:
> +int sysctl_sched_uclamp_handler(struct ctl_table *table, int write,
> + void __user *buffer, size_t *lenp,
> + loff_t *ppos)
> +{
> + int old_min, old_max;
> + int result = 0;
Should this not have an internal mutex to serialize concurrent usage?
See for example sched_rt_handler().
> +
> + old_min = sysctl_sched_uclamp_util_min;
> + old_max = sysctl_sched_uclamp_util_max;
> +
> + result = proc_dointvec(table, write, buffer, lenp, ppos);
> + if (result)
> + goto undo;
> + if (!write)
> + goto done;
> +
> + if (sysctl_sched_uclamp_util_min > sysctl_sched_uclamp_util_max ||
> + sysctl_sched_uclamp_util_max > SCHED_CAPACITY_SCALE) {
> + result = -EINVAL;
> + goto undo;
> + }
> +
> + if (old_min != sysctl_sched_uclamp_util_min) {
> + uclamp_default[UCLAMP_MIN].value =
> + sysctl_sched_uclamp_util_min;
> + uclamp_default[UCLAMP_MIN].bucket_id =
> + uclamp_bucket_id(sysctl_sched_uclamp_util_min);
> + }
> + if (old_max != sysctl_sched_uclamp_util_max) {
> + uclamp_default[UCLAMP_MAX].value =
> + sysctl_sched_uclamp_util_max;
> + uclamp_default[UCLAMP_MAX].bucket_id =
> + uclamp_bucket_id(sysctl_sched_uclamp_util_max);
> + }
> +
> + /*
> + * Updating all the RUNNABLE task is expensive, keep it simple and do
> + * just a lazy update at each next enqueue time.
> + */
> + goto done;
> +
> +undo:
> + sysctl_sched_uclamp_util_min = old_min;
> + sysctl_sched_uclamp_util_max = old_max;
> +done:
> +
> + return result;
> +}
^ permalink raw reply
* Re: [PATCH v7 03/15] sched/core: uclamp: Add system default clamps
From: Peter Zijlstra @ 2019-03-13 20:10 UTC (permalink / raw)
To: Patrick Bellasi
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190313170940.ngiafkmiijryhl2k@e110439-lin>
On Wed, Mar 13, 2019 at 05:09:40PM +0000, Patrick Bellasi wrote:
> Yes, that should be possible... will look into splitting this out in
> v8 to have something like:
>
> ---8<---
> struct uclamp_req {
> /* Clamp value "requested" by a scheduling entity */
> unsigned int value : bits_per(SCHED_CAPACITY_SCALE);
> unsigned int bucket_id : bits_per(UCLAMP_BUCKETS);
> unsigned int active : 1;
> unsigned int user_defined : 1;
> }
>
> struct uclamp_eff {
> unsigned int value : bits_per(SCHED_CAPACITY_SCALE);
> unsigned int bucket_id : bits_per(UCLAMP_BUCKETS);
> }
No, have _1_ type. There is no point what so ever to splitting this.
Also, what's @user_defined about, I don't think I've seen that in the
parent patch.
> struct task_struct {
> // ...
> #ifdef CONFIG_UCLAMP_TASK
> struct uclamp_req uclamp_req[UCLAMP_CNT];
> struct uclamp_eff uclamp_eff[UCLAMP_CNT];
struct uclamp_se uclamp[UCLAMP_CNT];
struct uclamp_se uclamp_req[UCLAMP_CNT];
Where the first is the very same introduced in patch #1, and leaving it
in place avoids having to update the sites already using that (or start
#1 with the _eff name to avoid having to change things around?).
> #endif
> // ...
> }
>
> static inline struct uclamp_eff
> uclamp_eff_get(struct task_struct *p, unsigned int clamp_id)
> {
> struct uclamp_eff uc_eff = p->uclamp_eff[clamp_id];
just this ^, these lines seem like a superfluous duplication:
> uc_eff.bucket_id = p->uclamp_req[clamp_id].bucket_id;
> uc_eff.value = p->uclamp_req[clamp_id].value;
> if (unlikely(uc_eff.clamp_value > uclamp_default[clamp_id].value)) {
> uc_eff.clamp_value = uclamp_default[clamp_id].value;
> uc_eff.bucket_id = uclamp_default[clamp_id].bucket_id;
and:
uc = uclamp_default[clamp_id];
> }
>
> return uc_eff;
> }
>
> static inline void
> uclamp_eff_set(struct task_struct *p, unsigned int clamp_id)
> {
> p->uclamp_eff[clamp_id] = uclamp_eff_get(p, clamp_id);
> }
> ---8<---
>
> Is that what you mean ?
Getting there :-)
^ permalink raw reply
* Re: [PATCH v7 03/15] sched/core: uclamp: Add system default clamps
From: Peter Zijlstra @ 2019-03-13 19:58 UTC (permalink / raw)
To: Patrick Bellasi
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190313170940.ngiafkmiijryhl2k@e110439-lin>
On Wed, Mar 13, 2019 at 05:09:40PM +0000, Patrick Bellasi wrote:
> On 13-Mar 15:32, Peter Zijlstra wrote:
> > I still think that this effective thing is backwards.
> With respect to the previous v6, I've now moved this concept to the
> patch where we actually use it for the first time.
> The "effective" values allows to efficiently enforce the most
> restrictive clamp value for a task at enqueue time by:
> - not loosing track of the original request
> - don't caring about updating non runnable tasks
My point is that you already had an effective value; namely p->uclamp[],
since patch 1.
This patch then changes that into something else, instead of adding
p->uclamp_orig[], and concequently has to update all sites that
previously used p->uclamp[], which is a lot of pointless churn.
^ permalink raw reply
* Re: [PATCH v7 01/15] sched/core: uclamp: Add CPU's clamp buckets refcounting
From: Peter Zijlstra @ 2019-03-13 19:48 UTC (permalink / raw)
To: Patrick Bellasi
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190313161229.pkib2tmjass5chtb@e110439-lin>
On Wed, Mar 13, 2019 at 04:12:29PM +0000, Patrick Bellasi wrote:
> On 13-Mar 14:40, Peter Zijlstra wrote:
> > On Fri, Feb 08, 2019 at 10:05:40AM +0000, Patrick Bellasi wrote:
> > > +static inline unsigned int uclamp_bucket_id(unsigned int clamp_value)
> > > +{
> > > + return clamp_value / UCLAMP_BUCKET_DELTA;
> > > +}
> > > +
> > > +static inline unsigned int uclamp_bucket_value(unsigned int clamp_value)
> > > +{
> > > + return UCLAMP_BUCKET_DELTA * uclamp_bucket_id(clamp_value);
> >
> > return clamp_value - (clamp_value % UCLAMP_BUCKET_DELTA);
> >
> > might generate better code; just a single division, instead of a div and
> > mult.
>
> Wondering if compilers cannot do these optimizations... but yes, looks
> cool and will do it in v8, thanks.
I'd be most impressed if they pull this off. Check the generated code
and see I suppose :-)
^ permalink raw reply
* Re: [PATCH v7 01/15] sched/core: uclamp: Add CPU's clamp buckets refcounting
From: Peter Zijlstra @ 2019-03-13 19:46 UTC (permalink / raw)
To: Patrick Bellasi
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190313152359.lkyzel7fakjoi5hu@e110439-lin>
On Wed, Mar 13, 2019 at 03:23:59PM +0000, Patrick Bellasi wrote:
> On 13-Mar 15:09, Peter Zijlstra wrote:
> > On Fri, Feb 08, 2019 at 10:05:40AM +0000, Patrick Bellasi wrote:
> > > +static inline void uclamp_rq_update(struct rq *rq, unsigned int clamp_id)
> > > +{
> > > + struct uclamp_bucket *bucket = rq->uclamp[clamp_id].bucket;
> > > + unsigned int max_value = uclamp_none(clamp_id);
> >
> > That's 1024 for uclamp_max
> >
> > > + unsigned int bucket_id;
> > > +
> > > + /*
> > > + * Both min and max clamps are MAX aggregated, thus the topmost
> > > + * bucket with some tasks defines the rq's clamp value.
> > > + */
> > > + bucket_id = UCLAMP_BUCKETS;
> > > + do {
> > > + --bucket_id;
> > > + if (!rq->uclamp[clamp_id].bucket[bucket_id].tasks)
> > > + continue;
> > > + max_value = bucket[bucket_id].value;
> >
> > but this will then _lower_ it. That's not a MAX aggregate.
>
> For uclamp_max we want max_value=1024 when there are no active tasks,
> which means: no max clamp enforced on CFS/RT "idle" cpus.
>
> If instead there are active RT/CFS tasks then we want the clamp value
> of the max group, which means: MAX aggregate active clamps.
>
> That's what the code above does and the comment says.
That's (obviously) not how I read it... maybe something like:
static inline void uclamp_rq_update(struct rq *rq, unsigned int clamp_id)
{
struct uclamp_bucket *bucket = rq->uclamp[clamp_id].bucket;
int i;
/*
* Since both min and max clamps are max aggregated, find the
* top most bucket with tasks in.
*/
for (i = UCLMAP_BUCKETS-1; i>=0; i--) {
if (!bucket[i].tasks)
continue;
return bucket[i].value;
}
/* No tasks -- default clamp values */
return uclamp_none(clamp_id);
}
would make it clearer?
^ permalink raw reply
* Re: [PATCH v7 01/15] sched/core: uclamp: Add CPU's clamp buckets refcounting
From: Peter Zijlstra @ 2019-03-13 19:39 UTC (permalink / raw)
To: Patrick Bellasi
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190313155954.jse2tyn5iqxm6wle@e110439-lin>
On Wed, Mar 13, 2019 at 03:59:54PM +0000, Patrick Bellasi wrote:
> On 13-Mar 14:52, Peter Zijlstra wrote:
> Because of backetization, we potentially end up tracking tasks with
> different requested clamp values in the same bucket.
>
> For example, with 20% bucket size, we can have:
> Task1: util_min=25%
> Task2: util_min=35%
> accounted in the same bucket.
> > Given all that, what is to stop the bucket value to climbing to
> > uclamp_bucket_value(+1)-1 and staying there (provided there's someone
> > runnable)?
>
> Nothing... but that's an expected consequence of bucketization.
No, it is not.
> > Why are we doing this... ?
>
> You can either decide to:
>
> a) always boost tasks to just the bucket nominal value
> thus always penalizing both Task1 and Task2 of the example above
This is the expected behaviour. When was the last time your histogram
did something like b?
> b) always boost tasks to the bucket "max" value
> thus always overboosting both Task1 and Task2 of the example above
>
> The solution above instead has a very good property: in systems
> where you have only few and well defined clamp values we always
> provide the exact boost.
>
> For example, if your system requires only 23% and 47% boost values
> (totally random numbers), then you can always get the exact boost
> required using just 3 bucksts or ~33% size each.
>
> In systems where you don't know which boost values you will have, you
> can still defined the maximum overboost granularity you accept for
> each task by just tuning the number of clamp groups. For example, with
> 20 groups you can have a 5% max overboost.
Maybe, but this is not a direct concequence of buckets, but an
additional heuristic that might work well in this case.
Maybe split this out in a separate patch? So start with the trivial
bucket, and then do this change on top with the above few paragraphs as
changelog?
^ permalink raw reply
* Re: [PATCH v7 01/15] sched/core: uclamp: Add CPU's clamp buckets refcounting
From: Peter Zijlstra @ 2019-03-13 19:30 UTC (permalink / raw)
To: Patrick Bellasi
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190313155954.jse2tyn5iqxm6wle@e110439-lin>
On Wed, Mar 13, 2019 at 03:59:54PM +0000, Patrick Bellasi wrote:
> On 13-Mar 14:52, Peter Zijlstra wrote:
> > > +static inline void uclamp_rq_dec_id(struct task_struct *p, struct rq *rq,
> > > + unsigned int clamp_id)
> > > +{
> > > + unsigned int bucket_id = p->uclamp[clamp_id].bucket_id;
> > > + unsigned int rq_clamp, bkt_clamp;
> > > +
> > > + SCHED_WARN_ON(!rq->uclamp[clamp_id].bucket[bucket_id].tasks);
> > > + if (likely(rq->uclamp[clamp_id].bucket[bucket_id].tasks))
> > > + rq->uclamp[clamp_id].bucket[bucket_id].tasks--;
> > > +
> > > + /*
> > > + * Keep "local clamping" simple and accept to (possibly) overboost
> > > + * still RUNNABLE tasks in the same bucket.
> > > + */
> > > + if (likely(rq->uclamp[clamp_id].bucket[bucket_id].tasks))
> > > + return;
> >
> > (Oh man, I hope that generates semi sane code; long live CSE passes I
> > suppose)
>
> What do you mean ?
that does: 'rq->uclamp[clamp_id].bucket[bucket_id].tasks' three times in
a row. And yes the compiler _should_ dtrt, but....
^ permalink raw reply
* Re: [PATCH v7 02/15] sched/core: uclamp: Enforce last task UCLAMP_MAX
From: Patrick Bellasi @ 2019-03-13 18:29 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190313172911.GH5996@hirez.programming.kicks-ass.net>
On 13-Mar 18:29, Peter Zijlstra wrote:
> On Wed, Mar 13, 2019 at 04:20:51PM +0000, Patrick Bellasi wrote:
> > On 13-Mar 15:10, Peter Zijlstra wrote:
> > > On Fri, Feb 08, 2019 at 10:05:41AM +0000, Patrick Bellasi wrote:
> > > > +uclamp_idle_value(struct rq *rq, unsigned int clamp_id, unsigned int clamp_value)
> > > > +{
> > > > + /*
> > > > + * Avoid blocked utilization pushing up the frequency when we go
> > > > + * idle (which drops the max-clamp) by retaining the last known
> > > > + * max-clamp.
> > > > + */
> > > > + if (clamp_id == UCLAMP_MAX) {
> > > > + rq->uclamp_flags |= UCLAMP_FLAG_IDLE;
> > > > + return clamp_value;
> > > > + }
> > > > +
> > > > + return uclamp_none(UCLAMP_MIN);
> > >
> > > That's a very complicated way or writing: return 0, right?
> >
> > In my mind it's just a simple way to hardcode values in just one place.
> >
> > In the current implementation uclamp_none(UCLAMP_MIN) is 0 and the
> > compiler is not in trubles to inline a 0 there.
> >
> > Is it really so disgusting ?
>
> Not disguisting per se, just complicated. It had me go back and check
> wth uclamp_none() did again.
Yes, I see... every time I read it I just consider that uclamp_none()
it's just returning whatever is (or will be) the "non clamped" value
for the specified clamp index.
If it's ok with you, I would keep the code above as it is now.
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v7 01/15] sched/core: uclamp: Add CPU's clamp buckets refcounting
From: Patrick Bellasi @ 2019-03-13 18:22 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190313172225.GG5996@hirez.programming.kicks-ass.net>
On 13-Mar 18:22, Peter Zijlstra wrote:
> On Wed, Mar 13, 2019 at 04:12:29PM +0000, Patrick Bellasi wrote:
> > Yes, the for looks better, but perhaps like that:
> >
> > unsigned int bucket_id = UCLAMP_BUCKETS;
> >
> > /*
> > * Both min and max clamps are MAX aggregated, thus the topmost
> > * bucket with some tasks defines the rq's clamp value.
> > */
> > for (; bucket_id >= 0; --bucket_id) {
>
> GCC will be clever and figure that unsigned will never be smaller than 0
> and turn the above into an infinite loop or something daft.
>
> That is; use 'int', not 'unsigned int' for bucket_id.
Right, which remembers me now why I originally went for a do { } while();
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v7 02/15] sched/core: uclamp: Enforce last task UCLAMP_MAX
From: Peter Zijlstra @ 2019-03-13 17:29 UTC (permalink / raw)
To: Patrick Bellasi
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190313162051.djiu5dwc5ahp5p5p@e110439-lin>
On Wed, Mar 13, 2019 at 04:20:51PM +0000, Patrick Bellasi wrote:
> On 13-Mar 15:10, Peter Zijlstra wrote:
> > On Fri, Feb 08, 2019 at 10:05:41AM +0000, Patrick Bellasi wrote:
> > > +uclamp_idle_value(struct rq *rq, unsigned int clamp_id, unsigned int clamp_value)
> > > +{
> > > + /*
> > > + * Avoid blocked utilization pushing up the frequency when we go
> > > + * idle (which drops the max-clamp) by retaining the last known
> > > + * max-clamp.
> > > + */
> > > + if (clamp_id == UCLAMP_MAX) {
> > > + rq->uclamp_flags |= UCLAMP_FLAG_IDLE;
> > > + return clamp_value;
> > > + }
> > > +
> > > + return uclamp_none(UCLAMP_MIN);
> >
> > That's a very complicated way or writing: return 0, right?
>
> In my mind it's just a simple way to hardcode values in just one place.
>
> In the current implementation uclamp_none(UCLAMP_MIN) is 0 and the
> compiler is not in trubles to inline a 0 there.
>
> Is it really so disgusting ?
Not disguisting per se, just complicated. It had me go back and check
wth uclamp_none() did again.
^ permalink raw reply
* Re: [PATCH v7 01/15] sched/core: uclamp: Add CPU's clamp buckets refcounting
From: Peter Zijlstra @ 2019-03-13 17:22 UTC (permalink / raw)
To: Patrick Bellasi
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190313161229.pkib2tmjass5chtb@e110439-lin>
On Wed, Mar 13, 2019 at 04:12:29PM +0000, Patrick Bellasi wrote:
> Yes, the for looks better, but perhaps like that:
>
> unsigned int bucket_id = UCLAMP_BUCKETS;
>
> /*
> * Both min and max clamps are MAX aggregated, thus the topmost
> * bucket with some tasks defines the rq's clamp value.
> */
> for (; bucket_id >= 0; --bucket_id) {
GCC will be clever and figure that unsigned will never be smaller than 0
and turn the above into an infinite loop or something daft.
That is; use 'int', not 'unsigned int' for bucket_id.
^ permalink raw reply
* Re: [PATCH v7 03/15] sched/core: uclamp: Add system default clamps
From: Patrick Bellasi @ 2019-03-13 17:09 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190313143240.GH5922@hirez.programming.kicks-ass.net>
On 13-Mar 15:32, Peter Zijlstra wrote:
> On Fri, Feb 08, 2019 at 10:05:42AM +0000, Patrick Bellasi wrote:
>
> > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > index 45460e7a3eee..447261cd23ba 100644
> > --- a/include/linux/sched.h
> > +++ b/include/linux/sched.h
> > @@ -584,14 +584,32 @@ struct sched_dl_entity {
> > * Utilization clamp for a scheduling entity
> > * @value: clamp value "requested" by a se
> > * @bucket_id: clamp bucket corresponding to the "requested" value
> > + * @effective: clamp value and bucket actually "assigned" to the se
> > + * @active: the se is currently refcounted in a rq's bucket
> > *
> > + * Both bucket_id and effective::bucket_id are the index of the clamp bucket
> > + * matching the corresponding clamp value which are pre-computed and stored to
> > + * avoid expensive integer divisions from the fast path.
> > + *
> > + * The active bit is set whenever a task has got an effective::value assigned,
> > + * which can be different from the user requested clamp value. This allows to
> > + * know a task is actually refcounting the rq's effective::bucket_id bucket.
> > */
> > struct uclamp_se {
> > + /* Clamp value "requested" by a scheduling entity */
> > unsigned int value : bits_per(SCHED_CAPACITY_SCALE);
> > unsigned int bucket_id : bits_per(UCLAMP_BUCKETS);
> > + unsigned int active : 1;
> > + /*
> > + * Clamp value "obtained" by a scheduling entity.
> > + *
> > + * This cache the actual clamp value, possibly enforced by system
> > + * default clamps, a task is subject to while enqueued in a rq.
> > + */
> > + struct {
> > + unsigned int value : bits_per(SCHED_CAPACITY_SCALE);
> > + unsigned int bucket_id : bits_per(UCLAMP_BUCKETS);
> > + } effective;
>
> I still think that this effective thing is backwards.
>
> The existing code already used @value and @bucket_id as 'effective' and
> you're now changing all that again. This really doesn't make sense to
> me.
With respect to the previous v6, I've now moved this concept to the
patch where we actually use it for the first time.
In this patch we add system default values, thus a task is now subject
to two possible constraints: the task specific (TS) one or the system
default (SD) one.
The most restrictive of the two must be enforced but we also want to
keep track of the task specific value, while system defaults are
enforce, to restore it when the system defaults are relaxed.
For example:
TS: |.............. 20 .................|
SD: |... 0 ....|..... 40 .....|... 0 ...|
Time: |..........|..............|.........|
t0 t1 t2 t3
Despite the task asking always only for a 20% boost:
- in [t1,t2] we want to boost it to 40% but, right after...
- in [t2,t3] we want to go back to the 20% boost.
The "effective" values allows to efficiently enforce the most
restrictive clamp value for a task at enqueue time by:
- not loosing track of the original request
- don't caring about updating non runnable tasks
> Also; if you don't add it inside struct uclamp_se, but add a second
> instance,
>
> > };
> > #endif /* CONFIG_UCLAMP_TASK */
> >
>
>
> > @@ -803,6 +811,70 @@ static inline void uclamp_rq_update(struct rq *rq, unsigned int clamp_id,
> > WRITE_ONCE(rq->uclamp[clamp_id].value, max_value);
> > }
> >
> > +/*
> > + * The effective clamp bucket index of a task depends on, by increasing
> > + * priority:
> > + * - the task specific clamp value, when explicitly requested from userspace
> > + * - the system default clamp value, defined by the sysadmin
> > + *
> > + * As a side effect, update the task's effective value:
> > + * task_struct::uclamp::effective::value
> > + * to represent the clamp value of the task effective bucket index.
> > + */
> > +static inline void
> > +uclamp_effective_get(struct task_struct *p, unsigned int clamp_id,
> > + unsigned int *clamp_value, unsigned int *bucket_id)
> > +{
> > + /* Task specific clamp value */
> > + *bucket_id = p->uclamp[clamp_id].bucket_id;
> > + *clamp_value = p->uclamp[clamp_id].value;
> > +
> > + /* Always apply system default restrictions */
> > + if (unlikely(*clamp_value > uclamp_default[clamp_id].value)) {
> > + *clamp_value = uclamp_default[clamp_id].value;
> > + *bucket_id = uclamp_default[clamp_id].bucket_id;
> > + }
> > +}
>
> you can avoid horrors like this and simply return a struct uclamp_se by
> value.
Yes, that should be possible... will look into splitting this out in
v8 to have something like:
---8<---
struct uclamp_req {
/* Clamp value "requested" by a scheduling entity */
unsigned int value : bits_per(SCHED_CAPACITY_SCALE);
unsigned int bucket_id : bits_per(UCLAMP_BUCKETS);
unsigned int active : 1;
unsigned int user_defined : 1;
}
struct uclamp_eff {
unsigned int value : bits_per(SCHED_CAPACITY_SCALE);
unsigned int bucket_id : bits_per(UCLAMP_BUCKETS);
}
struct task_struct {
// ...
#ifdef CONFIG_UCLAMP_TASK
struct uclamp_req uclamp_req[UCLAMP_CNT];
struct uclamp_eff uclamp_eff[UCLAMP_CNT];
#endif
// ...
}
static inline struct uclamp_eff
uclamp_eff_get(struct task_struct *p, unsigned int clamp_id)
{
struct uclamp_eff uc_eff = p->uclamp_eff[clamp_id];
uc_eff.bucket_id = p->uclamp_req[clamp_id].bucket_id;
uc_eff.clamp_value = p->uclamp_req[clamp_id].value;
if (unlikely(uc_eff.clamp_value > uclamp_default[clamp_id].value)) {
uc_eff.clamp_value = uclamp_default[clamp_id].value;
uc_eff.bucket_id = uclamp_default[clamp_id].bucket_id;
}
return uc_eff;
}
static inline void
uclamp_eff_set(struct task_struct *p, unsigned int clamp_id)
{
p->uclamp_eff[clamp_id] = uclamp_eff_get(p, clamp_id);
}
---8<---
Is that what you mean ?
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v7 02/15] sched/core: uclamp: Enforce last task UCLAMP_MAX
From: Patrick Bellasi @ 2019-03-13 16:20 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190313141008.GF5922@hirez.programming.kicks-ass.net>
On 13-Mar 15:10, Peter Zijlstra wrote:
> On Fri, Feb 08, 2019 at 10:05:41AM +0000, Patrick Bellasi wrote:
> > +uclamp_idle_value(struct rq *rq, unsigned int clamp_id, unsigned int clamp_value)
> > +{
> > + /*
> > + * Avoid blocked utilization pushing up the frequency when we go
> > + * idle (which drops the max-clamp) by retaining the last known
> > + * max-clamp.
> > + */
> > + if (clamp_id == UCLAMP_MAX) {
> > + rq->uclamp_flags |= UCLAMP_FLAG_IDLE;
> > + return clamp_value;
> > + }
> > +
> > + return uclamp_none(UCLAMP_MIN);
>
> That's a very complicated way or writing: return 0, right?
In my mind it's just a simple way to hardcode values in just one place.
In the current implementation uclamp_none(UCLAMP_MIN) is 0 and the
compiler is not in trubles to inline a 0 there.
Is it really so disgusting ?
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v7 02/15] sched/core: uclamp: Enforce last task UCLAMP_MAX
From: Patrick Bellasi @ 2019-03-13 16:16 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190313141253.GG5922@hirez.programming.kicks-ass.net>
On 13-Mar 15:12, Peter Zijlstra wrote:
> On Fri, Feb 08, 2019 at 10:05:41AM +0000, Patrick Bellasi wrote:
> > +static inline void uclamp_idle_reset(struct rq *rq, unsigned int clamp_id,
> > + unsigned int clamp_value)
> > +{
> > + /* Reset max-clamp retention only on idle exit */
> > + if (!(rq->uclamp_flags & UCLAMP_FLAG_IDLE))
> > + return;
> > +
> > + WRITE_ONCE(rq->uclamp[clamp_id].value, clamp_value);
> > +
> > + /*
> > + * This function is called for both UCLAMP_MIN (before) and UCLAMP_MAX
> > + * (after). The idle flag is reset only the second time, when we know
> > + * that UCLAMP_MIN has been already updated.
>
> Why do we care? That is, what is this comment trying to tell us.
Right, the code is clear enough, I'll remove this comment.
>
> > + */
> > + if (clamp_id == UCLAMP_MAX)
> > + rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
> > +}
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v7 01/15] sched/core: uclamp: Add CPU's clamp buckets refcounting
From: Patrick Bellasi @ 2019-03-13 16:12 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190313134022.GB5922@hirez.programming.kicks-ass.net>
On 13-Mar 14:40, Peter Zijlstra wrote:
> On Fri, Feb 08, 2019 at 10:05:40AM +0000, Patrick Bellasi wrote:
> > +static inline unsigned int uclamp_bucket_id(unsigned int clamp_value)
> > +{
> > + return clamp_value / UCLAMP_BUCKET_DELTA;
> > +}
> > +
> > +static inline unsigned int uclamp_bucket_value(unsigned int clamp_value)
> > +{
> > + return UCLAMP_BUCKET_DELTA * uclamp_bucket_id(clamp_value);
>
> return clamp_value - (clamp_value % UCLAMP_BUCKET_DELTA);
>
> might generate better code; just a single division, instead of a div and
> mult.
Wondering if compilers cannot do these optimizations... but yes, looks
cool and will do it in v8, thanks.
> > +}
> > +
> > +static inline unsigned int uclamp_none(int clamp_id)
> > +{
> > + if (clamp_id == UCLAMP_MIN)
> > + return 0;
> > + return SCHED_CAPACITY_SCALE;
> > +}
> > +
> > +static inline void uclamp_rq_update(struct rq *rq, unsigned int clamp_id)
> > +{
> > + struct uclamp_bucket *bucket = rq->uclamp[clamp_id].bucket;
> > + unsigned int max_value = uclamp_none(clamp_id);
> > + unsigned int bucket_id;
> > +
> > + /*
> > + * Both min and max clamps are MAX aggregated, thus the topmost
> > + * bucket with some tasks defines the rq's clamp value.
> > + */
> > + bucket_id = UCLAMP_BUCKETS;
> > + do {
> > + --bucket_id;
> > + if (!rq->uclamp[clamp_id].bucket[bucket_id].tasks)
> > + continue;
> > + max_value = bucket[bucket_id].value;
> > + break;
>
> If you flip the if condition the code will be nicer.
>
> > + } while (bucket_id);
>
> But you can also use a for loop:
>
> for (i = UCLAMP_BUCKETS-1; i>=0; i--) {
> if (rq->uclamp[clamp_id].bucket[i].tasks) {
> max_value = bucket[i].value;
> break;
> }
> }
Yes, the for looks better, but perhaps like that:
unsigned int bucket_id = UCLAMP_BUCKETS;
/*
* Both min and max clamps are MAX aggregated, thus the topmost
* bucket with some tasks defines the rq's clamp value.
*/
for (; bucket_id >= 0; --bucket_id) {
if (!bucket[bucket_id].tasks)
continue;
max_value = bucket[bucket_id].value;
break;
}
... just to save a {} block.
> > + WRITE_ONCE(rq->uclamp[clamp_id].value, max_value);
> > +}
--
#include <best/regards.h>
Patrick Bellasi
^ 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