* Re: [PATCH v8 09/46] KVM: guest_memfd: Introduce function to check GFN private/shared status
From: Binbin Wu @ 2026-06-25 1:39 UTC (permalink / raw)
To: Ackerley Tng
Cc: aik, andrew.jones, brauner, chao.p.peng, david, jmattson,
jthoughton, michael.roth, oupton, pankaj.gupta, qperret,
rick.p.edgecombe, rientjes, shivankg, steven.price, tabba, willy,
wyihan, yan.y.zhao, forkloop, pratyush, suzuki.poulose,
aneesh.kumar, liam, Paolo Bonzini, Sean Christopherson,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Steven Rostedt, Masami Hiramatsu,
Mathieu Desnoyers, Jonathan Corbet, Shuah Khan, Shuah Khan,
Vishal Annapurve, Andrew Morton, Chris Li, Kairui Song,
Kemeng Shi, Nhat Pham, Barry Song, Axel Rasmussen, Yuanchu Xie,
Wei Xu, Youngjun Park, Qi Zheng, Shakeel Butt, Kiryl Shutsemau,
Baoquan He, Jason Gunthorpe, Vlastimil Babka, kvm, linux-kernel,
linux-trace-kernel, linux-doc, linux-kselftest, linux-mm,
linux-coco
In-Reply-To: <CAEvNRgG-WDzHp-15Mig4hiU5Dag0pFCu70-R-9b=PkD69W=ZMg@mail.gmail.com>
On 6/24/2026 10:38 PM, Ackerley Tng wrote:
> Binbin Wu <binbin.wu@linux.intel.com> writes:
>
>>
>> [...snip...]
>>
>>> +bool kvm_gmem_is_private(struct kvm *kvm, gfn_t gfn)
>>> +{
>>> + struct kvm_memory_slot *slot = gfn_to_memslot(kvm, gfn);
>>> + struct inode *inode;
>>> +
>>> + /*
>>> + * If this gfn has no associated memslot, there's no chance of the gfn
>>> + * being backed by private memory, since guest_memfd must be used for
>>> + * private memory,
>>
>> "guest_memfd must be used for private memory" is a bit confusing to me.
>>
>
> Hmm good point. Is the source of confusion that guest_memfd can be used
> for both shared and private memory?
Yes.
>
> Perhaps this can be rephrased as:
>
> guest_memfd is the only provider of private memory and guest_memfd must
> be used with a memslot, hence if there's no associated memslot, there's
> no chance of this gfn being private.
LGTM.
>
>>> and guest_memfd must be associated with some memslot.
>>> + */
>>> + if (!slot)
>>> + return 0;
>>> +
>>>
>>> [...snip...]
>>>
>
^ permalink raw reply
* Re: [PATCH v8 24/46] KVM: guest_memfd: Make in-place conversion the default
From: Yan Zhao @ 2026-06-25 1:51 UTC (permalink / raw)
To: Sean Christopherson
Cc: Ackerley Tng, aik, andrew.jones, binbin.wu, brauner, chao.p.peng,
david, jmattson, jthoughton, michael.roth, oupton, pankaj.gupta,
qperret, rick.p.edgecombe, rientjes, shivankg, steven.price,
tabba, willy, wyihan, forkloop, pratyush, suzuki.poulose,
aneesh.kumar, liam, Paolo Bonzini, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, Steven Rostedt,
Masami Hiramatsu, Mathieu Desnoyers, Jonathan Corbet, Shuah Khan,
Shuah Khan, Vishal Annapurve, Andrew Morton, Chris Li,
Kairui Song, Kemeng Shi, Nhat Pham, Barry Song, Axel Rasmussen,
Yuanchu Xie, Wei Xu, Youngjun Park, Qi Zheng, Shakeel Butt,
Kiryl Shutsemau, Baoquan He, Jason Gunthorpe, Vlastimil Babka,
kvm, linux-kernel, linux-trace-kernel, linux-doc, linux-kselftest,
linux-mm, linux-coco
In-Reply-To: <ajx5Vrz9ma--hrGH@google.com>
On Wed, Jun 24, 2026 at 05:41:58PM -0700, Sean Christopherson wrote:
> On Wed, Jun 24, 2026, Ackerley Tng wrote:
> > Yan Zhao <yan.y.zhao@intel.com> writes:
> > > With gmem_in_place_conversion=true, userspace can create guest_memfd without the
> > > MMAP flag. In such cases, shared memory is allocated from different backends.
> > > This means this module parameter only enables per-gmem memory attribute and does
> > > not guarantee that gmem in-place conversion will actually occur.
>
> KVM module params are pretty much always about what KVM supports, not what is
> guaranteed to happen.
>
> - enable_mmio_caching doesn't guarantee there will actually be MMIO SPTEs,
> because maybe the guest never accesses emulated MMIO.
> - enable_pmu doesn't guarantee VMs will get a PMU, because userspace may elect
> not to advertise one.
> - and so on and so forth...
>
> Yes, there's a small mental jump to get from "KVM supports in-place conversion"
> to "I need to set memory attributes on the guest_memfd instance, not the VM",
> but I don't see that as a big hurdle, certainly not in the long term. And once
> the VMM code is written, I really do think most people are going to care about
> whether or not KVM supports in-place conversion, not where PRIVATE is tracked.
Sorry, I just saw this mail after posting my reply in [1].
I'm ok with gmem_in_place_conversion=true just means KVM supports in-place
conversion, while we can still create VMs with shared memory not from gmem.
Though it still feels a bit odd to require TDX huge pages to depend on
gmem_in_place_conversion=true when shared memory is not currently allocated from
gmem, it should become more natural over time once gmem supports in-place
conversions for huge page.
[1] https://lore.kernel.org/all/ajyCn0PnFtQK+Nka@yzhao56-desk.sh.intel.com
> > > To avoid confusion, could we rename this module parameter to something more
> > > accurate, such as gmem_memory_attribute?
> >
> > I asked Sean about this after getting some fixes off list. Sean said
> > gmem_in_place_conversion is named for a host admin to use, and something
> > like gmem_memory_attributes is too much implementation details for the
> > admin.
> >
> > Sean, would you reconsider since Yan also asked? If the admin compiled
> > the kernel knowing what CONFIG_KVM_VM_MEMORY_ATTRIBUTES means, then the
> > admin would also be able to use a param like gmem_memory_attributes?
>
> No, because it's not all memory attributes, it's very specifically the PRIVATE
> attribute that will get moved to guest_memfd. I don't want to pick a name that
> will become stale and confusing when RWX attributes come along. The RWX bits
> will be per-VM, while PRIVATE will be per-guest_memfd.
^ permalink raw reply
* [RFC PATCH 0/0] PCI P2PDMA: Add observability support via tracepoints, debugfs, and sysfs.
From: xiaobing.li @ 2026-06-25 1:59 UTC (permalink / raw)
To: bhelgaas, logang, m.szyprowski, linux-pci, linux-kernel,
linux-trace-kernel
Cc: kun.dou, peiwei.li
In-Reply-To: <CGME20260625015930epcas5p33fa9d4833d45b53597e2994fb9ec2577@epcas5p3.samsung.com>
Hi all,
The Linux kernel's P2P DMA infrastructure is already very mature, but currently it is not user-friendly in terms of metric observability.
For example, without manually adding logs, there is no intuitive data to see how many P2P transfers, which paths are taken,
and how performance is. It is impossible to clearly observe P2PDMA activity from user space, making the following operations difficult:
- Diagnose the reasons why P2PDMA may not work (or perform poorly).
- Verify whether the P2PDMA mapping uses the expected type (BUS_ADDR or THRU_HOST_BRIDGE)
- Monitor the use of P2PDMA in production environments
- Detect potential memory leaks (unmapped allocations)
P2PDMA is a subtle feature. When P2PDMA mapping cannot use BUS_ADDR (Direct PCIe Switch Path), it silently falls back to the THRU_HOST_BRIDGE,
routing traffic to the host bridge. This significantly reduces performance (usually by 10 times or more), but it cannot be detected
from user space.
Therefore, I plan to export some metrics in the user space to better observe P2PDMA activity.
This series of solutions adds three layers of observability:
1. Tracepoints (5 events, optional, no overhead when disabled)
- p2p_dma_alloc: P2P memory allocation
- p2p_dma_free: P2P memory release
- p2p_dma_map: P2P DMA mapping (including client/provider, mapping type,
PCIe distance and process information)
- p2p_dma_unmap: P2P DMA removes mapping
- p2p_map_type_change: New mapping type calculations (xarray missed)
All tracking points include the calling process (comm pid), enabling P2PDMA activity tracking for each process.
Example:
$ cat /sys/kernel/debug/tracing/trace | grep p2p_dma_map
nvme[1234] map nvme0 -> p2p_mem type=BUS_ADDR dist=4
python[5678] map nvme1 -> p2p_mem type=THRU_HOST_BRIDGE dist=8
2. Debugfs (global cumulative counter, always available)
- /sys/kernel/debug/pci-p2pdma/
- 11 counters: total_mappings, bus_addr_mappings, host_bridge_mappings,
total_allocations, error_count, etc.
- Enable the calculation of the "BUS_ADDR ratio" to quantify the effectiveness of P2PDMA.
3. Sysfs (Statistical Information for Each Device, Production Environment Safety)
- /sys/bus/pci/devices/*/p2pmem/stats/
- 4 attributes: alloc_count, free_count, mapped_bytes, peak_mapped_bytes
Performance impact
- Tracking point: Static branch, zero overhead when disabled (by default).
- Debugfs/sysfs: atomic64_t counter, no locking, negligible overhead
- After disabling all observability, the P2PDMA thermal path remains unchanged
I would appreciate feedback on:
1. Is the overall solution worth implementing?
2. Is the set of tracepoints appropriate? Any events I'm missing?
3. Are the tracepoint fields sufficient for debugging?
4. Is the debugfs/sysfs interface design acceptable?
5. Any concerns about the implementation approach?
^ permalink raw reply
* Re: [PATCH v8 23/46] KVM: TDX: Make source page optional for KVM_TDX_INIT_MEM_REGION
From: Yan Zhao @ 2026-06-25 2:25 UTC (permalink / raw)
To: Ackerley Tng
Cc: Sean Christopherson, aik, andrew.jones, binbin.wu, brauner,
chao.p.peng, david, jmattson, jthoughton, michael.roth, oupton,
pankaj.gupta, qperret, rick.p.edgecombe, rientjes, shivankg,
steven.price, tabba, willy, wyihan, forkloop, pratyush,
suzuki.poulose, aneesh.kumar, liam, Paolo Bonzini,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Steven Rostedt, Masami Hiramatsu,
Mathieu Desnoyers, Jonathan Corbet, Shuah Khan, Shuah Khan,
Vishal Annapurve, Andrew Morton, Chris Li, Kairui Song,
Kemeng Shi, Nhat Pham, Barry Song, Axel Rasmussen, Yuanchu Xie,
Wei Xu, Youngjun Park, Qi Zheng, Shakeel Butt, Kiryl Shutsemau,
Baoquan He, Jason Gunthorpe, Vlastimil Babka, kvm, linux-kernel,
linux-trace-kernel, linux-doc, linux-kselftest, linux-mm,
linux-coco
In-Reply-To: <CAEvNRgG1nHipzw4=eBgwhvyXi8xYo7FQD_sy9Ax6FDf7YDu3Og@mail.gmail.com>
On Wed, Jun 24, 2026 at 04:00:32PM -0700, Ackerley Tng wrote:
> Sean Christopherson <seanjc@google.com> writes:
>
> > On Tue, Jun 23, 2026, Yan Zhao wrote:
> >> On Tue, Jun 23, 2026 at 01:16:14PM +0800, Yan Zhao wrote:
> >> > On Mon, Jun 22, 2026 at 06:22:45PM -0700, Sean Christopherson wrote:
> >> > > On Mon, Jun 22, 2026, Yan Zhao wrote:
> >> > > > On Thu, Jun 18, 2026 at 05:32:00PM -0700, Ackerley Tng via B4 Relay wrote:
> >> > > > > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> >> > > > > index ffe9d0db58c59..56d10333c61a7 100644
> >> > > > > --- a/arch/x86/kvm/vmx/tdx.c
> >> > > > > +++ b/arch/x86/kvm/vmx/tdx.c
> >> > > > > @@ -3198,8 +3198,12 @@ static int tdx_gmem_post_populate(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn,
> >> > > > > if (KVM_BUG_ON(kvm_tdx->page_add_src, kvm))
> >> > > > > return -EIO;
> >> > > > >
> >> > > > > - if (!src_page)
> >> > > > > - return -EOPNOTSUPP;
> >> > > > > + if (!src_page) {
> >> > > > > + if (!gmem_in_place_conversion)
> >> > > > When userspace turns on gmem_in_place_conversion while creating guest_memfd
> >> > > > without the MMAP flag, the absence of src_page should still be treated as an
> >> > > > error.
> >> > >
> >> > > Why MMAP?
> >> > Hmm, I was showing a scenario that in-place conversion couldn't occur.
> >> > I didn't mean that with the MMAP flag, mmap() and user write must occur.
> >> >
> >> > > Shouldn't this be a general "if (!src_page && !up-to-date)"? Just
> >> > > because userspace _can_ mmap() the memory doesn't mean userspace _has_ mmap()'d
> >> > > and written memory. And when write() lands, MMAP wouldn't be necessary to
> >> > > initialize the memory.
> >> > Do you mean using up-to-date flag as below?
> >
> > Yes? I didn't actually look at the implementation details.
> >
> >> > if (!src_page) {
> >> > src_page = pfn_to_page(pfn);
> >> > if (!folio_test_uptodate(page_folio(src_page)))
> >> > return -EOPNOTSUPP;
> >> > }
>
> Yan is right that with the earlier patch "Zero page while getting pfn",
> folio_test_uptodate() here will always return true.
>
> Actually, this is an alternative fix for the issue Sashiko pointed out
> on v7 where userspace can do a populate() (either TDX or SNP) without
> first allocating the page, with src_address == NULL, and leak
> uninitialized memory into the guest.
>
> Advantage of using the uptodate check in populate: if the host never
> allocates the page, populate doesn't incur zeroing before writing the
> page anyway in populate().
>
> Disadvantage: Both TDX and SNP will have to implement this uptodate
> check. guest_memfd can't check centrally because for SNP, for a
> PAGE_TYPE_ZERO, !src_page should be allowed with a !uptodate page since
> firmware will zero and there's no leakage of uninitialized host memory?
Another disadvantage: the uptodate flag is per-folio. What if the folio
is only partially initialized by the userspace especially after huge page is
supported?
> >> Another concern with this fix is that:
> >> commit "KVM: guest_memfd: Zero page while getting pfn" [1] always marks the
> >> folio uptodate before reaching post_populate().
> >>
> >> [1] https://lore.kernel.org/all/20260618-gmem-inplace-conversion-v8-21-9d2959357853@google.com/
> >>
> >> > One concern is that TDX now does not much care about the up-to-date flag since
> >> > TDX doesn't rely on the flag to clear pages on conversions.
> >> > I'm not sure if the flag can be reliably checked in this case. e.g.,
> >> > now the whole folio is marked up-to-date even if only part of it is faulted by
> >> > user access.
> >> > Ensuring that the up-to-date flag works correctly with huge page support seems
> >> > to have more effort than introducing a dedicated flag for TDX.
> >> >
> >> > > > Additionally, to properly enable in-place copying for the TDX initial memory
> >> > > > region, userspace must not only specify source_addr to NULL, but also follow
> >> > > > a specific sequence (where steps 1/2/3/7 are required only for in-place copy):
> >> > > > 1. create guest_memfd with MMAP flag
> >> > > > 2. mmap the guest_memfd.
> >> > > > 3. convert the initial memory range to shared.
> >> > > > 4. copy initial content to the source page.
> >> > > > 5. convert the initial memory range to private
> >> > > > 6. invoke ioctl KVM_TDX_INIT_MEM_REGION.
> >> > > > 7. do not unmap the source backend.
> >> > > >
> >> > > > So, would it be reasonable to introduce a dedicated flag that allows userspace
> >> > > > to explicitly opt into the in-place copy functionality? e.g.,
> >> > >
> >> > > Why? It's userspace's responsibility to get the above right. If userspace fails
> >> > > to provide a src_page when it doesn't want in-place copy, that's a userspace bug.
>
> Yan, is your concern that userspace forgot to update the code and
> forgets to provide a src_page, and if we keep the "Zero page while
Yes. Previously, it would be rejected after GUP fails.
> getting pfn" patch, ends up with the guest silently having a zero page?
> I think that would be found quite early in userspace VMM testing...
I actually encountered this during testing this patch.
I update most code path to follow this sequence. However, still some corner ones
for TDVF HOB, which are less obvious and harder to update.
The TD just booted up and hang silently.
> >> > I mean if userspace specifies a NULL source_addr by mistake, it's better for
> >> > kernel to detect this mistake, similar to how it validates whether source_addr
> >> > is PAGE_ALIGNED.
> >
> > The alignment case is different. If userspace provides an unaligned value, KVM
> > *can't* do what userspace is asking because hardware and thus KVM only supports
> > converting on page boundaries.
> >
> > For a NULL source, KVM can still do what userspace is asking. Rejecting userspace's
> > request would then be making assumptions about what userspace wants.
> >
>
> Also, +1 on this, what if userspace, knowing that pages are zeroed on
> allocation, actually wants to rely on that to get a zero page in the guest?
What if 0 uaddr is a valid address? :)
> >> > Since userspace already needs to perform additional steps to enable in-place
> >> > copy, specifying a dedicated flag to indicate that the NULL source_addr is
> >> > intentional seems like a reasonable burden.
> >
> > I don't see how it adds any value. I wouldn't be at all surprised if most VMMs
> > just wen up with code that does:
> >
> > if (in-place) {
> > src = NULL;
> > flags |= KVM_TDX_IN_PLACE_COPY_INITIAL_MEMORY_REGION;
> > }
>
^ permalink raw reply
* Re: [PATCH v3 1/7] list: Add mutable iterator variants
From: Kaitao Cheng @ 2026-06-25 3:01 UTC (permalink / raw)
To: David Laight, Christian König, Jani Nikula,
David Hildenbrand (Arm), Alexei Starovoitov
Cc: Andrew Morton, David Hildenbrand, Jens Axboe, Tejun Heo,
Alexander Viro, Christian Brauner, Daniel Borkmann,
Andrii Nakryiko, Johannes Weiner, Peter Zijlstra, Ingo Molnar,
Arnaldo Carvalho de Melo, Namhyung Kim, Thomas Gleixner,
Juri Lelli, Vincent Guittot, Paul Moore, Andy Shevchenko,
Paul E. McKenney, Shakeel Butt, David Howells, Simona Vetter,
Randy Dunlap, Luca Ceresoli, Philipp Stanner, linux-block,
linux-kernel, cgroups, linux-ntfs-dev, linux-fsdevel, io-uring,
audit, bpf, netdev, dri-devel, linux-perf-users,
linux-trace-kernel, kexec, live-patching, linux-modules,
linux-crypto, linux-pm, rcu, sched-ext, linux-mm, virtualization,
damon, llvm, Kaitao Cheng, Muchun Song
In-Reply-To: <20260624152324.3def88ce@pumpkin>
在 2026/6/24 22:23, David Laight 写道:
> On Wed, 24 Jun 2026 15:23:47 +0200
> Christian König <christian.koenig@amd.com> wrote:
>> On 6/24/26 15:14, Kaitao Cheng wrote:
>>> 在 2026/6/22 16:42, David Laight 写道:
>>>> On Mon, 22 Jun 2026 12:05:31 +0800
>>>> Kaitao Cheng <kaitao.cheng@linux.dev> wrote:
>>>>
>>>>> From: Kaitao Cheng <chengkaitao@kylinos.cn>
>>>>>
>>>>> The list_for_each*_safe() helpers are used when the loop body may
>>>>> remove the current entry. Their API exposes the temporary cursor at
>>>>> every call site, even though most users only need it for the iterator
>>>>> implementation and never reference it in the loop body.
>>>>>
>>>>> Add *_mutable() variants for list and hlist iteration. The new helpers
>>>>> support both forms: callers may keep passing an explicit temporary cursor
>>>>> when they need to inspect or reset it, or omit it and let the helper use
>>>>> a unique internal cursor.
>>>>
>>>> I'm not really sure 'mutable' means anything either.
>>>> It is possible to make it valid for the loop body (or even other threads)
>>>> to delete arbitrary list items - but that needs significant extra overheads.
>>>>
>>>> It might be worth doing something that doesn't need the extra variable,
>>>> but there is little point doing all the churn just to rename things.
>>>>
>>>>>
>>>>> This makes call sites that only mutate the list through the current entry
>>>>> less noisy, while keeping the existing *_safe() helpers available for
>>>>> compatibility.
>>>>>
>>>>> Signed-off-by: Kaitao Cheng <chengkaitao@kylinos.cn>
>>>>> ---
>>>>> include/linux/list.h | 269 +++++++++++++++++++++++++++++++++++++------
>>>>> 1 file changed, 231 insertions(+), 38 deletions(-)
>>>>>
>>>>> diff --git a/include/linux/list.h b/include/linux/list.h
>>>>> index 09d979976b3b..1081def7cea9 100644
>>>>> --- a/include/linux/list.h
>>>>> +++ b/include/linux/list.h
>>>>> @@ -7,6 +7,7 @@
>>>>> #include <linux/stddef.h>
>>>>> #include <linux/poison.h>
>>>>> #include <linux/const.h>
>>>>> +#include <linux/args.h>
>>>>>
>>>>> #include <asm/barrier.h>
>>>>>
>>>>> @@ -763,28 +764,72 @@ static inline void list_splice_tail_init(struct list_head *list,
>>>>> #define list_for_each_prev(pos, head) \
>>>>> for (pos = (head)->prev; !list_is_head(pos, (head)); pos = pos->prev)
>>>>>
>>>>> -/**
>>>>> - * list_for_each_safe - iterate over a list safe against removal of list entry
>>>>> - * @pos: the &struct list_head to use as a loop cursor.
>>>>> - * @n: another &struct list_head to use as temporary storage
>>>>> - * @head: the head for your list.
>>>>> +/*
>>>>> + * list_for_each_safe is an old interface, use list_for_each_mutable instead.
>>>>> */
>>>>> #define list_for_each_safe(pos, n, head) \
>>>>> for (pos = (head)->next, n = pos->next; \
>>>>> !list_is_head(pos, (head)); \
>>>>> pos = n, n = pos->next)
>>>>>
>>>>> +#define __list_for_each_mutable_internal(pos, tmp, head) \
>>>>> + for (typeof(pos) tmp = (pos = (head)->next)->next; \
>>>>
>>>> Use auto
>>>>
>>>>> + !list_is_head(pos, (head)); \
>>>>> + pos = tmp, tmp = pos->next)
>>>>> +
>>>>> +#define __list_for_each_mutable1(pos, head) \
>>>>> + __list_for_each_mutable_internal(pos, __UNIQUE_ID(next), head)
>>>>> +
>>>>> +#define __list_for_each_mutable2(pos, next, head) \
>>>>> + list_for_each_safe(pos, next, head)
>>>>> +
>>>>> /**
>>>>> - * list_for_each_prev_safe - iterate over a list backwards safe against removal of list entry
>>>>> + * list_for_each_mutable - iterate over a list safe against entry removal
>>>>> * @pos: the &struct list_head to use as a loop cursor.
>>>>> - * @n: another &struct list_head to use as temporary storage
>>>>> - * @head: the head for your list.
>>>>> + * @...: either (head) or (next, head)
>>>>> + *
>>>>> + * next: another &struct list_head to use as optional temporary storage.
>>>>> + * The temporary cursor is internal unless explicitly supplied by
>>>>> + * the caller.
>>>>> + * head: the head for your list.
>>>>> + */
>>>>> +#define list_for_each_mutable(pos, ...) \
>>>>> + CONCATENATE(__list_for_each_mutable, COUNT_ARGS(__VA_ARGS__)) \
>>>>> + (pos, __VA_ARGS__)
>>>>
>>>> The variable argument count logic really just slows down compilation.
>>>> Maybe there aren't enough copies of this code to make that significant.
>>>> But just because you can do it doesn't mean it is a gooD idea.
>>>> I'm also not sure it really adds anything to the readability.
>>>>
>>>> And, it you are going to make the middle argument optional there is
>>>> no need to change the macro name.
>>>
>>> Christian König and Jani Nikula also disagree with the variadic-argument
>>> implementation approach. If we abandon that method, it means we will
>>> inevitably need to add some new macros. If mutable is not a good name,
>>> suggestions for better alternatives would be welcome; coming up with a
>>> suitable name is indeed rather tricky.
>>
>> I don't think you need to add a new macro for the specific use case that people want to modify the next element of the iteration.
>>
>> If I remember your numbers correctly that is a really corner case and keeping using the existing *_safe() macros for that sounds perfectly fine to me.
>
> IIRC currently you have a choice of either:
> define Item that can't be deleted
> list_for_each() The current item.
> list_for_each_safe() The next item.
> There is also likely to be code that updates the variables to allow
> for other scenarios.
>
> Note that if increase a reference count and release a lock then list_for_each()
> is likely safer than list_for_each_safe() :-)
>
> list.h has 9 variants of the 'safe' loop.
> The bloat of another 9 is getting excessive.
>
> It has to be said that this is one of my least favourite type of list...
Hi Christian König, David Laight, Jani Nikula, David Hildenbrand,
Andy Shevchenko, Alexei Starovoitov
For ease of discussion, I need to summarize the currently possible
approaches and briefly describe their respective pros and cons,
using the list_for_each_entry* interfaces as examples.
1. Add list_for_each_entry_mutable, while keeping list_for_each_entry
and list_for_each_entry_safe unchanged. list_for_each_entry_mutable
would be used specifically for safe deletion scenarios that do not
need to expose the temporary cursor externally. The code can refer to
the v1 version.
Pros: Does not depend on immediate per-subsystem adaptation and can be
merged directly.
Cons: Requires adding a whole set of mutable interfaces, which makes the
code somewhat redundant.
2. Directly optimize away the temporary cursor in list_for_each_entry_safe
and define it inside the loop instead, changing the interface from four
arguments to three.
Pros: Does not add redundant interfaces.
Cons: (1) Users need to manually update special cases that use the
traversal variable of list_for_each_entry_safe, the new
list_for_each_entry_safe would no longer apply there and would
need to be open-coded.
(2) Because the macro arguments changes, all list_for_each_entry_safe
callers would need to be modified and merged together, making it
difficult to merge such a large amount of code at once.
3. Use a variadic macro approach to optimize list_for_each_entry_safe,
so that it supports both three and four arguments.
Pros: (1) Does not add redundant interfaces.
(2) Does not depend on immediate per-subsystem adaptation and can
be merged directly.
Cons: (1) Increases compile time.
(2) Makes the interface harder for users to use.
4. Optimize list_for_each_entry by defining the temporary cursor internally,
making it compatible with the functionality of list_for_each_entry_safe.
The code can refer to the v2 version.
Pros: (1) Does not add redundant interfaces.
(2) The number of externally visible arguments of list_for_each_entry
remains unchanged, still three.
Cons: (1) list_for_each_entry and list_for_each_entry_safe would be merged
into one, and list_for_each_entry_safe would gradually be deprecated.
(2) Users need to manually update special cases that use the traversal
variable of list_for_each_entry, the new list_for_each_entry would no
longer apply there and would need to be open-coded. There are 15 such
cases in total.
5. Use a variadic macro approach to optimize list_for_each_entry, so that
it supports both three and four arguments.
Pros: (1) Does not add redundant interfaces.
(2) Does not depend on immediate per-subsystem adaptation and can be
merged directly.
Cons: (1) Increases compile time.
(2) list_for_each_entry and list_for_each_entry_safe would be merged
into one, and list_for_each_entry_safe would gradually be deprecated.
6. Make no changes, keep the current logic unchanged, and close the current
email discussion.
Which of the six solutions above do people prefer?
--
Thanks
Kaitao Cheng
^ permalink raw reply
page: | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox