Linux userland API discussions
 help / color / mirror / Atom feed
* [PATCH net-next v2 04/12] net: homa: create homa_pool.h and homa_pool.c
From: John Ousterhout @ 2024-11-11 23:39 UTC (permalink / raw)
  To: netdev, linux-api; +Cc: John Ousterhout
In-Reply-To: <20241111234006.5942-1-ouster@cs.stanford.edu>

These files implement Homa's mechanism for managing application-level
buffer space for incoming messages This mechanism is needed to allow
Homa to copy data out to user space in parallel with receiving packets;
it was discussed in a talk at NetDev 0x17.

Signed-off-by: John Ousterhout <ouster@cs.stanford.edu>
---
 net/homa/homa_pool.c | 420 +++++++++++++++++++++++++++++++++++++++++++
 net/homa/homa_pool.h | 152 ++++++++++++++++
 2 files changed, 572 insertions(+)
 create mode 100644 net/homa/homa_pool.c
 create mode 100644 net/homa/homa_pool.h

diff --git a/net/homa/homa_pool.c b/net/homa/homa_pool.c
new file mode 100644
index 000000000000..4d166eb4ac19
--- /dev/null
+++ b/net/homa/homa_pool.c
@@ -0,0 +1,420 @@
+// SPDX-License-Identifier: BSD-2-Clause
+
+#include "homa_impl.h"
+#include "homa_pool.h"
+
+/* This file contains functions that manage user-space buffer pools. */
+
+/* Pools must always have at least this many bpages (no particular
+ * reasoning behind this value).
+ */
+#define MIN_POOL_SIZE 2
+
+/* Used when determining how many bpages to consider for allocation. */
+#define MIN_EXTRA 4
+
+/**
+ * set_bpages_needed() - Set the bpages_needed field of @pool based
+ * on the length of the first RPC that's waiting for buffer space.
+ * The caller must own the lock for @pool->hsk.
+ * @pool: Pool to update.
+ */
+static void set_bpages_needed(struct homa_pool *pool)
+{
+	struct homa_rpc *rpc = list_first_entry(&pool->hsk->waiting_for_bufs,
+			struct homa_rpc, buf_links);
+	pool->bpages_needed = (rpc->msgin.length + HOMA_BPAGE_SIZE - 1)
+			>> HOMA_BPAGE_SHIFT;
+}
+
+/**
+ * homa_pool_init() - Initialize a homa_pool; any previous contents of the
+ * objects are overwritten.
+ * @hsk:          Socket containing the pool to initialize.
+ * @region:       First byte of the memory region for the pool, allocated
+ *                by the application; must be page-aligned.
+ * @region_size:  Total number of bytes available at @buf_region.
+ * Return: Either zero (for success) or a negative errno for failure.
+ */
+int homa_pool_init(struct homa_sock *hsk, void __user *region, __u64 region_size)
+{
+	struct homa_pool *pool = hsk->buffer_pool;
+	int i, result;
+
+	if (((uintptr_t)region) & ~PAGE_MASK)
+		return -EINVAL;
+	pool->hsk = hsk;
+	pool->region = (char __user *)region;
+	pool->num_bpages = region_size >> HOMA_BPAGE_SHIFT;
+	pool->descriptors = NULL;
+	pool->cores = NULL;
+	if (pool->num_bpages < MIN_POOL_SIZE) {
+		result = -EINVAL;
+		goto error;
+	}
+	pool->descriptors = kmalloc_array(pool->num_bpages,
+					  sizeof(struct homa_bpage), GFP_ATOMIC);
+	if (!pool->descriptors) {
+		result = -ENOMEM;
+		goto error;
+	}
+	for (i = 0; i < pool->num_bpages; i++) {
+		struct homa_bpage *bp = &pool->descriptors[i];
+
+		spin_lock_init(&bp->lock);
+		atomic_set(&bp->refs, 0);
+		bp->owner = -1;
+		bp->expiration = 0;
+	}
+	atomic_set(&pool->free_bpages, pool->num_bpages);
+	pool->bpages_needed = INT_MAX;
+
+	/* Allocate and initialize core-specific data. */
+	pool->cores = kmalloc_array(nr_cpu_ids, sizeof(struct homa_pool_core),
+				    GFP_ATOMIC);
+	if (!pool->cores) {
+		result = -ENOMEM;
+		goto error;
+	}
+	pool->num_cores = nr_cpu_ids;
+	for (i = 0; i < pool->num_cores; i++) {
+		pool->cores[i].page_hint = 0;
+		pool->cores[i].allocated = 0;
+		pool->cores[i].next_candidate = 0;
+	}
+	pool->check_waiting_invoked = 0;
+
+	return 0;
+
+error:
+	kfree(pool->descriptors);
+	kfree(pool->cores);
+	pool->region = NULL;
+	return result;
+}
+
+/**
+ * homa_pool_destroy() - Destructor for homa_pool. After this method
+ * returns, the object should not be used unless it has been reinitialized.
+ * @pool: Pool to destroy.
+ */
+void homa_pool_destroy(struct homa_pool *pool)
+{
+	if (!pool->region)
+		return;
+	kfree(pool->descriptors);
+	kfree(pool->cores);
+	pool->region = NULL;
+}
+
+/**
+ * homa_pool_get_pages() - Allocate one or more full pages from the pool.
+ * @pool:         Pool from which to allocate pages
+ * @num_pages:    Number of pages needed
+ * @pages:        The indices of the allocated pages are stored here; caller
+ *                must ensure this array is big enough. Reference counts have
+ *                been set to 1 on all of these pages (or 2 if set_owner
+ *                was specified).
+ * @set_owner:    If nonzero, the current core is marked as owner of all
+ *                of the allocated pages (and the expiration time is also
+ *                set). Otherwise the pages are left unowned.
+ * Return: 0 for success, -1 if there wasn't enough free space in the pool.
+ */
+int homa_pool_get_pages(struct homa_pool *pool, int num_pages, __u32 *pages,
+			int set_owner)
+{
+	int core_num = raw_smp_processor_id();
+	struct homa_pool_core *core;
+	__u64 now = sched_clock();
+	int alloced = 0;
+	int limit = 0;
+
+	core = &pool->cores[core_num];
+	if (atomic_sub_return(num_pages, &pool->free_bpages) < 0) {
+		atomic_add(num_pages, &pool->free_bpages);
+		return -1;
+	}
+
+	/* Once we get to this point we know we will be able to find
+	 * enough free pages; now we just have to find them.
+	 */
+	while (alloced != num_pages) {
+		struct homa_bpage *bpage;
+		int cur, ref_count;
+
+		/* If we don't need to use all of the bpages in the pool,
+		 * then try to use only the ones with low indexes. This
+		 * will reduce the cache footprint for the pool by reusing
+		 * a few bpages over and over. Specifically this code will
+		 * not consider any candidate page whose index is >= limit.
+		 * Limit is chosen to make sure there are a reasonable
+		 * number of free pages in the range, so we won't have to
+		 * check a huge number of pages.
+		 */
+		if (limit == 0) {
+			int extra;
+
+			limit = pool->num_bpages
+					- atomic_read(&pool->free_bpages);
+			extra = limit >> 2;
+			limit += (extra < MIN_EXTRA) ? MIN_EXTRA : extra;
+			if (limit > pool->num_bpages)
+				limit = pool->num_bpages;
+		}
+
+		cur = core->next_candidate;
+		core->next_candidate++;
+		if (cur >= limit) {
+			core->next_candidate = 0;
+
+			/* Must recompute the limit for each new loop through
+			 * the bpage array: we may need to consider a larger
+			 * range of pages because of concurrent allocations.
+			 */
+			limit = 0;
+			continue;
+		}
+		bpage = &pool->descriptors[cur];
+
+		/* Figure out whether this candidate is free (or can be
+		 * stolen). Do a quick check without locking the page, and
+		 * if the page looks promising, then lock it and check again
+		 * (must check again in case someone else snuck in and
+		 * grabbed the page).
+		 */
+		ref_count = atomic_read(&bpage->refs);
+		if (ref_count >= 2 || (ref_count == 1 && (bpage->owner < 0 ||
+							  bpage->expiration > now)))
+			continue;
+		if (!spin_trylock_bh(&bpage->lock))
+			continue;
+		ref_count = atomic_read(&bpage->refs);
+		if (ref_count >= 2 || (ref_count == 1 && (bpage->owner < 0 ||
+							  bpage->expiration > now))) {
+			spin_unlock_bh(&bpage->lock);
+			continue;
+		}
+		if (bpage->owner >= 0)
+			atomic_inc(&pool->free_bpages);
+		if (set_owner) {
+			atomic_set(&bpage->refs, 2);
+			bpage->owner = core_num;
+			bpage->expiration = now +
+					1000 * pool->hsk->homa->bpage_lease_usecs;
+		} else {
+			atomic_set(&bpage->refs, 1);
+			bpage->owner = -1;
+		}
+		spin_unlock_bh(&bpage->lock);
+		pages[alloced] = cur;
+		alloced++;
+	}
+	return 0;
+}
+
+/**
+ * homa_pool_allocate() - Allocate buffer space for an RPC.
+ * @rpc:  RPC that needs space allocated for its incoming message (space must
+ *        not already have been allocated). The fields @msgin->num_buffers
+ *        and @msgin->buffers are filled in. Must be locked by caller.
+ * Return: The return value is normally 0, which means either buffer space
+ * was allocated or the @rpc was queued on @hsk->waiting. If a fatal error
+ * occurred, such as no buffer pool present, then a negative errno is
+ * returned.
+ */
+int homa_pool_allocate(struct homa_rpc *rpc)
+{
+	struct homa_pool *pool = rpc->hsk->buffer_pool;
+	int full_pages, partial, i, core_id;
+	__u32 pages[HOMA_MAX_BPAGES];
+	struct homa_pool_core *core;
+	struct homa_bpage *bpage;
+	struct homa_rpc *other;
+
+	if (!pool->region)
+		return -ENOMEM;
+
+	/* First allocate any full bpages that are needed. */
+	full_pages = rpc->msgin.length >> HOMA_BPAGE_SHIFT;
+	if (unlikely(full_pages)) {
+		if (homa_pool_get_pages(pool, full_pages, pages, 0) != 0)
+			goto out_of_space;
+		for (i = 0; i < full_pages; i++)
+			rpc->msgin.bpage_offsets[i] = pages[i] << HOMA_BPAGE_SHIFT;
+	}
+	rpc->msgin.num_bpages = full_pages;
+
+	/* The last chunk may be less than a full bpage; for this we use
+	 * the bpage that we own (and reuse it for multiple messages).
+	 */
+	partial = rpc->msgin.length & (HOMA_BPAGE_SIZE - 1);
+	if (unlikely(partial == 0))
+		goto success;
+	core_id = raw_smp_processor_id();
+	core = &pool->cores[core_id];
+	bpage = &pool->descriptors[core->page_hint];
+	if (!spin_trylock_bh(&bpage->lock))
+		spin_lock_bh(&bpage->lock);
+	if (bpage->owner != core_id) {
+		spin_unlock_bh(&bpage->lock);
+		goto new_page;
+	}
+	if ((core->allocated + partial) > HOMA_BPAGE_SIZE) {
+		if (atomic_read(&bpage->refs) == 1) {
+			/* Bpage is totally free, so we can reuse it. */
+			core->allocated = 0;
+		} else {
+			bpage->owner = -1;
+
+			/* We know the reference count can't reach zero here
+			 * because of check above, so we won't have to decrement
+			 * pool->free_bpages.
+			 */
+			atomic_dec_return(&bpage->refs);
+			spin_unlock_bh(&bpage->lock);
+			goto new_page;
+		}
+	}
+	bpage->expiration = sched_clock() +
+			1000 * pool->hsk->homa->bpage_lease_usecs;
+	atomic_inc(&bpage->refs);
+	spin_unlock_bh(&bpage->lock);
+	goto allocate_partial;
+
+	/* Can't use the current page; get another one. */
+new_page:
+	if (homa_pool_get_pages(pool, 1, pages, 1) != 0) {
+		homa_pool_release_buffers(pool, rpc->msgin.num_bpages,
+					  rpc->msgin.bpage_offsets);
+		rpc->msgin.num_bpages = 0;
+		goto out_of_space;
+	}
+	core->page_hint = pages[0];
+	core->allocated = 0;
+
+allocate_partial:
+	rpc->msgin.bpage_offsets[rpc->msgin.num_bpages] = core->allocated
+			+ (core->page_hint << HOMA_BPAGE_SHIFT);
+	rpc->msgin.num_bpages++;
+	core->allocated += partial;
+
+success:
+	return 0;
+
+	/* We get here if there wasn't enough buffer space for this
+	 * message; add the RPC to hsk->waiting_for_bufs.
+	 */
+out_of_space:
+	homa_sock_lock(pool->hsk, "homa_pool_allocate");
+	list_for_each_entry(other, &pool->hsk->waiting_for_bufs, buf_links) {
+		if (other->msgin.length > rpc->msgin.length) {
+			list_add_tail(&rpc->buf_links, &other->buf_links);
+			goto queued;
+		}
+	}
+	list_add_tail_rcu(&rpc->buf_links, &pool->hsk->waiting_for_bufs);
+
+queued:
+	set_bpages_needed(pool);
+	homa_sock_unlock(pool->hsk);
+	return 0;
+}
+
+/**
+ * homa_pool_get_buffer() - Given an RPC, figure out where to store incoming
+ * message data.
+ * @rpc:        RPC for which incoming message data is being processed; its
+ *              msgin must be properly initialized and buffer space must have
+ *              been allocated for the message.
+ * @offset:     Offset within @rpc's incoming message.
+ * @available:  Will be filled in with the number of bytes of space available
+ *              at the returned address.
+ * Return:      The application's virtual address for buffer space corresponding
+ *              to @offset in the incoming message for @rpc.
+ */
+void __user *homa_pool_get_buffer(struct homa_rpc *rpc, int offset,
+				  int *available)
+{
+	int bpage_index, bpage_offset;
+
+	bpage_index = offset >> HOMA_BPAGE_SHIFT;
+	BUG_ON(bpage_index >= rpc->msgin.num_bpages);
+	bpage_offset = offset & (HOMA_BPAGE_SIZE - 1);
+	*available = (bpage_index < (rpc->msgin.num_bpages - 1))
+			? HOMA_BPAGE_SIZE - bpage_offset
+			: rpc->msgin.length - offset;
+	return rpc->hsk->buffer_pool->region + rpc->msgin.bpage_offsets[bpage_index]
+			+ bpage_offset;
+}
+
+/**
+ * homa_pool_release_buffers() - Release buffer space so that it can be
+ * reused.
+ * @pool:         Pool that the buffer space belongs to. Doesn't need to
+ *                be locked.
+ * @num_buffers:  How many buffers to release.
+ * @buffers:      Points to @num_buffers values, each of which is an offset
+ *                from the start of the pool to the buffer to be released.
+ */
+void homa_pool_release_buffers(struct homa_pool *pool, int num_buffers,
+			       __u32 *buffers)
+{
+	int i;
+
+	if (!pool->region)
+		return;
+	for (i = 0; i < num_buffers; i++) {
+		__u32 bpage_index = buffers[i] >> HOMA_BPAGE_SHIFT;
+		struct homa_bpage *bpage = &pool->descriptors[bpage_index];
+
+		if (bpage_index < pool->num_bpages)
+			if (atomic_dec_return(&bpage->refs) == 0)
+				atomic_inc(&pool->free_bpages);
+		}
+	}
+
+/**
+ * homa_pool_check_waiting() - Checks to see if there are enough free
+ * bpages to wake up any RPCs that were blocked. Whenever
+ * homa_pool_release_buffers is invoked, this function must be invoked later,
+ * at a point when the caller holds no locks (homa_pool_release_buffers may
+ * be invoked with locks held, so it can't safely invoke this function).
+ * This is regrettably tricky, but I can't think of a better solution.
+ * @pool:         Information about the buffer pool.
+ */
+void homa_pool_check_waiting(struct homa_pool *pool)
+{
+	while (atomic_read(&pool->free_bpages) >= pool->bpages_needed) {
+		struct homa_rpc *rpc;
+
+		homa_sock_lock(pool->hsk, "buffer pool");
+		if (list_empty(&pool->hsk->waiting_for_bufs)) {
+			pool->bpages_needed = INT_MAX;
+			homa_sock_unlock(pool->hsk);
+			break;
+		}
+		rpc = list_first_entry(&pool->hsk->waiting_for_bufs,
+				       struct homa_rpc, buf_links);
+		if (!homa_bucket_try_lock(rpc->bucket, rpc->id,
+					  "homa_pool_check_waiting")) {
+			/* Can't just spin on the RPC lock because we're
+			 * holding the socket lock (see sync.txt). Instead,
+			 * release the socket lock and try the entire
+			 * operation again.
+			 */
+			homa_sock_unlock(pool->hsk);
+			continue;
+		}
+		list_del_init(&rpc->buf_links);
+		if (list_empty(&pool->hsk->waiting_for_bufs))
+			pool->bpages_needed = INT_MAX;
+		else
+			set_bpages_needed(pool);
+		homa_sock_unlock(pool->hsk);
+		homa_pool_allocate(rpc);
+		if (rpc->msgin.num_bpages > 0)
+			/* Allocation succeeded; "wake up" the RPC. */
+			rpc->msgin.resend_all = 1;
+		homa_rpc_unlock(rpc);
+	}
+}
diff --git a/net/homa/homa_pool.h b/net/homa/homa_pool.h
new file mode 100644
index 000000000000..609d99b59707
--- /dev/null
+++ b/net/homa/homa_pool.h
@@ -0,0 +1,152 @@
+/* SPDX-License-Identifier: BSD-2-Clause */
+
+/* This file contains definitions used to manage user-space buffer pools.
+ */
+
+#ifndef _HOMA_POOL_H
+#define _HOMA_POOL_H
+
+#include "homa_rpc.h"
+
+/**
+ * struct homa_bpage - Contains information about a single page in
+ * a buffer pool.
+ */
+struct homa_bpage {
+	union {
+		/**
+		 * @cache_line: Ensures that each homa_bpage object
+		 * is exactly one cache line long.
+		 */
+		char cache_line[L1_CACHE_BYTES];
+		struct {
+			/** @lock: to synchronize shared access. */
+			spinlock_t lock;
+
+			/**
+			 * @refs: Counts number of distinct uses of this
+			 * bpage (1 tick for each message that is using
+			 * this page, plus an additional tick if the @owner
+			 * field is set).
+			 */
+			atomic_t refs;
+
+			/**
+			 * @owner: kernel core that currently owns this page
+			 * (< 0 if none).
+			 */
+			int owner;
+
+			/**
+			 * @expiration: time (in sched_clock() units) after
+			 * which it's OK to steal this page from its current
+			 * owner (if @refs is 1).
+			 */
+			__u64 expiration;
+		};
+	};
+};
+
+/**
+ * struct homa_pool_core - Holds core-specific data for a homa_pool (a bpage
+ * out of which that core is allocating small chunks).
+ */
+struct homa_pool_core {
+	union {
+		/**
+		 * @cache_line: Ensures that each object is exactly one
+		 * cache line long.
+		 */
+		char cache_line[L1_CACHE_BYTES];
+		struct {
+			/**
+			 * @page_hint: Index of bpage in pool->descriptors,
+			 * which may be owned by this core. If so, we'll use it
+			 * for allocating partial pages.
+			 */
+			int page_hint;
+
+			/**
+			 * @allocated: if the page given by @page_hint is
+			 * owned by this core, this variable gives the number of
+			 * (initial) bytes that have already been allocated
+			 * from the page.
+			 */
+			int allocated;
+
+			/**
+			 * @next_candidate: when searching for free bpages,
+			 * check this index next.
+			 */
+			int next_candidate;
+		};
+	};
+};
+
+/**
+ * struct homa_pool - Describes a pool of buffer space for incoming
+ * messages for a particular socket; managed by homa_pool.c. The pool is
+ * divided up into "bpages", which are a multiple of the hardware page size.
+ * A bpage may be owned by a particular core so that it can more efficiently
+ * allocate space for small messages.
+ */
+struct homa_pool {
+	/**
+	 * @hsk: the socket that this pool belongs to.
+	 */
+	struct homa_sock *hsk;
+
+	/**
+	 * @region: beginning of the pool's region (in the app's virtual
+	 * memory). Divided into bpages. 0 means the pool hasn't yet been
+	 * initialized.
+	 */
+	char __user *region;
+
+	/** @num_bpages: total number of bpages in the pool. */
+	int num_bpages;
+
+	/** @descriptors: kmalloced area containing one entry for each bpage. */
+	struct homa_bpage *descriptors;
+
+	/**
+	 * @free_bpages: the number of pages still available for allocation
+	 * by homa_pool_get pages. This equals the number of pages with zero
+	 * reference counts, minus the number of pages that have been claimed
+	 * by homa_get_pool_pages but not yet allocated.
+	 */
+	atomic_t free_bpages;
+
+	/**
+	 * The number of free bpages required to satisfy the needs of the
+	 * first RPC on @hsk->waiting_for_bufs, or INT_MAX if that queue
+	 * is empty.
+	 */
+	int bpages_needed;
+
+	/** @cores: core-specific info; dynamically allocated. */
+	struct homa_pool_core *cores;
+
+	/** @num_cores: number of elements in @cores. */
+	int num_cores;
+
+	/**
+	 * @check_waiting_invoked: incremented during unit tests when
+	 * homa_pool_check_waiting is invoked.
+	 */
+	int check_waiting_invoked;
+};
+
+int      homa_pool_allocate(struct homa_rpc *rpc);
+void     homa_pool_check_waiting(struct homa_pool *pool);
+void     homa_pool_destroy(struct homa_pool *pool);
+void __user *homa_pool_get_buffer(struct homa_rpc *rpc, int offset,
+			      int *available);
+int      homa_pool_get_pages(struct homa_pool *pool, int num_pages,
+			     __u32 *pages, int leave_locked);
+int      homa_pool_init(struct homa_sock *hsk, void __user *buf_region,
+			__u64 region_size);
+void     homa_pool_release_buffers(struct homa_pool *pool,
+				   int num_buffers, __u32 *buffers);
+
+#endif /* _HOMA_POOL_H */
-- 
2.34.1


^ permalink raw reply related

* [PATCH net-next v2 00/12] Begin upstreaming Homa transport protocol
From: John Ousterhout @ 2024-11-11 23:39 UTC (permalink / raw)
  To: netdev, linux-api; +Cc: John Ousterhout

This patch series begins the process of upstreaming the Homa transport
protocol. Homa is an alternative to TCP for use in datacenter
environments. It provides 10-100x reductions in tail latency for short
messages relative to TCP. Its benefits are greatest for mixed workloads
containing both short and long messages running under high network loads.
Homa is not API-compatible with TCP: it is connectionless and message-
oriented (but still reliable and flow-controlled). Homa's new API not
only contributes to its performance gains, but it also eliminates the
massive amount of connection state required by TCP for highly connected
datacenter workloads.

For more details on Homa, please consult the Homa Wiki:
https://homa-transport.atlassian.net/wiki/spaces/HOMA/overview
The Wiki has pointers to two papers on Homa (one of which describes
this implementation) as well as man pages describing the application
API and other information.

There is also a GitHub repo for Homa:
https://github.com/PlatformLab/HomaModule
The GitHub repo contains a superset of this patch set, including:
* Additional source code that will eventually be upstreamed
* Extensive unit tests (which will also be upstreamed eventually)
* Application-level library functions (which need to go in glibc?)
* Man pages (which need to be upstreamed as well)
* Benchmarking and instrumentation code

For this patch series, Homa has been stripped down to the bare minimum
functionality capable of actually executing remote procedure calls. (about
7500 lines of source code, compared to 15000 in the complete Homa). The
remaining code will be upstreamed in smaller batches once this patch
series has been accepted. Note: the code in this patch series is
functional but its performance is not very interesting (about the same
as TCP).

The patch series is arranged to introduce the major functional components
of Homa. Until the last patch has been applied, the code is inert (it
will not be compiled).

Note: this implementation of Homa supports both IPv4 and IPv6.

John Ousterhout (12):
  net: homa: define user-visible API for Homa
  net: homa: define Homa packet formats
  net: homa: create shared Homa header files
  net: homa: create homa_pool.h and homa_pool.c
  net: homa: create homa_rpc.h and homa_rpc.c
  net: homa: create homa_peer.h and homa_peer.c
  net: homa: create homa_sock.h and homa_sock.c
  net: homa: create homa_incoming.c
  net: homa: create homa_outgoing.c
  net: homa: create homa_timer.c
  net: homa: create homa_plumbing.c homa_utils.c
  net: homa: create Makefile and Kconfig

 MAINTAINERS               |    7 +
 include/uapi/linux/homa.h |  165 ++++++
 net/Kconfig               |    1 +
 net/Makefile              |    1 +
 net/homa/Kconfig          |   19 +
 net/homa/Makefile         |   14 +
 net/homa/homa_impl.h      |  767 ++++++++++++++++++++++++++
 net/homa/homa_incoming.c  | 1076 +++++++++++++++++++++++++++++++++++++
 net/homa/homa_outgoing.c  |  854 +++++++++++++++++++++++++++++
 net/homa/homa_peer.c      |  319 +++++++++++
 net/homa/homa_peer.h      |  234 ++++++++
 net/homa/homa_plumbing.c  |  965 +++++++++++++++++++++++++++++++++
 net/homa/homa_pool.c      |  420 +++++++++++++++
 net/homa/homa_pool.h      |  152 ++++++
 net/homa/homa_rpc.c       |  488 +++++++++++++++++
 net/homa/homa_rpc.h       |  446 +++++++++++++++
 net/homa/homa_sock.c      |  380 +++++++++++++
 net/homa/homa_sock.h      |  426 +++++++++++++++
 net/homa/homa_stub.h      |   80 +++
 net/homa/homa_timer.c     |  156 ++++++
 net/homa/homa_utils.c     |  150 ++++++
 net/homa/homa_wire.h      |  378 +++++++++++++
 22 files changed, 7498 insertions(+)
 create mode 100644 include/uapi/linux/homa.h
 create mode 100644 net/homa/Kconfig
 create mode 100644 net/homa/Makefile
 create mode 100644 net/homa/homa_impl.h
 create mode 100644 net/homa/homa_incoming.c
 create mode 100644 net/homa/homa_outgoing.c
 create mode 100644 net/homa/homa_peer.c
 create mode 100644 net/homa/homa_peer.h
 create mode 100644 net/homa/homa_plumbing.c
 create mode 100644 net/homa/homa_pool.c
 create mode 100644 net/homa/homa_pool.h
 create mode 100644 net/homa/homa_rpc.c
 create mode 100644 net/homa/homa_rpc.h
 create mode 100644 net/homa/homa_sock.c
 create mode 100644 net/homa/homa_sock.h
 create mode 100644 net/homa/homa_stub.h
 create mode 100644 net/homa/homa_timer.c
 create mode 100644 net/homa/homa_utils.c
 create mode 100644 net/homa/homa_wire.h

--
2.34.1


^ permalink raw reply

* Re: [RFC PATCH 0/4] Add fbind() and NUMA mempolicy support for KVM guest_memfd
From: David Hildenbrand @ 2024-11-11 22:14 UTC (permalink / raw)
  To: Vlastimil Babka, Paolo Bonzini, Matthew Wilcox, Shivank Garg
  Cc: x86, viro, brauner, jack, akpm, linux-kernel, linux-fsdevel,
	linux-mm, linux-api, linux-arch, kvm, chao.gao, pgonda,
	thomas.lendacky, seanjc, luto, tglx, mingo, bp, dave.hansen, arnd,
	kees, bharata, nikunj, michael.day, Neeraj.Upadhyay, linux-coco
In-Reply-To: <ff3174f8-c8b5-4fae-a9d9-87546d37c162@suse.cz>

On 11.11.24 12:02, Vlastimil Babka wrote:
> On 11/8/24 18:31, Paolo Bonzini wrote:
>> On 11/7/24 16:10, Matthew Wilcox wrote:
>>> On Thu, Nov 07, 2024 at 02:24:20PM +0530, Shivank Garg wrote:
>>>> The folio allocation path from guest_memfd typically looks like this...
>>>>
>>>> kvm_gmem_get_folio
>>>>     filemap_grab_folio
>>>>       __filemap_get_folio
>>>>         filemap_alloc_folio
>>>>           __folio_alloc_node_noprof
>>>>             -> goes to the buddy allocator
>>>>
>>>> Hence, I am trying to have a version of filemap_alloc_folio() that takes an mpol.
>>>
>>> It only takes that path if cpuset_do_page_mem_spread() is true.  Is the
>>> real problem that you're trying to solve that cpusets are being used
>>> incorrectly?
>>
>> If it's false it's not very different, it goes to alloc_pages_noprof().
>> Then it respects the process's policy, but the policy is not
>> customizable without mucking with state that is global to the process.
>>
>> Taking a step back: the problem is that a VM can be configured to have
>> multiple guest-side NUMA nodes, each of which will pick memory from the
>> right NUMA node in the host.  Without a per-file operation it's not
>> possible to do this on guest_memfd.  The discussion was whether to use
>> ioctl() or a new system call.  The discussion ended with the idea of
>> posting a *proposal* asking for *comments* as to whether the system call
>> would be useful in general beyond KVM.
>>
>> Commenting on the system call itself I am not sure I like the
>> file_operations entry, though I understand that it's the simplest way to
>> implement this in an RFC series.  It's a bit surprising that fbind() is
>> a total no-op for everything except KVM's guest_memfd.
>>
>> Maybe whatever you pass to fbind() could be stored in the struct file *,
>> and used as the default when creating VMAs; as if every mmap() was
>> followed by an mbind(), except that it also does the right thing with
>> MAP_POPULATE for example.  Or maybe that's a horrible idea?
> 
> mbind() manpage has this:
> 
>         The  specified  policy  will  be  ignored  for  any MAP_SHARED
> mappings in the specified memory range.  Rather the pages will be allocated
> according to the memory policy of the thread that caused the page to be
> allocated. Again, this may not be the thread that called mbind().

I recall discussing that a couple of times in the context of QEMU. I 
have some faint recollection that the manpage is a bit imprecise:

IIRC, hugetlb also ends up using the VMA policy for MAP_SHARED mappings 
during faults (huge_node()->get_vma_policy()) -- but in contrast to 
shmem, it doesn't end up becoming the "shared" policy for the file, used 
when accessed through other VMAs.

> 
> So that seems like we're not very keen on having one user of a file set a
> policy that would affect other users of the file?

For VMs in QEMU we really want to configure the policy once in the main 
process and have all other processes (e.g., vhost-user) not worry about 
that when they mmap() guest memory.

With shmem this works by "shared policy" design (below). For hugetlb, we 
rely on the fact that mbind()+MADV_POPULATE_WRITE allows us to 
preallocate NUMA-aware. So with hugetlb we really preallocate all guest 
RAM to guarantee the NUMA placement.

It would not be the worst idea to have a clean interface to configure 
file-range policies instead of having this weird shmem mbind() behavior 
and the hugetlb hack.

Having that said, other filesystem are rarely used for backing VMs, at 
least in combination with NUMA. So nobody really cared that much for now.

Maybe fbind() would primarily only be useful for in-memory filesystems 
(shmem/hugetlb/guest_memfd).

> 
> Now the next paragraph of the manpage says that shmem is different, and
> guest_memfd is more like shmem than a regular file.
> 
> My conclusion from that is that fbind() might be too broad and we don't want
> this for actual filesystem-backed files? And if it's limited to guest_memfd,
> it shouldn't be an fbind()?

I was just once again diving into how mbind() on shmem is handled. And 
in fact, mbind() will call vma->set_policy() to update the per 
file-range policy. I wonder why we didn't do the same for hugetlb ... 
but of course, hugetlb must be special in any possible way.

Not saying it's the best idea, but as we are talking about mmap support 
of guest_memfd (only allowing to fault in shared/faultable pages), one 
*could* look into implementing mbind()+vma->set_policy() for guest_memfd 
similar to how shmem handles it.

It would require a (temporary) dummy VMA in the worst case (all private 
memory).

It sounds a bit weird, though, to require a VMA to configure this, 
though. But at least it's similar to what shmem does ...

-- 
Cheers,

David / dhildenb


^ permalink raw reply

* Re: [RFC PATCH 0/4] Add fbind() and NUMA mempolicy support for KVM guest_memfd
From: Vlastimil Babka @ 2024-11-11 11:02 UTC (permalink / raw)
  To: Paolo Bonzini, Matthew Wilcox, Shivank Garg
  Cc: x86, viro, brauner, jack, akpm, linux-kernel, linux-fsdevel,
	linux-mm, linux-api, linux-arch, kvm, chao.gao, pgonda,
	thomas.lendacky, seanjc, luto, tglx, mingo, bp, dave.hansen, arnd,
	kees, bharata, nikunj, michael.day, Neeraj.Upadhyay, linux-coco
In-Reply-To: <10ffac79-0dba-4c30-991e-f3ca2b5ff639@redhat.com>

On 11/8/24 18:31, Paolo Bonzini wrote:
> On 11/7/24 16:10, Matthew Wilcox wrote:
>> On Thu, Nov 07, 2024 at 02:24:20PM +0530, Shivank Garg wrote:
>>> The folio allocation path from guest_memfd typically looks like this...
>>>
>>> kvm_gmem_get_folio
>>>    filemap_grab_folio
>>>      __filemap_get_folio
>>>        filemap_alloc_folio
>>>          __folio_alloc_node_noprof
>>>            -> goes to the buddy allocator
>>>
>>> Hence, I am trying to have a version of filemap_alloc_folio() that takes an mpol.
>> 
>> It only takes that path if cpuset_do_page_mem_spread() is true.  Is the
>> real problem that you're trying to solve that cpusets are being used
>> incorrectly?
> 
> If it's false it's not very different, it goes to alloc_pages_noprof(). 
> Then it respects the process's policy, but the policy is not 
> customizable without mucking with state that is global to the process.
> 
> Taking a step back: the problem is that a VM can be configured to have 
> multiple guest-side NUMA nodes, each of which will pick memory from the 
> right NUMA node in the host.  Without a per-file operation it's not 
> possible to do this on guest_memfd.  The discussion was whether to use 
> ioctl() or a new system call.  The discussion ended with the idea of 
> posting a *proposal* asking for *comments* as to whether the system call 
> would be useful in general beyond KVM.
> 
> Commenting on the system call itself I am not sure I like the 
> file_operations entry, though I understand that it's the simplest way to 
> implement this in an RFC series.  It's a bit surprising that fbind() is 
> a total no-op for everything except KVM's guest_memfd.
> 
> Maybe whatever you pass to fbind() could be stored in the struct file *, 
> and used as the default when creating VMAs; as if every mmap() was 
> followed by an mbind(), except that it also does the right thing with 
> MAP_POPULATE for example.  Or maybe that's a horrible idea?

mbind() manpage has this:

       The  specified  policy  will  be  ignored  for  any MAP_SHARED
mappings in the specified memory range.  Rather the pages will be allocated
according to the memory policy of the thread that caused the page to be
allocated. Again, this may not be the thread that called mbind().

So that seems like we're not very keen on having one user of a file set a
policy that would affect other users of the file?

Now the next paragraph of the manpage says that shmem is different, and
guest_memfd is more like shmem than a regular file.

My conclusion from that is that fbind() might be too broad and we don't want
this for actual filesystem-backed files? And if it's limited to guest_memfd,
it shouldn't be an fbind()?

> Adding linux-api to get input; original thread is at
> https://lore.kernel.org/kvm/20241105164549.154700-1-shivankg@amd.com/.
> 
> Paolo
> 
>> Backing up, it seems like you want to make a change to the page cache,
>> you've had a long discussion with people who aren't the page cache
>> maintainer, and you all understand the pros and cons of everything,
>> and here you are dumping a solution on me without talking to me, even
>> though I was at Plumbers, you didn't find me to tell me I needed to go
>> to your talk.
>> 
>> So you haven't explained a damned thing to me, and I'm annoyed at you.
>> Do better.  Starting with your cover letter.
>> 
> 
> 


^ permalink raw reply

* Re: [RFC PATCH 0/4] Add fbind() and NUMA mempolicy support for KVM guest_memfd
From: Paolo Bonzini @ 2024-11-08 17:31 UTC (permalink / raw)
  To: Matthew Wilcox, Shivank Garg
  Cc: x86, viro, brauner, jack, akpm, linux-kernel, linux-fsdevel,
	linux-mm, linux-api, linux-arch, kvm, chao.gao, pgonda,
	thomas.lendacky, seanjc, luto, tglx, mingo, bp, dave.hansen, arnd,
	kees, bharata, nikunj, michael.day, Neeraj.Upadhyay, linux-coco,
	Linux API
In-Reply-To: <ZyzYUOX_r3uWin5f@casper.infradead.org>

On 11/7/24 16:10, Matthew Wilcox wrote:
> On Thu, Nov 07, 2024 at 02:24:20PM +0530, Shivank Garg wrote:
>> The folio allocation path from guest_memfd typically looks like this...
>>
>> kvm_gmem_get_folio
>>    filemap_grab_folio
>>      __filemap_get_folio
>>        filemap_alloc_folio
>>          __folio_alloc_node_noprof
>>            -> goes to the buddy allocator
>>
>> Hence, I am trying to have a version of filemap_alloc_folio() that takes an mpol.
> 
> It only takes that path if cpuset_do_page_mem_spread() is true.  Is the
> real problem that you're trying to solve that cpusets are being used
> incorrectly?

If it's false it's not very different, it goes to alloc_pages_noprof(). 
Then it respects the process's policy, but the policy is not 
customizable without mucking with state that is global to the process.

Taking a step back: the problem is that a VM can be configured to have 
multiple guest-side NUMA nodes, each of which will pick memory from the 
right NUMA node in the host.  Without a per-file operation it's not 
possible to do this on guest_memfd.  The discussion was whether to use 
ioctl() or a new system call.  The discussion ended with the idea of 
posting a *proposal* asking for *comments* as to whether the system call 
would be useful in general beyond KVM.

Commenting on the system call itself I am not sure I like the 
file_operations entry, though I understand that it's the simplest way to 
implement this in an RFC series.  It's a bit surprising that fbind() is 
a total no-op for everything except KVM's guest_memfd.

Maybe whatever you pass to fbind() could be stored in the struct file *, 
and used as the default when creating VMAs; as if every mmap() was 
followed by an mbind(), except that it also does the right thing with 
MAP_POPULATE for example.  Or maybe that's a horrible idea?

Adding linux-api to get input; original thread is at
https://lore.kernel.org/kvm/20241105164549.154700-1-shivankg@amd.com/.

Paolo

> Backing up, it seems like you want to make a change to the page cache,
> you've had a long discussion with people who aren't the page cache
> maintainer, and you all understand the pros and cons of everything,
> and here you are dumping a solution on me without talking to me, even
> though I was at Plumbers, you didn't find me to tell me I needed to go
> to your talk.
> 
> So you haven't explained a damned thing to me, and I'm annoyed at you.
> Do better.  Starting with your cover letter.
> 


^ permalink raw reply

* Re: [PATCH v6 2/5] pidfd: add PIDFD_SELF_* sentinels to refer to own thread/process
From: Lorenzo Stoakes @ 2024-11-08 14:28 UTC (permalink / raw)
  To: Christian Brauner
  Cc: Oleg Nesterov, Christian Brauner, Shuah Khan, Liam R . Howlett,
	Suren Baghdasaryan, Vlastimil Babka, pedro.falcato,
	linux-kselftest, linux-mm, linux-fsdevel, linux-api, linux-kernel,
	Oliver Sang, John Hubbard
In-Reply-To: <b8f4664c-b8f0-46ca-b9a3-8d73e398b5ca@lucifer.local>

On Wed, Oct 30, 2024 at 04:37:37PM +0000, Lorenzo Stoakes wrote:
> On Mon, Oct 28, 2024 at 04:06:07PM +0000, Lorenzo Stoakes wrote:
> > I guess I'll try to adapt that and respin a v7 when I get a chance.
>
> Hm looking at this draft patch, it seems like a total rework of pidfd's
> across the board right (now all pidfd's will need to be converted to
> pid_fd)? Correct me if I'm wrong.
>
> If only for the signal case, it seems like overkill to define a whole
> pid_fd and to use this CLASS() wrapper just for this one instance.
>
> If the intent is to convert _all_ pidfd's to use this type, it feels really
> out of scope for this series and I think we'd probably instead want to go
> off and do that as a separate series and put this on hold until that is
> done.
>
> If instead you mean that we ought to do something like this just for the
> signal case, it feels like it'd be quite a bit of extra abstraction just
> used in this one case but nowhere else, I think if you did an abstraction
> like this it would _have_ to be across the board right?
>
> I agree that the issue is with this one signal case that pins only the fd
> (rather than this pid) where this 'pinning' doesn't _necessary_ mess around
> with reference counts.
>
> So we definitely must address this, but the issue you had with the first
> approach was that I think (correct me if I'm wrong) I was passing a pointer
> to a struct fd which is not permitted right?
>
> Could we pass the struct fd by value to avoid this? I think we'd have to
> unfortunately special-case this and probably duplicate some code which is a
> pity as I liked the idea of abstracting everything to one place, but we can
> obviously do that.
>
> So I guess to TL;DR it, the options are:
>
> 1. Implement pid_fd everywhere, in which case I will leave off on
>    this series and I guess, if I have time I could look at trying to
>    implement that or perhaps you'd prefer to?
>
> 2. We are good for the sake of this series to special-case a pidfd_to_pid()
>    implementation (used only by the pidfd_send_signal() syscall)
>
> 3. Something else, or I am misunderstanding your point :)
>
> Let me know how you want me to proceed on this as we're at v6 already and I
> want to be _really_ sure I'm doing what you want here.
>
> Thanks!

Hi Christian,

Just a gentle nudge on this - as I need some guidance in order to know how
to move the series forwards.

Obviously no rush if your workload is high at the moment as this is pretty
low priority, but just in case you missed it :)

Thanks, Lorenzo

^ permalink raw reply

* Re: [RFC PATCH 0/4] Add fbind() and NUMA mempolicy support for KVM guest_memfd
From: Shivank Garg @ 2024-11-08  9:21 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: x86, viro, brauner, jack, akpm, linux-kernel, linux-fsdevel,
	linux-mm, linux-api, linux-arch, kvm, chao.gao, pgonda,
	thomas.lendacky, seanjc, luto, tglx, mingo, bp, dave.hansen, arnd,
	pbonzini, kees, bharata, nikunj, michael.day, Neeraj.Upadhyay,
	linux-coco
In-Reply-To: <ZyzYUOX_r3uWin5f@casper.infradead.org>



On 11/7/2024 8:40 PM, Matthew Wilcox wrote:
> On Thu, Nov 07, 2024 at 02:24:20PM +0530, Shivank Garg wrote:
>> The folio allocation path from guest_memfd typically looks like this...
>>
>> kvm_gmem_get_folio
>>   filemap_grab_folio
>>     __filemap_get_folio
>>       filemap_alloc_folio
>>         __folio_alloc_node_noprof
>>           -> goes to the buddy allocator
>>
>> Hence, I am trying to have a version of filemap_alloc_folio() that takes an mpol.
> 
> It only takes that path if cpuset_do_page_mem_spread() is true.  Is the
> real problem that you're trying to solve that cpusets are being used
> incorrectly?
> 
> Backing up, it seems like you want to make a change to the page cache,
> you've had a long discussion with people who aren't the page cache
> maintainer, and you all understand the pros and cons of everything,
> and here you are dumping a solution on me without talking to me, even
> though I was at Plumbers, you didn't find me to tell me I needed to go
> to your talk.
> 
> So you haven't explained a damned thing to me, and I'm annoyed at you.
> Do better.  Starting with your cover letter.

Hi Matthew,

I apologize for any misunderstanding and not providing adequate context.

To clarify:
- You may recall this work from its earlier iteration as an
  IOCTL-based approach, where you provided valuable review comments [1].
- I was not physically present at LPC. The discussion happened through
  the mailing list [2] and lobby discussion with my colleagues who visited
  Vienna.
- Based on feedback, particularly regarding the suggestion to consider
  fbind() as a more generic solution, we shifted to the current approach.

I posted this as *RFC* specifically to gather feedback on the feasibility of
this approach and to ensure I'm heading in the right direction.

Would you be willing to help me understand:
1. What additional information would be helpful to you and other reviewers?
2. How cpusets can be used correctly to fix this? (your point on
   cpuset_do_page_mem_spread() is interesting and I'll investigate it more
   thoroughly to understand).

I'll work on improving the cover letter to better explain the problem space
and proposed solution.

Thank you for the valuable feedback.

[1] https://lore.kernel.org/linux-mm/ZuimLtrpv1dXczf5@casper.infradead.org
[2] https://lore.kernel.org/linux-mm/ZvEga7srKhympQBt@intel.com

Best regards,
Shivank

^ permalink raw reply

* Re: [PATCH net 1/1] net/ipv6: Netlink flag for new IPv6 Default Routes
From: Matt Muggeridge @ 2024-11-08  2:20 UTC (permalink / raw)
  To: idosch
  Cc: Matt.Muggeridge, davem, dsahern, edumazet, horms, kuba, linux-api,
	linux-kernel, netdev, pabeni, stable
In-Reply-To: <ZyyN2bSgrpbhbkpp@shredder>

> > You probably already know how to reproduce it, but in case it helps, I still
> > have the packet captures and can share them with you. Let me know if you'd
> > like me to share them (and how to share them).
> 
> It would be best if you could provide a reproducer using iproute2:
> Configure a dummy device using ip-link, install the multipath route
> using ip-route, configure the neighbour table using ip-neigh and then
> perform route queries using "ip route get ..." showing the problem. We
> can then use it as the basis for a new test case in
> tools/testing/selftests/net/fib_tests.sh 

I'll try to do that next week.

> BTW, do you have CONFIG_IPV6_ROUTER_PREF=y in your config?

Yes.

$ gunzip -c /proc/config.gz | grep ROUTER_PREF
CONFIG_IPV6_ROUTER_PREF=y

> > 
> > As such, it still seems appropriate (to me) that this be implemented in the
> > legacy API as well as ensuring it works with the NH API.
> 
> As I understand it you currently get different results because the
> kernel installs two default routes whereas user space can only create
> one default multipath route.

Yes, that's the end result of an underlying problem.

Perhaps more to the point, the fact that a coalesced, INCOMPLETE, multipath
route is selected when a REACHABLE alternative exists, is what prevents us
from using coalesced multipath routes. This seems like a bug, since it violates
RFC4861 6.3.6, bullet 1.

Imagine adding a 2nd router to an IPv6 network for added resiliency, but when
one becomes unreachable, some network flows keep choosing the unreachable
router. This is what is happening with ECMP routes. It doesn't happen with
multiple default routes.

I'll just reiterate earlier comments, this doesn't happen all of the time.
It seems I have a 50/50 chance of the INCOMPLETE route being selected.

> Before adding a new uAPI I want to
> understand the source of the difference and see if we can improve / fix
> the current multipath code so that the two behave the same. If we can
> get them to behave the same then I don't think user space will care
> about two default routes versus one default multipath route.

Exactly, I totally support that approach.

Regards,
Matt.

^ permalink raw reply

* Re: [RFC PATCH 0/4] Add fbind() and NUMA mempolicy support for KVM guest_memfd
From: Matthew Wilcox @ 2024-11-07 15:10 UTC (permalink / raw)
  To: Shivank Garg
  Cc: x86, viro, brauner, jack, akpm, linux-kernel, linux-fsdevel,
	linux-mm, linux-api, linux-arch, kvm, chao.gao, pgonda,
	thomas.lendacky, seanjc, luto, tglx, mingo, bp, dave.hansen, arnd,
	pbonzini, kees, bharata, nikunj, michael.day, Neeraj.Upadhyay,
	linux-coco
In-Reply-To: <6004eaa4-934c-48f4-b502-cf7e436462fc@amd.com>

On Thu, Nov 07, 2024 at 02:24:20PM +0530, Shivank Garg wrote:
> The folio allocation path from guest_memfd typically looks like this...
> 
> kvm_gmem_get_folio
>   filemap_grab_folio
>     __filemap_get_folio
>       filemap_alloc_folio
>         __folio_alloc_node_noprof
>           -> goes to the buddy allocator
> 
> Hence, I am trying to have a version of filemap_alloc_folio() that takes an mpol.

It only takes that path if cpuset_do_page_mem_spread() is true.  Is the
real problem that you're trying to solve that cpusets are being used
incorrectly?

Backing up, it seems like you want to make a change to the page cache,
you've had a long discussion with people who aren't the page cache
maintainer, and you all understand the pros and cons of everything,
and here you are dumping a solution on me without talking to me, even
though I was at Plumbers, you didn't find me to tell me I needed to go
to your talk.

So you haven't explained a damned thing to me, and I'm annoyed at you.
Do better.  Starting with your cover letter.

^ permalink raw reply

* Re: [PATCH net 1/1] net/ipv6: Netlink flag for new IPv6 Default Routes
From: Ido Schimmel @ 2024-11-07  9:52 UTC (permalink / raw)
  To: Matt Muggeridge
  Cc: davem, dsahern, edumazet, horms, kuba, linux-api, linux-kernel,
	netdev, pabeni, stable
In-Reply-To: <20241107035303.24057-1-Matt.Muggeridge@hpe.com>

On Wed, Nov 06, 2024 at 10:53:03PM -0500, Matt Muggeridge wrote:
> Hi Ido,
> 
> > >>> Is the problem that fib6_table_lookup() chooses a reachable
> > >>> nexthop and then fib6_select_path() overrides it with an unreachable
> > >>> one?
> > 
> > >> I'm afraid I don't know.
> > >>
> > > We need to understand the current behavior before adding a new interface
> > > that we will never be able to remove. It is possible we can improve /
> > > fix the current code. I won't have time to look into it myself until
> > > next week.
> 
> I am grateful that you want to look into it. Thank you! And I look forward to
> learning what you discover.
> 
> You probably already know how to reproduce it, but in case it helps, I still
> have the packet captures and can share them with you. Let me know if you'd
> like me to share them (and how to share them).

It would be best if you could provide a reproducer using iproute2:
Configure a dummy device using ip-link, install the multipath route
using ip-route, configure the neighbour table using ip-neigh and then
perform route queries using "ip route get ..." showing the problem. We
can then use it as the basis for a new test case in
tools/testing/selftests/net/fib_tests.sh 

BTW, do you have CONFIG_IPV6_ROUTER_PREF=y in your config?

> 
> > > 
> > > The objective is to allow IPv6 Netlink clients to be able to create default
> > > routes from RAs in the same way the kernel creates default routes from RAs.
> > > Essentially, I'm trying to have Netlink and Kernel behaviors match.
> > 
> > I understand, but it's essentially an extension for the legacy IPv6
> > multipath API which we are trying to move away from towards the nexthop
> > API (see more below).
> 
> Very interesting, I wasn't aware of this movement.
> 
> While this change is an extension of the legacy IPv6 multipath API, won't it
> still need to support Netlink clients that have been designed around it? I
> imagine that transitioning Netlink clients to the NH API will take many years?

FRR already supports it and I saw that there is some support for nexthop
objects in systemd:

https://github.com/systemd/systemd/pull/13735

> 
> As such, it still seems appropriate (to me) that this be implemented in the
> legacy API as well as ensuring it works with the NH API.

As I understand it you currently get different results because the
kernel installs two default routes whereas user space can only create
one default multipath route. Before adding a new uAPI I want to
understand the source of the difference and see if we can improve / fix
the current multipath code so that the two behave the same. If we can
get them to behave the same then I don't think user space will care
about two default routes versus one default multipath route.

> 
> Another consideration...
> 
> Will the kernel RA processing go through the same nh pathway? The reason I
> ask is because I faced several challenges with IPv6 Logo certification due to
> Netlink clients being unable to achieve the same as the kernel's behavior.

If you are asking if the kernel can install RA routes using nexthop
objects, then the answer is no. Only user space can create nexthop
objects and I don't think we want to allow the kernel to do that.

> 
> As long as the kernel is creating RA routes in a way that meets RFC4861, then
> I'd hope that  Netlink clients would be able to leverage that for 'free'.
> 
> > > 
> > > My analysis led me to the need for Netlink clients to set the kernel's
> > > fib6_config flags RTF_RA_ROUTER, where:
> > > 
> > >     #define RTF_RA_ROUTER		(RTF_ADDRCONF | RTF_DEFAULT)
> > > 
> > >>> +	if (rtm->rtm_flags & RTM_F_RA_ROUTER)
> > >>> +		cfg->fc_flags |= RTF_RA_ROUTER;
> > >>> +
> > >> 
> > >> It is possible there are user space programs out there that set this bit
> > >> (knowingly or not) when sending requests to the kernel and this change
> > >> will result in a behavior change for them. So, if we were to continue in
> > >> this path, this would need to be converted to a new netlink attribute to
> > >> avoid such potential problems.
> > >> 
> > > 
> > > Is this a mandated approach to implementing unspecified bits in a flag?
> > > 
> > > I'm a little surprised by this consideration. If we account for poorly
> > > written buggy user-programs, doesn't this open any API to an explosion
> > > of new attributes or other odd extensions? I'd imagine the same argument
> > > would be applicable to ioctl flags, socket flags, and so on. Why would we
> > > treat implementing unspecified Netlink bits differently to implementing
> > > unspecified ioctl bits, etc.
> > > 
> > > Naturally, if this is the mandated approach, then I'll reimplement it with
> > > a new Netlink attribute. I'm just trying to understand what is the
> > > Linux-lore, here?
> > 
> > Using this bit could have been valid if previously the kernel rejected
> > requests with this bit set, but as evident by your patch the kernel does
> > not do it. It is therefore possible that there are user space programs
> > out there that are working perfectly fine right now and they will break
> > / misbehave after this change.
> > 
> 
> Understood and I agree.
> 
> > > 
> > >> BTW, you can avoid the coalescing problem by using the nexthop API (man
> > >> ip-nexthop).
> > > 
> > > I'm not sure how that would help in this case. We need the nexthop to be
> > > determined according to its REACHABILITY and other considerations described
> > > in RFC4861.
> > 
> > Using your example:
> > 
> > # ip nexthop add id 1 via fe80::200:10ff:fe10:1060 dev enp0s9
> > # ip -6 route add default nhid 1 expires 600 proto ra
> > # ip nexthop add id 2 via fe80::200:10ff:fe10:1061 dev enp0s9
> > # ip -6 route append default nhid 2 expires 600 proto ra
> > # ip -6 route
> > fe80::/64 dev enp0s9 proto kernel metric 256 pref medium
> > default nhid 1 via fe80::200:10ff:fe10:1060 dev enp0s9 proto ra metric 1024 expires 563sec pref medium
> > default nhid 2 via fe80::200:10ff:fe10:1061 dev enp0s9 proto ra metric 1024 expires 594sec pref medium
> 
> Thanks! That looks like it should work. I'll raise this with the the developers
> of systemd-networkd.
> 
> Just to confirm; are these two nhid routes equivalent to having two separate
> default routes that are created when the kernel processes IPv6 RAs?
> 
> Specifically, if one of these nhid routes becomes UNREACHABLE, will that be
> taken into consideration during the routing decision? (I'm guessing so?)

I didn't test it, but I don't see a reason for these two routes to
behave differently than two default routes installed with legacy
nexthops.

^ permalink raw reply

* Re: [RFC PATCH 0/4] Add fbind() and NUMA mempolicy support for KVM guest_memfd
From: Shivank Garg @ 2024-11-07  8:54 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: x86, viro, brauner, jack, akpm, linux-kernel, linux-fsdevel,
	linux-mm, linux-api, linux-arch, kvm, chao.gao, pgonda,
	thomas.lendacky, seanjc, luto, tglx, mingo, bp, dave.hansen, arnd,
	pbonzini, kees, bharata, nikunj, michael.day, Neeraj.Upadhyay,
	linux-coco
In-Reply-To: <ZypqJ0e-J3C_K8LA@casper.infradead.org>

Hi Matthew,

On 11/6/2024 12:25 AM, Matthew Wilcox wrote:
> On Tue, Nov 05, 2024 at 04:45:45PM +0000, Shivank Garg wrote:
>> This patch series introduces fbind() syscall to support NUMA memory
>> policies for KVM guest_memfd, allowing VMMs to configure memory placement
>> for guest memory. This addresses the current limitation where guest_memfd
>> allocations ignore NUMA policies, potentially impacting performance of
>> memory-locality-sensitive workloads.
> 
> Why does guest_memfd ignore numa policies?  The pagecache doesn't,
> eg in vma_alloc_folio_noprof().
guest_memfd doesn't have VMAs and hence can't store policy information in
VMA and use vma_alloc_folio_noprof() that fetches mpol from VMA.

The folio allocation path from guest_memfd typically looks like this...

kvm_gmem_get_folio
  filemap_grab_folio
    __filemap_get_folio
      filemap_alloc_folio
        __folio_alloc_node_noprof
          -> goes to the buddy allocator

Hence, I am trying to have a version of filemap_alloc_folio() that takes an mpol.


Thanks,
Shivank

^ permalink raw reply

* Re: [PATCH net 1/1] net/ipv6: Netlink flag for new IPv6 Default Routes
From: Matt Muggeridge @ 2024-11-07  3:53 UTC (permalink / raw)
  To: idosch
  Cc: Matt.Muggeridge, davem, dsahern, edumazet, horms, kuba, linux-api,
	linux-kernel, netdev, pabeni, stable
In-Reply-To: <ZytjEINNRmtpadr_@shredder>

Hi Ido,

> >>> Is the problem that fib6_table_lookup() chooses a reachable
> >>> nexthop and then fib6_select_path() overrides it with an unreachable
> >>> one?
> 
> >> I'm afraid I don't know.
> >>
> > We need to understand the current behavior before adding a new interface
> > that we will never be able to remove. It is possible we can improve /
> > fix the current code. I won't have time to look into it myself until
> > next week.

I am grateful that you want to look into it. Thank you! And I look forward to
learning what you discover.

You probably already know how to reproduce it, but in case it helps, I still
have the packet captures and can share them with you. Let me know if you'd
like me to share them (and how to share them).

> > 
> > The objective is to allow IPv6 Netlink clients to be able to create default
> > routes from RAs in the same way the kernel creates default routes from RAs.
> > Essentially, I'm trying to have Netlink and Kernel behaviors match.
> 
> I understand, but it's essentially an extension for the legacy IPv6
> multipath API which we are trying to move away from towards the nexthop
> API (see more below).

Very interesting, I wasn't aware of this movement.

While this change is an extension of the legacy IPv6 multipath API, won't it
still need to support Netlink clients that have been designed around it? I
imagine that transitioning Netlink clients to the NH API will take many years?

As such, it still seems appropriate (to me) that this be implemented in the
legacy API as well as ensuring it works with the NH API.

Another consideration...

Will the kernel RA processing go through the same nh pathway? The reason I
ask is because I faced several challenges with IPv6 Logo certification due to
Netlink clients being unable to achieve the same as the kernel's behavior.

As long as the kernel is creating RA routes in a way that meets RFC4861, then
I'd hope that  Netlink clients would be able to leverage that for 'free'.

> > 
> > My analysis led me to the need for Netlink clients to set the kernel's
> > fib6_config flags RTF_RA_ROUTER, where:
> > 
> >     #define RTF_RA_ROUTER		(RTF_ADDRCONF | RTF_DEFAULT)
> > 
> >>> +	if (rtm->rtm_flags & RTM_F_RA_ROUTER)
> >>> +		cfg->fc_flags |= RTF_RA_ROUTER;
> >>> +
> >> 
> >> It is possible there are user space programs out there that set this bit
> >> (knowingly or not) when sending requests to the kernel and this change
> >> will result in a behavior change for them. So, if we were to continue in
> >> this path, this would need to be converted to a new netlink attribute to
> >> avoid such potential problems.
> >> 
> > 
> > Is this a mandated approach to implementing unspecified bits in a flag?
> > 
> > I'm a little surprised by this consideration. If we account for poorly
> > written buggy user-programs, doesn't this open any API to an explosion
> > of new attributes or other odd extensions? I'd imagine the same argument
> > would be applicable to ioctl flags, socket flags, and so on. Why would we
> > treat implementing unspecified Netlink bits differently to implementing
> > unspecified ioctl bits, etc.
> > 
> > Naturally, if this is the mandated approach, then I'll reimplement it with
> > a new Netlink attribute. I'm just trying to understand what is the
> > Linux-lore, here?
> 
> Using this bit could have been valid if previously the kernel rejected
> requests with this bit set, but as evident by your patch the kernel does
> not do it. It is therefore possible that there are user space programs
> out there that are working perfectly fine right now and they will break
> / misbehave after this change.
> 

Understood and I agree.

> > 
> >> BTW, you can avoid the coalescing problem by using the nexthop API (man
> >> ip-nexthop).
> > 
> > I'm not sure how that would help in this case. We need the nexthop to be
> > determined according to its REACHABILITY and other considerations described
> > in RFC4861.
> 
> Using your example:
> 
> # ip nexthop add id 1 via fe80::200:10ff:fe10:1060 dev enp0s9
> # ip -6 route add default nhid 1 expires 600 proto ra
> # ip nexthop add id 2 via fe80::200:10ff:fe10:1061 dev enp0s9
> # ip -6 route append default nhid 2 expires 600 proto ra
> # ip -6 route
> fe80::/64 dev enp0s9 proto kernel metric 256 pref medium
> default nhid 1 via fe80::200:10ff:fe10:1060 dev enp0s9 proto ra metric 1024 expires 563sec pref medium
> default nhid 2 via fe80::200:10ff:fe10:1061 dev enp0s9 proto ra metric 1024 expires 594sec pref medium

Thanks! That looks like it should work. I'll raise this with the the developers
of systemd-networkd.

Just to confirm; are these two nhid routes equivalent to having two separate
default routes that are created when the kernel processes IPv6 RAs?

Specifically, if one of these nhid routes becomes UNREACHABLE, will that be
taken into consideration during the routing decision? (I'm guessing so?)

Thank you for your interest in this.

Regards,
Matt.

^ permalink raw reply

* Re: [PATCH net 1/1] net/ipv6: Netlink flag for new IPv6 Default Routes
From: David Ahern @ 2024-11-06 18:59 UTC (permalink / raw)
  To: Matt Muggeridge, idosch
  Cc: davem, edumazet, horms, kuba, linux-api, linux-kernel, netdev,
	pabeni, stable
In-Reply-To: <20241106025056.11241-1-Matt.Muggeridge@hpe.com>

On 11/5/24 7:50 PM, Matt Muggeridge wrote:
> I'm a little surprised by this consideration. If we account for poorly
> written buggy user-programs, doesn't this open any API to an explosion
> of new attributes or other odd extensions? I'd imagine the same argument

yes, it does and there are many examples. UAPIs are hard to get right
yet persistent forever, so I do agree with Ido's push back on making
sure it is needed.


^ permalink raw reply

* Re: [PATCH net 1/1] net/ipv6: Netlink flag for new IPv6 Default Routes
From: Ido Schimmel @ 2024-11-06 12:37 UTC (permalink / raw)
  To: Matt Muggeridge
  Cc: davem, dsahern, edumazet, horms, kuba, linux-api, linux-kernel,
	netdev, pabeni, stable
In-Reply-To: <20241106025056.11241-1-Matt.Muggeridge@hpe.com>

On Tue, Nov 05, 2024 at 09:50:56PM -0500, Matt Muggeridge wrote:
> Thank you for your review and feedback, Ido.
> 
> >> Without this flag, when there are mutliple default routers, the kernel
> >> coalesces multiple default routes into an ECMP route. The ECMP route
> >> ignores per-route REACHABILITY information. If one of the default
> >> routers is unresponsive, with a Neighbor Cache entry of INCOMPLETE, then
> >> it can still be selected as the nexthop for outgoing packets. This
> >> results in an inability to communicate with remote hosts, even though
> >> one of the default routers remains REACHABLE. This violates RFC4861
> >> section 6.3.6, bullet 1.
> >
> >Do you have forwarding disabled (it causes RT6_LOOKUP_F_REACHABLE to be
> >set)?
> 
> Yes, forwarding is disabled on our embedded system. Though, this needs to
> work on systems regardless of the state of forwarding.
> 
> >  Is the problem that fib6_table_lookup() chooses a reachable
> >nexthop and then fib6_select_path() overrides it with an unreachable
> >one?
> 
> I'm afraid I don't know.

We need to understand the current behavior before adding a new interface
that we will never be able to remove. It is possible we can improve /
fix the current code. I won't have time to look into it myself until
next week.

> 
> The objective is to allow IPv6 Netlink clients to be able to create default
> routes from RAs in the same way the kernel creates default routes from RAs.
> Essentially, I'm trying to have Netlink and Kernel behaviors match.

I understand, but it's essentially an extension for the legacy IPv6
multipath API which we are trying to move away from towards the nexthop
API (see more below).

> 
> My analysis led me to the need for Netlink clients to set the kernel's
> fib6_config flags RTF_RA_ROUTER, where:
> 
>     #define RTF_RA_ROUTER		(RTF_ADDRCONF | RTF_DEFAULT)
> 
> >> +	if (rtm->rtm_flags & RTM_F_RA_ROUTER)
> >> +		cfg->fc_flags |= RTF_RA_ROUTER;
> >> +
> > 
> > It is possible there are user space programs out there that set this bit
> > (knowingly or not) when sending requests to the kernel and this change
> > will result in a behavior change for them. So, if we were to continue in
> > this path, this would need to be converted to a new netlink attribute to
> > avoid such potential problems.
> > 
> 
> Is this a mandated approach to implementing unspecified bits in a flag?
> 
> I'm a little surprised by this consideration. If we account for poorly
> written buggy user-programs, doesn't this open any API to an explosion
> of new attributes or other odd extensions? I'd imagine the same argument
> would be applicable to ioctl flags, socket flags, and so on. Why would we
> treat implementing unspecified Netlink bits differently to implementing
> unspecified ioctl bits, etc.
> 
> Naturally, if this is the mandated approach, then I'll reimplement it with
> a new Netlink attribute. I'm just trying to understand what is the
> Linux-lore, here?

Using this bit could have been valid if previously the kernel rejected
requests with this bit set, but as evident by your patch the kernel does
not do it. It is therefore possible that there are user space programs
out there that are working perfectly fine right now and they will break
/ misbehave after this change.

> 
> > BTW, you can avoid the coalescing problem by using the nexthop API (man
> > ip-nexthop).
> 
> I'm not sure how that would help in this case. We need the nexthop to be
> determined according to its REACHABILITY and other considerations described
> in RFC4861.

Using your example:

# ip nexthop add id 1 via fe80::200:10ff:fe10:1060 dev enp0s9
# ip -6 route add default nhid 1 expires 600 proto ra
# ip nexthop add id 2 via fe80::200:10ff:fe10:1061 dev enp0s9
# ip -6 route append default nhid 2 expires 600 proto ra
# ip -6 route
fe80::/64 dev enp0s9 proto kernel metric 256 pref medium
default nhid 1 via fe80::200:10ff:fe10:1060 dev enp0s9 proto ra metric 1024 expires 563sec pref medium
default nhid 2 via fe80::200:10ff:fe10:1061 dev enp0s9 proto ra metric 1024 expires 594sec pref medium

^ permalink raw reply

* Re: [PATCH net 1/1] net/ipv6: Netlink flag for new IPv6 Default Routes
From: Matt Muggeridge @ 2024-11-06  2:50 UTC (permalink / raw)
  To: idosch
  Cc: Matt.Muggeridge, davem, dsahern, edumazet, horms, kuba, linux-api,
	linux-kernel, netdev, pabeni, stable
In-Reply-To: <Zypgu5l7F1FpIpqo@shredder>

Thank you for your review and feedback, Ido.

>> Without this flag, when there are mutliple default routers, the kernel
>> coalesces multiple default routes into an ECMP route. The ECMP route
>> ignores per-route REACHABILITY information. If one of the default
>> routers is unresponsive, with a Neighbor Cache entry of INCOMPLETE, then
>> it can still be selected as the nexthop for outgoing packets. This
>> results in an inability to communicate with remote hosts, even though
>> one of the default routers remains REACHABLE. This violates RFC4861
>> section 6.3.6, bullet 1.
>
>Do you have forwarding disabled (it causes RT6_LOOKUP_F_REACHABLE to be
>set)?

Yes, forwarding is disabled on our embedded system. Though, this needs to
work on systems regardless of the state of forwarding.

>  Is the problem that fib6_table_lookup() chooses a reachable
>nexthop and then fib6_select_path() overrides it with an unreachable
>one?

I'm afraid I don't know.

The objective is to allow IPv6 Netlink clients to be able to create default
routes from RAs in the same way the kernel creates default routes from RAs.
Essentially, I'm trying to have Netlink and Kernel behaviors match.

My analysis led me to the need for Netlink clients to set the kernel's
fib6_config flags RTF_RA_ROUTER, where:

    #define RTF_RA_ROUTER		(RTF_ADDRCONF | RTF_DEFAULT)

>> +	if (rtm->rtm_flags & RTM_F_RA_ROUTER)
>> +		cfg->fc_flags |= RTF_RA_ROUTER;
>> +
> 
> It is possible there are user space programs out there that set this bit
> (knowingly or not) when sending requests to the kernel and this change
> will result in a behavior change for them. So, if we were to continue in
> this path, this would need to be converted to a new netlink attribute to
> avoid such potential problems.
> 

Is this a mandated approach to implementing unspecified bits in a flag?

I'm a little surprised by this consideration. If we account for poorly
written buggy user-programs, doesn't this open any API to an explosion
of new attributes or other odd extensions? I'd imagine the same argument
would be applicable to ioctl flags, socket flags, and so on. Why would we
treat implementing unspecified Netlink bits differently to implementing
unspecified ioctl bits, etc.

Naturally, if this is the mandated approach, then I'll reimplement it with
a new Netlink attribute. I'm just trying to understand what is the
Linux-lore, here?

> BTW, you can avoid the coalescing problem by using the nexthop API (man
> ip-nexthop).

I'm not sure how that would help in this case. We need the nexthop to be
determined according to its REACHABILITY and other considerations described
in RFC4861.

Kind regards,
Matt.

^ permalink raw reply

* Re: [PATCH net 1/1] net/ipv6: Netlink flag for new IPv6 Default Routes
From: Matt Muggeridge @ 2024-11-06  1:44 UTC (permalink / raw)
  To: nicolas.dichtel
  Cc: Matt.Muggeridge, davem, dsahern, edumazet, horms, kuba, linux-api,
	linux-kernel, netdev, pabeni, stable
In-Reply-To: <0a8d6565-fdc0-452f-b132-5d237a1b7dec@6wind.com>

> Please, don't mix whitespace changes with the changes related to the new flag.

Thanks, Nicolas. I will revert the changes that tidied up trailing whitespace in the next version.

^ permalink raw reply

* Re: [RFC PATCH 0/4] Add fbind() and NUMA mempolicy support for KVM guest_memfd
From: Matthew Wilcox @ 2024-11-05 18:55 UTC (permalink / raw)
  To: Shivank Garg
  Cc: x86, viro, brauner, jack, akpm, linux-kernel, linux-fsdevel,
	linux-mm, linux-api, linux-arch, kvm, chao.gao, pgonda,
	thomas.lendacky, seanjc, luto, tglx, mingo, bp, dave.hansen, arnd,
	pbonzini, kees, bharata, nikunj, michael.day, Neeraj.Upadhyay
In-Reply-To: <20241105164549.154700-1-shivankg@amd.com>

On Tue, Nov 05, 2024 at 04:45:45PM +0000, Shivank Garg wrote:
> This patch series introduces fbind() syscall to support NUMA memory
> policies for KVM guest_memfd, allowing VMMs to configure memory placement
> for guest memory. This addresses the current limitation where guest_memfd
> allocations ignore NUMA policies, potentially impacting performance of
> memory-locality-sensitive workloads.

Why does guest_memfd ignore numa policies?  The pagecache doesn't,
eg in vma_alloc_folio_noprof().

^ permalink raw reply

* Re: [PATCH net 1/1] net/ipv6: Netlink flag for new IPv6 Default Routes
From: Ido Schimmel @ 2024-11-05 18:15 UTC (permalink / raw)
  To: Matt Muggeridge
  Cc: David Ahern, David S . Miller, linux-api, stable, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Simon Horman, netdev, linux-kernel
In-Reply-To: <20241105031841.10730-2-Matt.Muggeridge@hpe.com>

On Mon, Nov 04, 2024 at 10:18:39PM -0500, Matt Muggeridge wrote:
> Add a Netlink rtm_flag, RTM_F_RA_ROUTER for the RTM_NEWROUTE message.
> This allows an IPv6 Netlink client to indicate the default route came
> from an RA. This results in the kernel creating individual default
> routes, rather than coalescing multiple default routes into a single
> ECMP route.
> 
> Details:
> 
> For IPv6, a Netlink client is unable to create default routes in the
> same manner as the kernel. This leads to failures when there are
> multiple default routers, as they were being coalesced into a single
> ECMP route. When one of the ECMP default routers becomes UNREACHABLE, it
> was still being selected as the nexthop.
> 
> Meanwhile, when the kernel processes RAs from multiple default routers,
> it sets the fib6_flags: RTF_ADDRCONF | RTF_DEFAULT. The RTF_ADDRCONF
> flag is checked by rt6_qualify_for_ecmp(), which returns false when
> ADDRCONF is set. As such, the kernel creates separate default routes.
> 
> E.g. compare the routing tables when RAs are processed by the kernel
> versus a Netlink client (systemd-networkd, in my case).
> 
> 1) RA Processed by kernel (accept_ra = 2)
> $ ip -6 route
> 2001:2:0:1000::/64 dev enp0s9 proto kernel metric 256 expires ...
> fe80::/64 dev enp0s9 proto kernel metric 256 pref medium
> default via fe80::200:10ff:fe10:1060 dev enp0s9 proto ra ...
> default via fe80::200:10ff:fe10:1061 dev enp0s9 proto ra ...
> 
> 2) RA Processed by Netlink client (accept_ra = 0)
> $ ip -6 route
> 2001:2:0:1000::/64 dev enp0s9 proto ra metric 1024 expires ...
> fe80::/64 dev enp0s3 proto kernel metric 256 pref medium
> fe80::/64 dev enp0s9 proto kernel metric 256 pref medium
> default proto ra metric 1024 expires 595sec pref medium
> 	nexthop via fe80::200:10ff:fe10:1060 dev enp0s9 weight 1
> 	nexthop via fe80::200:10ff:fe10:1061 dev enp0s9 weight 1
> 
> IPv6 Netlink clients need a mechanism to identify a route as coming from
> an RA. i.e. a Netlink client needs a method to set the kernel flags:
> 
>     RTF_ADDRCONF | RTF_DEFAULT
> 
> This is needed when there are multiple default routers that each send
> an RA. Setting the RTF_ADDRCONF flag ensures their fib entries do not
> qualify for ECMP routes, see rt6_qualify_for_ecmp().
> 
> To achieve this, introduce a user-level flag RTM_F_RA_ROUTER that a
> Netlink client can pass to the kernel.
> 
> A Netlink user-level network manager, such as systemd-networkd, may set
> the RTM_F_RA_ROUTER flag in the Netlink RTM_NEWROUTE rtmsg. When set,
> the kernel sets RTF_RA_ROUTER in the fib6_config fc_flags. This causes a
> default route to be created in the same way as if the kernel processed
> the RA, via rt6add_dflt_router().
> 
> This is needed by user-level network managers, like systemd-networkd,
> that prefer to do the RA processing themselves. ie. they disable the
> kernel's RA processing by setting net.ipv6.conf.<intf>.accept_ra=0.
> 
> Without this flag, when there are mutliple default routers, the kernel
> coalesces multiple default routes into an ECMP route. The ECMP route
> ignores per-route REACHABILITY information. If one of the default
> routers is unresponsive, with a Neighbor Cache entry of INCOMPLETE, then
> it can still be selected as the nexthop for outgoing packets. This
> results in an inability to communicate with remote hosts, even though
> one of the default routers remains REACHABLE. This violates RFC4861
> section 6.3.6, bullet 1.

Do you have forwarding disabled (it causes RT6_LOOKUP_F_REACHABLE to be
set)? Is the problem that fib6_table_lookup() chooses a reachable
nexthop and then fib6_select_path() overrides it with an unreachable
one?

> 
> Extract from RFC4861 6.3.6 bullet 1:
>      1) Routers that are reachable or probably reachable (i.e., in any
>         state other than INCOMPLETE) SHOULD be preferred over routers
>         whose reachability is unknown or suspect (i.e., in the
>         INCOMPLETE state, or for which no Neighbor Cache entry exists).
> 
> This fixes the IPv6 Logo conformance test v6LC_2_2_11, and others that
> test with multiple default routers. Also see systemd issue #33470:
> https://github.com/systemd/systemd/issues/33470.
> 
> Signed-off-by: Matt Muggeridge <Matt.Muggeridge@hpe.com>
> Cc: David Ahern <dsahern@kernel.org>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: linux-api@vger.kernel.org
> Cc: stable@vger.kernel.org
> ---
>  include/uapi/linux/rtnetlink.h | 9 +++++----
>  net/ipv6/route.c               | 3 +++
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
> index 3b687d20c9ed..9f0259f6e4ed 100644
> --- a/include/uapi/linux/rtnetlink.h
> +++ b/include/uapi/linux/rtnetlink.h
> @@ -202,7 +202,7 @@ enum {
>  #define RTM_NR_FAMILIES	(RTM_NR_MSGTYPES >> 2)
>  #define RTM_FAM(cmd)	(((cmd) - RTM_BASE) >> 2)
>  
> -/* 
> +/*
>     Generic structure for encapsulation of optional route information.
>     It is reminiscent of sockaddr, but with sa_family replaced
>     with attribute type.
> @@ -242,7 +242,7 @@ struct rtmsg {
>  
>  	unsigned char		rtm_table;	/* Routing table id */
>  	unsigned char		rtm_protocol;	/* Routing protocol; see below	*/
> -	unsigned char		rtm_scope;	/* See below */	
> +	unsigned char		rtm_scope;	/* See below */
>  	unsigned char		rtm_type;	/* See below	*/
>  
>  	unsigned		rtm_flags;
> @@ -336,6 +336,7 @@ enum rt_scope_t {
>  #define RTM_F_FIB_MATCH	        0x2000	/* return full fib lookup match */
>  #define RTM_F_OFFLOAD		0x4000	/* route is offloaded */
>  #define RTM_F_TRAP		0x8000	/* route is trapping packets */
> +#define RTM_F_RA_ROUTER		0x10000	/* route is a default route from RA */
>  #define RTM_F_OFFLOAD_FAILED	0x20000000 /* route offload failed, this value
>  					    * is chosen to avoid conflicts with
>  					    * other flags defined in
> @@ -568,7 +569,7 @@ struct ifinfomsg {
>  };
>  
>  /********************************************************************
> - *		prefix information 
> + *		prefix information
>   ****/
>  
>  struct prefixmsg {
> @@ -582,7 +583,7 @@ struct prefixmsg {
>  	unsigned char	prefix_pad3;
>  };
>  
> -enum 
> +enum
>  {
>  	PREFIX_UNSPEC,
>  	PREFIX_ADDRESS,
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index b4251915585f..5b0c16422720 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -5055,6 +5055,9 @@ static int rtm_to_fib6_config(struct sk_buff *skb, struct nlmsghdr *nlh,
>  	if (rtm->rtm_flags & RTM_F_CLONED)
>  		cfg->fc_flags |= RTF_CACHE;
>  
> +	if (rtm->rtm_flags & RTM_F_RA_ROUTER)
> +		cfg->fc_flags |= RTF_RA_ROUTER;
> +

It is possible there are user space programs out there that set this bit
(knowingly or not) when sending requests to the kernel and this change
will result in a behavior change for them. So, if we were to continue in
this path, this would need to be converted to a new netlink attribute to
avoid such potential problems.

BTW, you can avoid the coalescing problem by using the nexthop API (man
ip-nexthop).

>  	cfg->fc_flags |= (rtm->rtm_flags & RTNH_F_ONLINK);
>  
>  	if (tb[RTA_NH_ID]) {
> -- 
> 2.35.3
> 
> 

^ permalink raw reply

* [RFC PATCH 4/4] KVM: guest_memfd: Enforce NUMA mempolicy if available
From: Shivank Garg @ 2024-11-05 16:55 UTC (permalink / raw)
  To: x86, viro, brauner, jack, akpm, linux-kernel, linux-fsdevel,
	linux-mm, linux-api, linux-arch, kvm
  Cc: chao.gao, pgonda, thomas.lendacky, seanjc, luto, tglx, mingo, bp,
	dave.hansen, willy, arnd, pbonzini, kees, shivankg, bharata,
	nikunj, michael.day, Neeraj.Upadhyay
In-Reply-To: <20241105165515.154941-1-shivankg@amd.com>

Enforce memory policy on guest-memfd to provide proper NUMA support.
Previously, guest-memfd allocations were following local NUMA node id
in absence of process mempolicy, resulting in random memory allocation.
Moreover, it cannot use mbind() since memory isn't mapped to userspace.

To support NUMA policies, call fbind() syscall from VMM to store
mempolicy as f_policy in struct kvm_gmem of guest_memfd. The f_policy
is retrieved to pass in filemap_grab_folio_mpol() to ensure that
allocations follow the specified memory policy.

Signed-off-by: Shivank Garg <shivankg@amd.com>
---
 mm/mempolicy.c         |  2 ++
 virt/kvm/guest_memfd.c | 49 ++++++++++++++++++++++++++++++++++++++----
 2 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 3a697080ecad..af2e1ef4dae7 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -347,6 +347,7 @@ void __mpol_put(struct mempolicy *pol)
 		return;
 	kmem_cache_free(policy_cache, pol);
 }
+EXPORT_SYMBOL(__mpol_put);
 
 static void mpol_rebind_default(struct mempolicy *pol, const nodemask_t *nodes)
 {
@@ -2599,6 +2600,7 @@ struct mempolicy *__mpol_dup(struct mempolicy *old)
 	atomic_set(&new->refcnt, 1);
 	return new;
 }
+EXPORT_SYMBOL(__mpol_dup);
 
 /* Slow path of a mempolicy comparison */
 bool __mpol_equal(struct mempolicy *a, struct mempolicy *b)
diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
index 2c6fcf7c3ec9..0237bda4382c 100644
--- a/virt/kvm/guest_memfd.c
+++ b/virt/kvm/guest_memfd.c
@@ -4,6 +4,7 @@
 #include <linux/kvm_host.h>
 #include <linux/pagemap.h>
 #include <linux/anon_inodes.h>
+#include <linux/mempolicy.h>
 
 #include "kvm_mm.h"
 
@@ -11,6 +12,7 @@ struct kvm_gmem {
 	struct kvm *kvm;
 	struct xarray bindings;
 	struct list_head entry;
+	struct mempolicy *f_policy;
 };
 
 /**
@@ -87,7 +89,8 @@ static int kvm_gmem_prepare_folio(struct kvm *kvm, struct kvm_memory_slot *slot,
 }
 
 static struct folio *kvm_gmem_get_huge_folio(struct inode *inode, pgoff_t index,
-					     unsigned int order)
+					     unsigned int order,
+					     struct mempolicy *policy)
 {
 	pgoff_t npages = 1UL << order;
 	pgoff_t huge_index = round_down(index, npages);
@@ -104,7 +107,7 @@ static struct folio *kvm_gmem_get_huge_folio(struct inode *inode, pgoff_t index,
 				   (loff_t)(huge_index + npages - 1) << PAGE_SHIFT))
 		return NULL;
 
-	folio = filemap_alloc_folio(gfp, order);
+	folio = filemap_alloc_folio_mpol(gfp, order, policy);
 	if (!folio)
 		return NULL;
 
@@ -129,12 +132,26 @@ static struct folio *__kvm_gmem_get_folio(struct file *file, pgoff_t index,
 					  bool allow_huge)
 {
 	struct folio *folio = NULL;
+	struct kvm_gmem *gmem = file->private_data;
+	struct mempolicy *policy = NULL;
+
+	/*
+	 * RCU lock is required to prevent any race condition with set_policy().
+	 */
+	if (IS_ENABLED(CONFIG_NUMA)) {
+		rcu_read_lock();
+		policy = READ_ONCE(gmem->f_policy);
+		mpol_get(policy);
+		rcu_read_unlock();
+	}
 
 	if (gmem_2m_enabled && allow_huge)
-		folio = kvm_gmem_get_huge_folio(file_inode(file), index, PMD_ORDER);
+		folio = kvm_gmem_get_huge_folio(file_inode(file), index, PMD_ORDER, policy);
 
 	if (!folio)
-		folio = filemap_grab_folio(file_inode(file)->i_mapping, index);
+		folio = filemap_grab_folio_mpol(file_inode(file)->i_mapping, index, policy);
+
+	mpol_put(policy);
 
 	pr_debug("%s: allocate folio with PFN %lx order %d\n",
 		 __func__, folio_pfn(folio), folio_order(folio));
@@ -338,6 +355,7 @@ static int kvm_gmem_release(struct inode *inode, struct file *file)
 	mutex_unlock(&kvm->slots_lock);
 
 	xa_destroy(&gmem->bindings);
+	mpol_put(gmem->f_policy);
 	kfree(gmem);
 
 	kvm_put_kvm(kvm);
@@ -356,10 +374,32 @@ static inline struct file *kvm_gmem_get_file(struct kvm_memory_slot *slot)
 	return get_file_active(&slot->gmem.file);
 }
 
+#ifdef CONFIG_NUMA
+static int kvm_gmem_set_policy(struct file *file, struct mempolicy *mpol)
+{
+	struct mempolicy *old, *new;
+	struct kvm_gmem *gmem = file->private_data;
+
+	new = mpol_dup(mpol);
+	if (IS_ERR(new))
+		return PTR_ERR(new);
+
+	old = gmem->f_policy;
+	WRITE_ONCE(gmem->f_policy, new);
+	synchronize_rcu();
+	mpol_put(old);
+
+	return 0;
+}
+#endif
+
 static struct file_operations kvm_gmem_fops = {
 	.open		= generic_file_open,
 	.release	= kvm_gmem_release,
 	.fallocate	= kvm_gmem_fallocate,
+#ifdef CONFIG_NUMA
+	.set_policy	= kvm_gmem_set_policy,
+#endif
 };
 
 void kvm_gmem_init(struct module *module)
@@ -489,6 +529,7 @@ static int __kvm_gmem_create(struct kvm *kvm, loff_t size, u64 flags)
 
 	kvm_get_kvm(kvm);
 	gmem->kvm = kvm;
+	gmem->f_policy = NULL;
 	xa_init(&gmem->bindings);
 	list_add(&gmem->entry, &inode->i_mapping->i_private_list);
 
-- 
2.34.1


^ permalink raw reply related

* [RFC PATCH 3/4] KVM: guest_memfd: Pass file pointer instead of inode in guest_memfd APIs
From: Shivank Garg @ 2024-11-05 16:55 UTC (permalink / raw)
  To: x86, viro, brauner, jack, akpm, linux-kernel, linux-fsdevel,
	linux-mm, linux-api, linux-arch, kvm
  Cc: chao.gao, pgonda, thomas.lendacky, seanjc, luto, tglx, mingo, bp,
	dave.hansen, willy, arnd, pbonzini, kees, shivankg, bharata,
	nikunj, michael.day, Neeraj.Upadhyay
In-Reply-To: <20241105165515.154941-1-shivankg@amd.com>

Change the KVM guest_memfd APIs to pass file pointers instead of
inodes in the folio allocation functions. This is preparatory patch
for adding NUMA support to guest memory allocations.
The functional behavior remains unchanged.

Signed-off-by: Shivank Garg <shivankg@amd.com>
---
 virt/kvm/guest_memfd.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
index e930014b4bdc..2c6fcf7c3ec9 100644
--- a/virt/kvm/guest_memfd.c
+++ b/virt/kvm/guest_memfd.c
@@ -91,7 +91,7 @@ static struct folio *kvm_gmem_get_huge_folio(struct inode *inode, pgoff_t index,
 {
 	pgoff_t npages = 1UL << order;
 	pgoff_t huge_index = round_down(index, npages);
-	struct address_space *mapping  = inode->i_mapping;
+	struct address_space *mapping = inode->i_mapping;
 	gfp_t gfp = mapping_gfp_mask(mapping) | __GFP_NOWARN;
 	loff_t size = i_size_read(inode);
 	struct folio *folio;
@@ -125,16 +125,16 @@ static struct folio *kvm_gmem_get_huge_folio(struct inode *inode, pgoff_t index,
  * Ignore accessed, referenced, and dirty flags.  The memory is
  * unevictable and there is no storage to write back to.
  */
-static struct folio *__kvm_gmem_get_folio(struct inode *inode, pgoff_t index,
+static struct folio *__kvm_gmem_get_folio(struct file *file, pgoff_t index,
 					  bool allow_huge)
 {
 	struct folio *folio = NULL;
 
 	if (gmem_2m_enabled && allow_huge)
-		folio = kvm_gmem_get_huge_folio(inode, index, PMD_ORDER);
+		folio = kvm_gmem_get_huge_folio(file_inode(file), index, PMD_ORDER);
 
 	if (!folio)
-		folio = filemap_grab_folio(inode->i_mapping, index);
+		folio = filemap_grab_folio(file_inode(file)->i_mapping, index);
 
 	pr_debug("%s: allocate folio with PFN %lx order %d\n",
 		 __func__, folio_pfn(folio), folio_order(folio));
@@ -150,9 +150,9 @@ static struct folio *__kvm_gmem_get_folio(struct inode *inode, pgoff_t index,
  * Ignore accessed, referenced, and dirty flags.  The memory is
  * unevictable and there is no storage to write back to.
  */
-static struct folio *kvm_gmem_get_folio(struct inode *inode, pgoff_t index)
+static struct folio *kvm_gmem_get_folio(struct file *file, pgoff_t index)
 {
-	return __kvm_gmem_get_folio(inode, index, true);
+	return __kvm_gmem_get_folio(file, index, true);
 }
 
 static void kvm_gmem_invalidate_begin(struct kvm_gmem *gmem, pgoff_t start,
@@ -228,8 +228,9 @@ static long kvm_gmem_punch_hole(struct inode *inode, loff_t offset, loff_t len)
 	return 0;
 }
 
-static long kvm_gmem_allocate(struct inode *inode, loff_t offset, loff_t len)
+static long kvm_gmem_allocate(struct file *file, loff_t offset, loff_t len)
 {
+	struct inode *inode = file_inode(file);
 	struct address_space *mapping = inode->i_mapping;
 	pgoff_t start, index, end;
 	int r;
@@ -252,7 +253,7 @@ static long kvm_gmem_allocate(struct inode *inode, loff_t offset, loff_t len)
 			break;
 		}
 
-		folio = kvm_gmem_get_folio(inode, index);
+		folio = kvm_gmem_get_folio(file, index);
 		if (IS_ERR(folio)) {
 			r = PTR_ERR(folio);
 			break;
@@ -292,7 +293,7 @@ static long kvm_gmem_fallocate(struct file *file, int mode, loff_t offset,
 	if (mode & FALLOC_FL_PUNCH_HOLE)
 		ret = kvm_gmem_punch_hole(file_inode(file), offset, len);
 	else
-		ret = kvm_gmem_allocate(file_inode(file), offset, len);
+		ret = kvm_gmem_allocate(file, offset, len);
 
 	if (!ret)
 		file_modified(file);
@@ -626,7 +627,7 @@ __kvm_gmem_get_pfn(struct file *file, struct kvm_memory_slot *slot,
 		return ERR_PTR(-EIO);
 	}
 
-	folio = __kvm_gmem_get_folio(file_inode(file), index, allow_huge);
+	folio = __kvm_gmem_get_folio(file, index, allow_huge);
 	if (IS_ERR(folio))
 		return folio;
 
-- 
2.34.1


^ permalink raw reply related

* [RFC PATCH 2/4] Introduce fbind syscall
From: Shivank Garg @ 2024-11-05 16:55 UTC (permalink / raw)
  To: x86, viro, brauner, jack, akpm, linux-kernel, linux-fsdevel,
	linux-mm, linux-api, linux-arch, kvm
  Cc: chao.gao, pgonda, thomas.lendacky, seanjc, luto, tglx, mingo, bp,
	dave.hansen, willy, arnd, pbonzini, kees, shivankg, bharata,
	nikunj, michael.day, Neeraj.Upadhyay, Shivansh Dhiman
In-Reply-To: <20241105164549.154700-1-shivankg@amd.com>

The new fbind syscall sets the NUMA memory policy for file-backed memory
and has following signature:

long fbind(unsigned int fd, unsigned long mode,
                const unsigned long nodemask[(.maxnode + ULONG_WIDTH - 1)
                                               / ULONG_WIDTH],
                unsigned long maxnode, unsigned int flags);

fbind behaves similar to mbind except that it takes file descriptor as
input instead of address ranges.

TODO:
1. Support fbind syscall on all architectures.
2. Expand commit msg and add documentation.
3. clean-up the code.

[Shivansh: add create_mpol_from_args()]
Signed-off-by: Shivansh Dhiman <shivansh.dhiman@amd.com>
Signed-off-by: Shivank Garg <shivankg@amd.com>
---
 arch/x86/entry/syscalls/syscall_32.tbl |  1 +
 arch/x86/entry/syscalls/syscall_64.tbl |  1 +
 include/linux/fs.h                     |  3 ++
 include/linux/mempolicy.h              |  3 ++
 include/linux/syscalls.h               |  3 ++
 include/uapi/asm-generic/unistd.h      |  5 ++-
 kernel/sys_ni.c                        |  1 +
 mm/Makefile                            |  2 +-
 mm/fbind.c                             | 49 +++++++++++++++++++++++
 mm/mempolicy.c                         | 55 ++++++++++++++++++++++++++
 10 files changed, 121 insertions(+), 2 deletions(-)
 create mode 100644 mm/fbind.c

diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
index 534c74b14fab..0660ce6d08d8 100644
--- a/arch/x86/entry/syscalls/syscall_32.tbl
+++ b/arch/x86/entry/syscalls/syscall_32.tbl
@@ -468,3 +468,4 @@
 460	i386	lsm_set_self_attr	sys_lsm_set_self_attr
 461	i386	lsm_list_modules	sys_lsm_list_modules
 462	i386	mseal 			sys_mseal
+463	i386	fbind			sys_fbind
diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
index 7093ee21c0d1..9794347cc2e6 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -386,6 +386,7 @@
 460	common	lsm_set_self_attr	sys_lsm_set_self_attr
 461	common	lsm_list_modules	sys_lsm_list_modules
 462 	common  mseal			sys_mseal
+463	common	fbind			sys_fbind
 
 #
 # Due to a historical design error, certain syscalls are numbered differently
diff --git a/include/linux/fs.h b/include/linux/fs.h
index fd34b5755c0b..42042b62bdcd 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2058,6 +2058,9 @@ struct file_operations {
 				   struct file *file_out, loff_t pos_out,
 				   loff_t len, unsigned int remap_flags);
 	int (*fadvise)(struct file *, loff_t, loff_t, int);
+#ifdef CONFIG_NUMA
+	int (*set_policy)(struct file *, struct mempolicy *);
+#endif
 	int (*uring_cmd)(struct io_uring_cmd *ioucmd, unsigned int issue_flags);
 	int (*uring_cmd_iopoll)(struct io_uring_cmd *, struct io_comp_batch *,
 				unsigned int poll_flags);
diff --git a/include/linux/mempolicy.h b/include/linux/mempolicy.h
index 1add16f21612..b9023f6246a7 100644
--- a/include/linux/mempolicy.h
+++ b/include/linux/mempolicy.h
@@ -299,4 +299,7 @@ static inline bool mpol_is_preferred_many(struct mempolicy *pol)
 }
 
 #endif /* CONFIG_NUMA */
+struct mempolicy *create_mpol_from_args(unsigned char mode,
+					const unsigned long __user *nmask,
+					unsigned short maxnode);
 #endif
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 4bcf6754738d..2dc686921b9f 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -502,6 +502,9 @@ asmlinkage long sys_readlinkat(int dfd, const char __user *path, char __user *bu
 asmlinkage long sys_newfstatat(int dfd, const char __user *filename,
 			       struct stat __user *statbuf, int flag);
 asmlinkage long sys_newfstat(unsigned int fd, struct stat __user *statbuf);
+asmlinkage long sys_fbind(unsigned int fd, unsigned long mode,
+			const unsigned long __user *nmask,
+			unsigned long maxnode, unsigned int flags);
 #if defined(__ARCH_WANT_STAT64) || defined(__ARCH_WANT_COMPAT_STAT64)
 asmlinkage long sys_fstat64(unsigned long fd, struct stat64 __user *statbuf);
 asmlinkage long sys_fstatat64(int dfd, const char __user *filename,
diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
index 5bf6148cac2b..550730f36dae 100644
--- a/include/uapi/asm-generic/unistd.h
+++ b/include/uapi/asm-generic/unistd.h
@@ -841,8 +841,11 @@ __SYSCALL(__NR_lsm_list_modules, sys_lsm_list_modules)
 #define __NR_mseal 462
 __SYSCALL(__NR_mseal, sys_mseal)
 
+#define __NR_fbind 463
+__SYSCALL(__NR_fbind, sys_fbind)
+
 #undef __NR_syscalls
-#define __NR_syscalls 463
+#define __NR_syscalls 464
 
 /*
  * 32 bit systems traditionally used different
diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
index c00a86931f8c..f57350e581f6 100644
--- a/kernel/sys_ni.c
+++ b/kernel/sys_ni.c
@@ -195,6 +195,7 @@ COND_SYSCALL(move_pages);
 COND_SYSCALL(set_mempolicy_home_node);
 COND_SYSCALL(cachestat);
 COND_SYSCALL(mseal);
+COND_SYSCALL(fbind);
 
 COND_SYSCALL(perf_event_open);
 COND_SYSCALL(accept4);
diff --git a/mm/Makefile b/mm/Makefile
index d2915f8c9dc0..ba339ddc0be2 100644
--- a/mm/Makefile
+++ b/mm/Makefile
@@ -79,7 +79,7 @@ obj-$(CONFIG_ZSWAP)	+= zswap.o
 obj-$(CONFIG_HAS_DMA)	+= dmapool.o
 obj-$(CONFIG_HUGETLBFS)	+= hugetlb.o
 obj-$(CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP)	+= hugetlb_vmemmap.o
-obj-$(CONFIG_NUMA) 	+= mempolicy.o
+obj-$(CONFIG_NUMA) 	+= mempolicy.o fbind.o
 obj-$(CONFIG_SPARSEMEM)	+= sparse.o
 obj-$(CONFIG_SPARSEMEM_VMEMMAP) += sparse-vmemmap.o
 obj-$(CONFIG_MMU_NOTIFIER) += mmu_notifier.o
diff --git a/mm/fbind.c b/mm/fbind.c
new file mode 100644
index 000000000000..85ec7d13345c
--- /dev/null
+++ b/mm/fbind.c
@@ -0,0 +1,49 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ *  Implement fbind() syscall.
+ *
+ *  Copyright (c) 2024 AMD
+ *
+ *  Author: Shivank Garg <shivankg@amd.com>
+ */
+
+#include <linux/file.h>
+#include <linux/fs.h>
+#include <linux/mempolicy.h>
+#include <linux/syscalls.h>
+
+static long do_fbind(unsigned int fd, unsigned long mode,
+		const unsigned long __user *nmask,
+		unsigned long maxnode, unsigned int flags)
+{
+	struct mempolicy *mpol;
+	struct fd f;
+	int ret;
+
+	f = fdget(fd);
+	if (!f.file)
+		return -EBADF;
+
+	mpol = create_mpol_from_args(mode, nmask, maxnode);
+	if (IS_ERR_OR_NULL(mpol)) {
+		ret = PTR_ERR(mpol);
+		goto out_putf;
+	}
+
+	if (f.file->f_op->set_policy)
+		ret = f.file->f_op->set_policy(f.file, mpol);
+	else
+		ret = -EOPNOTSUPP;
+
+	mpol_put(mpol);
+out_putf:
+	fdput(f);
+	return ret;
+}
+
+SYSCALL_DEFINE5(fbind, unsigned int, fd, unsigned long, mode,
+		const unsigned long __user *, nmask,
+		unsigned long, maxnode, unsigned int, flags)
+{
+	return do_fbind(fd, mode, nmask, maxnode, flags);
+}
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index b858e22b259d..3a697080ecad 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -3557,3 +3557,58 @@ static int __init mempolicy_sysfs_init(void)
 
 late_initcall(mempolicy_sysfs_init);
 #endif /* CONFIG_SYSFS */
+
+/**
+ * create_mpol_from_args - create a mempolicy structure from args
+ * @mode:  NUMA memory policy mode
+ * @nmask:  bitmask of NUMA nodes
+ * @maxnode:  number of bits in the nodes bitmask
+ *
+ * Create a mempolicy from given nodemask and memory policy such as
+ * default, preferred, interleave or bind.
+ *
+ * Return: error encoded in a pointer or memory policy on success.
+ */
+struct mempolicy *create_mpol_from_args(unsigned char mode,
+					const unsigned long __user *nmask,
+					unsigned short maxnode)
+{
+	struct mm_struct *mm = current->mm;
+	unsigned short mode_flags;
+	struct mempolicy *mpol;
+	nodemask_t nodes;
+	int lmode = mode;
+	int err = -ENOMEM;
+
+	err = sanitize_mpol_flags(&lmode, &mode_flags);
+	if (err)
+		return ERR_PTR(err);
+
+	err = get_nodes(&nodes, nmask, maxnode);
+	if (err)
+		return ERR_PTR(err);
+
+	mpol = mpol_new(mode, mode_flags, &nodes);
+	if (IS_ERR_OR_NULL(mpol))
+		return mpol;
+
+	NODEMASK_SCRATCH(scratch);
+	if (!scratch) {
+		err = -ENOMEM;
+		goto err_out;
+	}
+
+	mmap_write_lock(mm);
+	err = mpol_set_nodemask(mpol, &nodes, scratch);
+	mmap_write_unlock(mm);
+	NODEMASK_SCRATCH_FREE(scratch);
+
+	if (err)
+		goto err_out;
+
+	return mpol;
+
+err_out:
+	mpol_put(mpol);
+	return ERR_PTR(err);
+}
-- 
2.34.1


^ permalink raw reply related

* [RFC PATCH 1/4] mm: Add mempolicy support to the filemap layer
From: Shivank Garg @ 2024-11-05 16:45 UTC (permalink / raw)
  To: x86, viro, brauner, jack, akpm, linux-kernel, linux-fsdevel,
	linux-mm, linux-api, linux-arch, kvm
  Cc: chao.gao, pgonda, thomas.lendacky, seanjc, luto, tglx, mingo, bp,
	dave.hansen, willy, arnd, pbonzini, kees, shivankg, bharata,
	nikunj, michael.day, Neeraj.Upadhyay, Shivansh Dhiman
In-Reply-To: <20241105164549.154700-1-shivankg@amd.com>

From: Shivansh Dhiman <shivansh.dhiman@amd.com>

Introduce mempolicy support to the filemap. Add filemap_grab_folio_mpol,
filemap_alloc_folio_mpol_noprof() and __filemap_get_folio_mpol() APIs that
take mempolicy struct as an argument.

The API is required by VMs using KVM guest-memfd memory backends for NUMA
mempolicy aware allocations.

Signed-off-by: Shivansh Dhiman <shivansh.dhiman@amd.com>
Signed-off-by: Shivank Garg <shivankg@amd.com>
---
 include/linux/pagemap.h | 40 ++++++++++++++++++++++++++++++++++++++++
 mm/filemap.c            | 30 +++++++++++++++++++++++++-----
 2 files changed, 65 insertions(+), 5 deletions(-)

diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index d9c7edb6422b..b05b696f310b 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -564,15 +564,25 @@ static inline void *detach_page_private(struct page *page)
 
 #ifdef CONFIG_NUMA
 struct folio *filemap_alloc_folio_noprof(gfp_t gfp, unsigned int order);
+struct folio *filemap_alloc_folio_mpol_noprof(gfp_t gfp, unsigned int order,
+						struct mempolicy *mpol);
 #else
 static inline struct folio *filemap_alloc_folio_noprof(gfp_t gfp, unsigned int order)
 {
 	return folio_alloc_noprof(gfp, order);
 }
+static inline struct folio *filemap_alloc_folio_mpol_noprof(gfp_t gfp,
+						unsigned int order,
+						struct mempolicy *mpol)
+{
+	return filemap_alloc_folio_noprof(gfp, order);
+}
 #endif
 
 #define filemap_alloc_folio(...)				\
 	alloc_hooks(filemap_alloc_folio_noprof(__VA_ARGS__))
+#define filemap_alloc_folio_mpol(...)				\
+	alloc_hooks(filemap_alloc_folio_mpol_noprof(__VA_ARGS__))
 
 static inline struct page *__page_cache_alloc(gfp_t gfp)
 {
@@ -652,6 +662,8 @@ static inline fgf_t fgf_set_order(size_t size)
 void *filemap_get_entry(struct address_space *mapping, pgoff_t index);
 struct folio *__filemap_get_folio(struct address_space *mapping, pgoff_t index,
 		fgf_t fgp_flags, gfp_t gfp);
+struct folio *__filemap_get_folio_mpol(struct address_space *mapping,
+		pgoff_t index, fgf_t fgp_flags, gfp_t gfp, struct mempolicy *mpol);
 struct page *pagecache_get_page(struct address_space *mapping, pgoff_t index,
 		fgf_t fgp_flags, gfp_t gfp);
 
@@ -710,6 +722,34 @@ static inline struct folio *filemap_grab_folio(struct address_space *mapping,
 			mapping_gfp_mask(mapping));
 }
 
+/**
+ * filemap_grab_folio_mpol - grab a folio from the page cache
+ * @mapping: The address space to search
+ * @index: The page index
+ * @mpol: The mempolicy to apply
+ *
+ * Same as filemap_grab_folio(), except that it allocates the folio using
+ * given memory policy.
+ *
+ * Return: A found or created folio. ERR_PTR(-ENOMEM) if no folio is found
+ * and failed to create a folio.
+ */
+#ifdef CONFIG_NUMA
+static inline struct folio *filemap_grab_folio_mpol(struct address_space *mapping,
+					pgoff_t index, struct mempolicy *mpol)
+{
+	return __filemap_get_folio_mpol(mapping, index,
+			FGP_LOCK | FGP_ACCESSED | FGP_CREAT,
+			mapping_gfp_mask(mapping), mpol);
+}
+#else
+static inline struct folio *filemap_grab_folio_mpol(struct address_space *mapping,
+					pgoff_t index, struct mempolicy *mpol)
+{
+	return filemap_grab_folio(mapping, index);
+}
+#endif /* CONFIG_NUMA */
+
 /**
  * find_get_page - find and get a page reference
  * @mapping: the address_space to search
diff --git a/mm/filemap.c b/mm/filemap.c
index d62150418b91..a870a05296c8 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -990,8 +990,13 @@ int filemap_add_folio(struct address_space *mapping, struct folio *folio,
 EXPORT_SYMBOL_GPL(filemap_add_folio);
 
 #ifdef CONFIG_NUMA
-struct folio *filemap_alloc_folio_noprof(gfp_t gfp, unsigned int order)
+struct folio *filemap_alloc_folio_mpol_noprof(gfp_t gfp, unsigned int order,
+			struct mempolicy *mpol)
 {
+	if (mpol)
+		return folio_alloc_mpol_noprof(gfp, order, mpol,
+				NO_INTERLEAVE_INDEX, numa_node_id());
+
 	int n;
 	struct folio *folio;
 
@@ -1007,6 +1012,12 @@ struct folio *filemap_alloc_folio_noprof(gfp_t gfp, unsigned int order)
 	}
 	return folio_alloc_noprof(gfp, order);
 }
+EXPORT_SYMBOL(filemap_alloc_folio_mpol_noprof);
+
+struct folio *filemap_alloc_folio_noprof(gfp_t gfp, unsigned int order)
+{
+	return filemap_alloc_folio_mpol_noprof(gfp, order, NULL);
+}
 EXPORT_SYMBOL(filemap_alloc_folio_noprof);
 #endif
 
@@ -1861,11 +1872,12 @@ void *filemap_get_entry(struct address_space *mapping, pgoff_t index)
 }
 
 /**
- * __filemap_get_folio - Find and get a reference to a folio.
+ * __filemap_get_folio_mpol - Find and get a reference to a folio.
  * @mapping: The address_space to search.
  * @index: The page index.
  * @fgp_flags: %FGP flags modify how the folio is returned.
  * @gfp: Memory allocation flags to use if %FGP_CREAT is specified.
+ * @mpol: The mempolicy to apply.
  *
  * Looks up the page cache entry at @mapping & @index.
  *
@@ -1876,8 +1888,8 @@ void *filemap_get_entry(struct address_space *mapping, pgoff_t index)
  *
  * Return: The found folio or an ERR_PTR() otherwise.
  */
-struct folio *__filemap_get_folio(struct address_space *mapping, pgoff_t index,
-		fgf_t fgp_flags, gfp_t gfp)
+struct folio *__filemap_get_folio_mpol(struct address_space *mapping, pgoff_t index,
+		fgf_t fgp_flags, gfp_t gfp, struct mempolicy *mpol)
 {
 	struct folio *folio;
 
@@ -1947,7 +1959,7 @@ struct folio *__filemap_get_folio(struct address_space *mapping, pgoff_t index,
 			err = -ENOMEM;
 			if (order > 0)
 				alloc_gfp |= __GFP_NORETRY | __GFP_NOWARN;
-			folio = filemap_alloc_folio(alloc_gfp, order);
+			folio = filemap_alloc_folio_mpol(alloc_gfp, order, mpol);
 			if (!folio)
 				continue;
 
@@ -1978,6 +1990,14 @@ struct folio *__filemap_get_folio(struct address_space *mapping, pgoff_t index,
 		return ERR_PTR(-ENOENT);
 	return folio;
 }
+EXPORT_SYMBOL(__filemap_get_folio_mpol);
+
+struct folio *__filemap_get_folio(struct address_space *mapping, pgoff_t index,
+		fgf_t fgp_flags, gfp_t gfp)
+{
+	return __filemap_get_folio_mpol(mapping, index,
+			fgp_flags, gfp, NULL);
+}
 EXPORT_SYMBOL(__filemap_get_folio);
 
 static inline struct folio *find_get_entry(struct xa_state *xas, pgoff_t max,
-- 
2.34.1


^ permalink raw reply related

* [RFC PATCH 0/4] Add fbind() and NUMA mempolicy support for KVM guest_memfd
From: Shivank Garg @ 2024-11-05 16:45 UTC (permalink / raw)
  To: x86, viro, brauner, jack, akpm, linux-kernel, linux-fsdevel,
	linux-mm, linux-api, linux-arch, kvm
  Cc: chao.gao, pgonda, thomas.lendacky, seanjc, luto, tglx, mingo, bp,
	dave.hansen, willy, arnd, pbonzini, kees, shivankg, bharata,
	nikunj, michael.day, Neeraj.Upadhyay

This patch series introduces fbind() syscall to support NUMA memory
policies for KVM guest_memfd, allowing VMMs to configure memory placement
for guest memory. This addresses the current limitation where guest_memfd
allocations ignore NUMA policies, potentially impacting performance of
memory-locality-sensitive workloads.

Currently, while mbind() enables NUMA policy support for userspace
applications, it cannot be used with guest_memfd as the memory isn't mapped
to userspace. This results in guest memory being allocated randomly across
host NUMA nodes, even when specific policies and node preferences are
specified in QEMU commands.

The fbind() syscall is particularly useful for SEV-SNP guests.

Following suggestions from LPC and review comment [1], I switched from an
IOCTL-based approach [2] to fbind() [3]. The fbind() approach is preferred
as it provides a generic NUMA policy configuration working with any fd,
rather than being tied to guest-memfd.

Testing:
QEMU tree- https://github.com/AMDESE/qemu/tree/guest_memfd_fbind_NUMA
Based upon
Github tree- https://github.com/AMDESE/linux/tree/snp-host-latest
Branch: snp-host-latest commit: 85ef1ac

Example command to run a SEV-SNP guest bound to NUMA Node 0 of the host:
$ qemu-system-x86_64 \
   -enable-kvm \
  ...
   -machine memory-encryption=sev0,vmport=off \
   -object sev-snp-guest,id=sev0,cbitpos=51,reduced-phys-bits=1 \
   -numa node,nodeid=0,memdev=ram0,cpus=0-15 \
   -object memory-backend-memfd,id=ram0,policy=bind,host-nodes=0,size=1024M,share=true,prealloc=false

[1]: https://lore.kernel.org/linux-mm/ZvEga7srKhympQBt@intel.com/
[2]: https://lore.kernel.org/linux-mm/20240919094438.10987-1-shivankg@amd.com
[3]: https://lore.kernel.org/kvm/ZOjpIL0SFH+E3Dj4@google.com/


Shivank Garg (3):
  Introduce fbind syscall
  KVM: guest_memfd: Pass file pointer instead of inode in guest_memfd
    APIs
  KVM: guest_memfd: Enforce NUMA mempolicy if available

Shivansh Dhiman (1):
  mm: Add mempolicy support to the filemap layer

 arch/x86/entry/syscalls/syscall_32.tbl |  1 +
 arch/x86/entry/syscalls/syscall_64.tbl |  1 +
 include/linux/fs.h                     |  3 ++
 include/linux/mempolicy.h              |  3 ++
 include/linux/pagemap.h                | 40 ++++++++++++++++
 include/linux/syscalls.h               |  3 ++
 include/uapi/asm-generic/unistd.h      |  5 +-
 kernel/sys_ni.c                        |  1 +
 mm/Makefile                            |  2 +-
 mm/fbind.c                             | 49 +++++++++++++++++++
 mm/filemap.c                           | 30 ++++++++++--
 mm/mempolicy.c                         | 57 ++++++++++++++++++++++
 virt/kvm/guest_memfd.c                 | 66 +++++++++++++++++++++-----
 13 files changed, 242 insertions(+), 19 deletions(-)
 create mode 100644 mm/fbind.c

-- 
2.34.1


^ permalink raw reply

* Re: [PATCH net 1/1] net/ipv6: Netlink flag for new IPv6 Default Routes
From: Nicolas Dichtel @ 2024-11-05 14:37 UTC (permalink / raw)
  To: Matt Muggeridge
  Cc: David Ahern, David S . Miller, linux-api, stable, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Simon Horman, netdev, linux-kernel
In-Reply-To: <20241105031841.10730-2-Matt.Muggeridge@hpe.com>

Le 05/11/2024 à 04:18, Matt Muggeridge a écrit :
> Add a Netlink rtm_flag, RTM_F_RA_ROUTER for the RTM_NEWROUTE message.
> This allows an IPv6 Netlink client to indicate the default route came
> from an RA. This results in the kernel creating individual default
> routes, rather than coalescing multiple default routes into a single
> ECMP route.
> 
> Details:
> 
> For IPv6, a Netlink client is unable to create default routes in the
> same manner as the kernel. This leads to failures when there are
> multiple default routers, as they were being coalesced into a single
> ECMP route. When one of the ECMP default routers becomes UNREACHABLE, it
> was still being selected as the nexthop.
> 
> Meanwhile, when the kernel processes RAs from multiple default routers,
> it sets the fib6_flags: RTF_ADDRCONF | RTF_DEFAULT. The RTF_ADDRCONF
> flag is checked by rt6_qualify_for_ecmp(), which returns false when
> ADDRCONF is set. As such, the kernel creates separate default routes.
> 
> E.g. compare the routing tables when RAs are processed by the kernel
> versus a Netlink client (systemd-networkd, in my case).
> 
> 1) RA Processed by kernel (accept_ra = 2)
> $ ip -6 route
> 2001:2:0:1000::/64 dev enp0s9 proto kernel metric 256 expires ...
> fe80::/64 dev enp0s9 proto kernel metric 256 pref medium
> default via fe80::200:10ff:fe10:1060 dev enp0s9 proto ra ...
> default via fe80::200:10ff:fe10:1061 dev enp0s9 proto ra ...
> 
> 2) RA Processed by Netlink client (accept_ra = 0)
> $ ip -6 route
> 2001:2:0:1000::/64 dev enp0s9 proto ra metric 1024 expires ...
> fe80::/64 dev enp0s3 proto kernel metric 256 pref medium
> fe80::/64 dev enp0s9 proto kernel metric 256 pref medium
> default proto ra metric 1024 expires 595sec pref medium
> 	nexthop via fe80::200:10ff:fe10:1060 dev enp0s9 weight 1
> 	nexthop via fe80::200:10ff:fe10:1061 dev enp0s9 weight 1
> 
> IPv6 Netlink clients need a mechanism to identify a route as coming from
> an RA. i.e. a Netlink client needs a method to set the kernel flags:
> 
>     RTF_ADDRCONF | RTF_DEFAULT
> 
> This is needed when there are multiple default routers that each send
> an RA. Setting the RTF_ADDRCONF flag ensures their fib entries do not
> qualify for ECMP routes, see rt6_qualify_for_ecmp().
> 
> To achieve this, introduce a user-level flag RTM_F_RA_ROUTER that a
> Netlink client can pass to the kernel.
> 
> A Netlink user-level network manager, such as systemd-networkd, may set
> the RTM_F_RA_ROUTER flag in the Netlink RTM_NEWROUTE rtmsg. When set,
> the kernel sets RTF_RA_ROUTER in the fib6_config fc_flags. This causes a
> default route to be created in the same way as if the kernel processed
> the RA, via rt6add_dflt_router().
> 
> This is needed by user-level network managers, like systemd-networkd,
> that prefer to do the RA processing themselves. ie. they disable the
> kernel's RA processing by setting net.ipv6.conf.<intf>.accept_ra=0.
> 
> Without this flag, when there are mutliple default routers, the kernel
> coalesces multiple default routes into an ECMP route. The ECMP route
> ignores per-route REACHABILITY information. If one of the default
> routers is unresponsive, with a Neighbor Cache entry of INCOMPLETE, then
> it can still be selected as the nexthop for outgoing packets. This
> results in an inability to communicate with remote hosts, even though
> one of the default routers remains REACHABLE. This violates RFC4861
> section 6.3.6, bullet 1.
> 
> Extract from RFC4861 6.3.6 bullet 1:
>      1) Routers that are reachable or probably reachable (i.e., in any
>         state other than INCOMPLETE) SHOULD be preferred over routers
>         whose reachability is unknown or suspect (i.e., in the
>         INCOMPLETE state, or for which no Neighbor Cache entry exists).
> 
> This fixes the IPv6 Logo conformance test v6LC_2_2_11, and others that
> test with multiple default routers. Also see systemd issue #33470:
> https://github.com/systemd/systemd/issues/33470.
> 
> Signed-off-by: Matt Muggeridge <Matt.Muggeridge@hpe.com>
> Cc: David Ahern <dsahern@kernel.org>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: linux-api@vger.kernel.org
> Cc: stable@vger.kernel.org
> ---
>  include/uapi/linux/rtnetlink.h | 9 +++++----
>  net/ipv6/route.c               | 3 +++
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
> index 3b687d20c9ed..9f0259f6e4ed 100644
> --- a/include/uapi/linux/rtnetlink.h
> +++ b/include/uapi/linux/rtnetlink.h
> @@ -202,7 +202,7 @@ enum {
>  #define RTM_NR_FAMILIES	(RTM_NR_MSGTYPES >> 2)
>  #define RTM_FAM(cmd)	(((cmd) - RTM_BASE) >> 2)
>  
> -/* 
> +/*
>     Generic structure for encapsulation of optional route information.
>     It is reminiscent of sockaddr, but with sa_family replaced
>     with attribute type.
> @@ -242,7 +242,7 @@ struct rtmsg {
>  
>  	unsigned char		rtm_table;	/* Routing table id */
>  	unsigned char		rtm_protocol;	/* Routing protocol; see below	*/
> -	unsigned char		rtm_scope;	/* See below */	
> +	unsigned char		rtm_scope;	/* See below */
>  	unsigned char		rtm_type;	/* See below	*/
>  
>  	unsigned		rtm_flags;
> @@ -336,6 +336,7 @@ enum rt_scope_t {
>  #define RTM_F_FIB_MATCH	        0x2000	/* return full fib lookup match */
>  #define RTM_F_OFFLOAD		0x4000	/* route is offloaded */
>  #define RTM_F_TRAP		0x8000	/* route is trapping packets */
> +#define RTM_F_RA_ROUTER		0x10000	/* route is a default route from RA */
Please, don't mix whitespace changes with the changes related to the new flag.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst#n166


Regards,
Nicolas

^ permalink raw reply

* Re: [PATCH v2 0/3] futex: Create set_robust_list2
From: Peter Zijlstra @ 2024-11-05 12:18 UTC (permalink / raw)
  To: Florian Weimer
  Cc: André Almeida, Thomas Gleixner, Ingo Molnar, Darren Hart,
	Davidlohr Bueso, Arnd Bergmann, sonicadvance1, linux-kernel,
	kernel-dev, linux-api, Nathan Chancellor, Mathieu Desnoyers
In-Reply-To: <87jzdjxjj8.fsf@oldenburg3.str.redhat.com>

On Mon, Nov 04, 2024 at 01:36:43PM +0100, Florian Weimer wrote:
> * Peter Zijlstra:
> 
> > On Sat, Nov 02, 2024 at 10:58:42PM +0100, Florian Weimer wrote:
> >
> >> QEMU hints towards further problems (in linux-user/syscall.c):
> >> 
> >>     case TARGET_NR_set_robust_list:
> >>     case TARGET_NR_get_robust_list:
> >>         /* The ABI for supporting robust futexes has userspace pass
> >>          * the kernel a pointer to a linked list which is updated by
> >>          * userspace after the syscall; the list is walked by the kernel
> >>          * when the thread exits. Since the linked list in QEMU guest
> >>          * memory isn't a valid linked list for the host and we have
> >>          * no way to reliably intercept the thread-death event, we can't
> >>          * support these. Silently return ENOSYS so that guest userspace
> >>          * falls back to a non-robust futex implementation (which should
> >>          * be OK except in the corner case of the guest crashing while
> >>          * holding a mutex that is shared with another process via
> >>          * shared memory).
> >>          */
> >>         return -TARGET_ENOSYS;
> >
> > I don't think we can sanely fix that. Can't QEMU track the robust thing
> > itself and use waitpid() to discover the thread is gone and fudge things
> > from there?
> 
> There are race conditions with munmap, I think, and they probably get a
> lot of worse if QEMU does that.
> 
> See Rich Felker's bug report:
> 
> | The corruption is performed by the kernel when it walks the robust
> | list. The basic situation is the same as in PR #13690, except that
> | here there's actually a potential write to the memory rather than just
> | a read.
> | 
> | The sequence of events leading to corruption goes like this:
> | 
> | 1. Thread A unlocks the process-shared, robust mutex and is preempted
> |    after the mutex is removed from the robust list and atomically
> |    unlocked, but before it's removed from the list_op_pending field of
> |    the robust list header.
> | 
> | 2. Thread B locks the mutex, and, knowing by program logic that it's
> |    the last user of the mutex, unlocks and unmaps it, allocates/maps
> |    something else that gets assigned the same address as the shared mutex
> |    mapping, and then exits.
> | 
> | 3. The kernel destroys the process, which involves walking each
> |   thread's robust list and processing each thread's list_op_pending
> |   field of the robust list header. Since thread A has a list_op_pending
> |   pointing at the address previously occupied by the mutex, the kernel
> |   obliviously "unlocks the mutex" by writing a 0 to the address and
> |   futex-waking it. However, the kernel has instead overwritten part of
> |   whatever mapping thread A created. If this is private memory it
> |   (probably) doesn't matter since the process is ending anyway (but are
> |   there race conditions where this can be seen?). If this is shared
> |   memory or a shared file mapping, however, the kernel corrupts it.
> | 
> | I suspect the race is difficult to hit since thread A has to get
> | preempted at exactly the wrong time AND thread B has to do a fair
> | amount of work without thread A getting scheduled again. So I'm not
> | sure how much luck we'd have getting a test case.
> 
> 
> <https://sourceware.org/bugzilla/show_bug.cgi?id=14485#c3>

So I've only managed to conjure up two horrible solutions for this:

 - put the robust futex operations under user-space RCU, and mandate a
   matching synchronize_rcu() before any munmap() calls.

 - add a robust-barrier syscall that waits until all list_op_pending are
   either NULL or changed since invocation. And mandate this call before
   munmap().

Neither are particularly pretty I admit, but at least they should work.

But doing this and mandating the alignment thing should at least make
this qemu thing workable, no?

> We also have a silent unlocking failure because userspace does not know
> about ROBUST_LIST_LIMIT:
> 
>   Bug 19089 - Robust mutexes do not take ROBUST_LIST_LIMIT into account
>   <https://sourceware.org/bugzilla/show_bug.cgi?id=19089>
> 
> (I think we may have discussed this one before, and you may have
> suggested to just hard-code 2048 in userspace because the constant is
> not expected to change.)
> 
> So the in-mutex linked list has quite a few problems even outside of
> emulation. 8-(

It's futex, ofcourse its a pain in the arse :-)

And yeah, no better ideas on that limit for now...

^ permalink raw reply


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