Re: [PATCH v4 1/9] mm: aggregate workingset information into histograms
On Tue, Nov 26, 2024 at 8:22 PM Matthew Wilcox wrote: > > On Tue, Nov 26, 2024 at 06:57:20PM -0800, Yuanchu Xie wrote: > > diff --git a/mm/internal.h b/mm/internal.h > > index 64c2eb0b160e..bbd3c1501bac 100644 > > --- a/mm/internal.h > > +++ b/mm/internal.h > > @@ -470,9 +470,14 @@ extern unsigned long highest_memmap_pfn; > > /* > > * in mm/vmscan.c: > > */ > > +struct scan_control; > > +bool isolate_lru_page(struct page *page); > > Is this a mismerge? It doesn't exist any more. Yes this is a mismerge. I'll fix it in the next version.
Re: [PATCH v6 00/15] integrity: Introduce the Integrity Digest Cache
On Tue, Nov 19, 2024 at 11:49:07AM +0100, Roberto Sassu wrote: Hi Roberto, I hope the week is going well for you. > From: Roberto Sassu > > Integrity detection and protection has long been a desirable feature, to > reach a large user base and mitigate the risk of flaws in the software > and attacks. > > However, while solutions exist, they struggle to reach a large user base, > due to requiring higher than desired constraints on performance, > flexibility and configurability, that only security conscious people are > willing to accept. > > For example, IMA measurement requires the target platform to collect > integrity measurements, and to protect them with the TPM, which introduces > a noticeable overhead (up to 10x slower in a microbenchmark) on frequently > used system calls, like the open(). > > IMA Appraisal currently requires individual files to be signed and > verified, and Linux distributions to rebuild all packages to include file > signatures (this approach has been adopted from Fedora 39+). Like a TPM, > also signature verification introduces a significant overhead, especially > if it is used to check the integrity of many files. > > This is where the new Integrity Digest Cache comes into play, it offers > additional support for new and existing integrity solutions, to make > them faster and easier to deploy. > > The Integrity Digest Cache can help IMA to reduce the number of TPM > operations and to make them happen in a deterministic way. If IMA knows > that a file comes from a Linux distribution, it can measure files in a > different way: measure the list of digests coming from the distribution > (e.g. RPM package headers), and subsequently measure a file if it is not > found in that list. > > The performance improvement comes at the cost of IMA not reporting which > files from installed packages were accessed, and in which temporal > sequence. This approach might not be suitable for all use cases. > > The Integrity Digest Cache can also help IMA for appraisal. IMA can simply > lookup the calculated digest of an accessed file in the list of digests > extracted from package headers, after verifying the header signature. It is > sufficient to verify only one signature for all files in the package, as > opposed to verifying a signature for each file. Roberto, a big picture question for you, our apologies if we completely misunderstand your patch series. The performance benefit comes from the fact that the kernel doesn't have to read a file and calculate the cryptographic digest when the file is accessed. The 'trusted' digest value comes from a signed list of digests that a packaging entity provides and the kernel validates. So there is an integrity guarantee that the supplied digests were the same as when the package was built. Is there a guarantee implemented, that we missed, that the on-disk file actually has the digest value that was initially generated by the packaging entity when the file is accessed operationally? Secondly, and in a related issue, what happens in a container environment when a pathname is accessed that is actually a different file but with the same effective pathname as a file that is in the vendor validated digest list? Once again, apologies, if we completely misinterpret the issues involved. Have a good remainder of the week. As always, Dr. Greg The Quixote Project - Flailing at the Travails of Cybersecurity https://github.com/Quixote-Project
Re: [PATCH v4 0/9] mm: workingset reporting
+ da...@lists.linux.dev I haven't thoroughly read any version of this patch series due to my laziness, sorry. So I may saying something completely wrong. My apology in advance, and please correct me in the case. > On Tue, Nov 26, 2024 at 06:57:19PM -0800, Yuanchu Xie wrote: > > This patch series provides workingset reporting of user pages in > > lruvecs, of which coldness can be tracked by accessed bits and fd > > references. DAMON provides data access patterns of user pages. It is not exactly named as workingset but a superset of the information. Users can therefore get the workingset from DAMON-provided raw data. So I feel I have to ask if DAMON can be used for, or help at achieving the purpose of this patch series. Depending on the detailed definition of workingset, of course, the workingset we can get from DAMON might not be technically same to what this patch series aim to provide, and the difference could be somewhat that makes DAMON unable to be used or help here. But I cannot know if this is the case with only this cover letter. > > However, the concept of workingset applies generically to > > all types of memory, which could be kernel slab caches, discardable > > userspace caches (databases), or CXL.mem. Therefore, data sources might > > come from slab shrinkers, device drivers, or the userspace. > > Another interesting idea might be hugepage workingset, so that we can > > measure the proportion of hugepages backing cold memory. However, with > > architectures like arm, there may be too many hugepage sizes leading to > > a combinatorial explosion when exporting stats to the userspace. > > Nonetheless, the kernel should provide a set of workingset interfaces > > that is generic enough to accommodate the various use cases, and extensible > > to potential future use cases. This again sounds similar to what DAMON aims to provide, to me. DAMON is designed to be easy to extend for vairous use cases and internal mechanisms. Specifically, it separates access check mechanisms and core logic into different layers, and provides an interface to use for implementing extending DAMON with new mechanisms. DAMON's two access check mechanisms for virtual address spaces and the physical address space are made using the interface, indeed. Also there were RFC patch series extending DAMON for NUMA-specific and write-only access monitoring using NUMA hinting fault and soft-dirty PTEs as the internal mechanisms. My humble understanding of the major difference between DAMON and workingset reporting is the internal mechanism. Workingset reporting uses MGLRU as the access check mechanism, while current access check mechanisms for DAMON are using page table accessed bits checking as the major mechanism. I think DAMON can be extended to use MGLRU as its another internal access check mechanism, but I understand that there could be many things that I overseeing. Yuanchu, I think it would help me and other reviewers better understand this patch series if you could share that. And I will also be more than happy to help you and others better understanding what DAMON can do or not with the discussion. > > Doesn't DAMON already provide this information? > > CCing SJ. Thank you for adding me, Johannes :) [...] > It does provide more detailed insight into userspace memory behavior, > which could be helpful when trying to make sense of applications that > sit on a rich layer of libraries and complicated runtimes. But here a > comparison to DAMON would be helpful. 100% agree. Thanks, SJ [...]
Re: [PATCH v6 07/15] digest_cache: Allow registration of digest list parsers
On Wed, Nov 27, 2024 at 10:51:11AM +0100, Roberto Sassu wrote: > For eBPF programs we are also in a need for a better way to > measure/appraise them. I am confused now, I was under the impression this "Integrity Digest Cache" is just a special thing for LSMs, and so I was under the impression that kernel_read_file() lsm hook already would take care of eBPF programs. > Now, I'm trying to follow you on the additional kernel_read_file() > calls. I agree with you, if a parser tries to open again the file that > is being verified it would cause a deadlock in IMA (since the inode > mutex is already locked for verifying the original file). Just document this on the parser as a requirement. > > > Supporting kernel modules opened the road for new deadlocks, since one > > > can ask a digest list to verify a kernel module, but that digest list > > > requires the same kernel module. That is why the in-kernel mechanism is > > > 100% reliable, > > > > Are users of this infrastructure really in need of modules for these > > parsers? > > I planned to postpone this to later, and introduced two parsers built- > in (TLV and RPM). However, due to Linus's concern regarding the RPM > parser, I moved it out in a kernel module. OK this should be part of the commit log, ie that it is not desirable to have an rpm parser in-kernel for some users. Luis
Re: [PATCH] docs: media: document media multi-committers rules and process
On Wed, Nov 27, 2024 at 12:54:15PM +0100, Mauro Carvalho Chehab wrote: > Em Wed, 27 Nov 2024 10:39:48 +0100 Mauro Carvalho Chehab escreveu: > > > > This workflow doesn't apply to patch submitters who are not allowed to > > > send pull requests and who don't have direct commit access. I thought > > > these submitters are the main audience of this document. In that case, I > > > think moving the next section that explains the e-mail workflow before > > > the "Media development workflow" section (which should likely be renamed > > > to make it clear that it is about merging patches, not developing them) > > > would be best. The "Review Cadence" section could also be folded in > > > there, to give a full view of what a submitter can expect. > > > > > > This would also have the advantage of introducing the linuvtv.org > > > patchwork instance, which you reference above. Documents are more > > > readable when they introduce concepts first before using them. > > > > Will try to do such change at v2. > > Actually, both workflows (a) and (b) apply to the ones that can't > send pull requests or push at media-committers.git: > > --- > > a. Normal workflow: patches are handled by subsystem maintainers:: > > +--+ +-+ +---+ +---+ > +-+ > |e-mail|-->|patchwork|-->|pull |-->|maintainers merge > |-->|media.git| > +--+ +-+ |request| |in media-committers.git| > +-+ > +---+ +---+ > >For this workflow, pull requests can be generated by a committer, >a previous committer, subsystem maintainers or by a couple of trusted >long-time contributors. If you are not in such group, please don't submit >pull requests, as they will likely be ignored. > > b. Committers' workflow: patches are handled by media committers:: > > +--+ +-+ ++ +---+ > +-+ > |e-mail|-->|patchwork|-->|committers merge at > |-->|maintainers|-->|media.git| > +--+ +-+ |media-committers.git| |approval | > +-+ > ++ +---+ > > --- > > No matter who sent an e-mail, this will be picked by patchwork and either > be part of a PR or a MR, depending on who picked it. Today the "normal" workflow for contributors who don't send pull requests is that you or Hans will pick their patches from the list. That's why I mentioned that neither of the above workflows apply there. Now, if we consider that you and Hans will keep doing that for some patches, and merge them using the committers workflow (where you would handle both steps of merging in the shared tree and giving the maintainer approval), it's true that the normal workflow would be one of the two above. Looking at the pull requests sent to the list over the past twelve months, we have 32 Sakari Ailus 24 Hans Verkuil 22 Laurent Pinchart 21 Sebastian Fricke 7 Sean Young 7 Hans de Goede 4 Stanimir Varbanov 1 Shuah Khan I expect people in that list to get commit rights either from the very beginning or very soon after. The committer workflow (if we consider it as including how you and Hans will continue picking patches from the list) will be the new norm. how about flipping things and listing it as a), and then name b) the "Pull request workflow" instead of the "Normal workflow" ? I would even go as far as proposing documenting the pull request workflow as legacy. -- Regards, Laurent Pinchart
Re: [PATCH] docs: media: document media multi-committers rules and process
Em Wed, 27 Nov 2024 15:39:38 +0200 Laurent Pinchart escreveu: > On Wed, Nov 27, 2024 at 12:54:15PM +0100, Mauro Carvalho Chehab wrote: > > Em Wed, 27 Nov 2024 10:39:48 +0100 Mauro Carvalho Chehab escreveu: > > > > > > This workflow doesn't apply to patch submitters who are not allowed to > > > > send pull requests and who don't have direct commit access. I thought > > > > these submitters are the main audience of this document. In that case, I > > > > think moving the next section that explains the e-mail workflow before > > > > the "Media development workflow" section (which should likely be renamed > > > > to make it clear that it is about merging patches, not developing them) > > > > would be best. The "Review Cadence" section could also be folded in > > > > there, to give a full view of what a submitter can expect. > > > > > > > > This would also have the advantage of introducing the linuvtv.org > > > > patchwork instance, which you reference above. Documents are more > > > > readable when they introduce concepts first before using them. > > > > > > Will try to do such change at v2. > > > > Actually, both workflows (a) and (b) apply to the ones that can't > > send pull requests or push at media-committers.git: > > > > --- > > > > a. Normal workflow: patches are handled by subsystem maintainers:: > > > > +--+ +-+ +---+ +---+ > > +-+ > > |e-mail|-->|patchwork|-->|pull |-->|maintainers merge > > |-->|media.git| > > +--+ +-+ |request| |in media-committers.git| > > +-+ > > +---+ +---+ > > > >For this workflow, pull requests can be generated by a committer, > >a previous committer, subsystem maintainers or by a couple of trusted > >long-time contributors. If you are not in such group, please don't submit > >pull requests, as they will likely be ignored. > > > > b. Committers' workflow: patches are handled by media committers:: > > > > +--+ +-+ ++ +---+ > > +-+ > > |e-mail|-->|patchwork|-->|committers merge at > > |-->|maintainers|-->|media.git| > > +--+ +-+ |media-committers.git| |approval | > > +-+ > > ++ +---+ > > > > --- > > > > No matter who sent an e-mail, this will be picked by patchwork and either > > be part of a PR or a MR, depending on who picked it. > > Today the "normal" workflow for contributors who don't send pull > requests is that you or Hans will pick their patches from the list. True, but we've been following process (b) since the last merge window: we are generating merges at the media-committers.git. As we're maintainers, the "maintainers approval" step is also handled by us, by the one that submitted the MR, after checking the media-ci results. > That's why I mentioned that neither of the above workflows apply there. > Now, if we consider that you and Hans will keep doing that for some > patches, and merge them using the committers workflow (where you would > handle both steps of merging in the shared tree and giving the > maintainer approval), it's true that the normal workflow would be one of > the two above. Yes, that's the case. > Looking at the pull requests sent to the list over the past twelve > months, we have > > 32 Sakari Ailus > 24 Hans Verkuil > 22 Laurent Pinchart > 21 Sebastian Fricke > 7 Sean Young > 7 Hans de Goede > 4 Stanimir Varbanov > 1 Shuah Khan > > I expect people in that list to get commit rights either from the very > beginning or very soon after. The committer workflow (if we consider it > as including how you and Hans will continue picking patches from the > list) will be the new norm. how about flipping things and listing it as > a), and then name b) the "Pull request workflow" instead of the "Normal > workflow" ? I would even go as far as proposing documenting the pull > request workflow as legacy. Renaming from Normal work flow to Pull request workflow makes sense. The pull request workflow won't be legacy. Even with major contributors using the new workflow for "normal work", pull requests will still be generated for API changes. Regards, Mauro
Re: [PATCH] docs: media: document media multi-committers rules and process
Jumping in the middle here with some clarifications. On Wed, 27 Nov 2024 at 12:19, Laurent Pinchart wrote: > On Wed, Nov 27, 2024 at 10:39:48AM +0100, Mauro Carvalho Chehab wrote: > > It is somewhat similar to drm-intel and drm-xe, where reviews are part > > of the acceptance criteria to become committers. > > Those are corporate trees, so it's easier to set such rules. Imo it's the other way round, because it's corporate you need stricter rules and spell them all out clearly - managers just love to apply pressure on their engineers too much otherwise "because it's our own tree". Totally forgetting that it's still part of the overall upstream, and that they don't own upstream. So that's why the corporate trees are stricter than drm-misc, but the goals are still exactly the same: - peer review is required in a tit-for-tat market, but not more. - committers push their own stuff, that's all. Senior committers often also push other people's work, like for smaller work they just reviewed or of people they mentor, but it's not required at all. - maintainership duties, like sending around pr, making sure patches dont get lost and things like that, is separate from commit rights. In my opinion, if you tie commit rights to maintainership you're doing something else than drm and I'd more call it a group maintainership model, not a commit rights model for landing patches. Anyway just figured I'll clarify what we do over at drm. I haven't looked at all the details of this proposal here and the already lengthy discussion, plus it's really not on me to chime in since I'm not involved. Cheers, Sima -- Simona Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH v4 24/26] arch_numa: switch over to numa_memblks
Hi Mike,
Sorry for reviving a rather old thread.
On Wed, 07 Aug 2024 07:41:08 +0100,
Mike Rapoport wrote:
>
> From: "Mike Rapoport (Microsoft)"
>
> Until now arch_numa was directly translating firmware NUMA information
> to memblock.
>
> Using numa_memblks as an intermediate step has a few advantages:
> * alignment with more battle tested x86 implementation
> * availability of NUMA emulation
> * maintaining node information for not yet populated memory
>
> Adjust a few places in numa_memblks to compile with 32-bit phys_addr_t
> and replace current functionality related to numa_add_memblk() and
> __node_distance() in arch_numa with the implementation based on
> numa_memblks and add functions required by numa_emulation.
>
> Signed-off-by: Mike Rapoport (Microsoft)
> Tested-by: Zi Yan # for x86_64 and arm64
> Reviewed-by: Jonathan Cameron
> Tested-by: Jonathan Cameron [arm64 + CXL via
> QEMU]
> Acked-by: Dan Williams
> Acked-by: David Hildenbrand
> ---
> drivers/base/Kconfig | 1 +
> drivers/base/arch_numa.c | 201 +++--
> include/asm-generic/numa.h | 6 +-
> mm/numa_memblks.c | 17 ++--
> 4 files changed, 75 insertions(+), 150 deletions(-)
>
[...]
> static int __init numa_register_nodes(void)
> {
> int nid;
> - struct memblock_region *mblk;
> -
> - /* Check that valid nid is set to memblks */
> - for_each_mem_region(mblk) {
> - int mblk_nid = memblock_get_region_node(mblk);
> - phys_addr_t start = mblk->base;
> - phys_addr_t end = mblk->base + mblk->size - 1;
> -
> - if (mblk_nid == NUMA_NO_NODE || mblk_nid >= MAX_NUMNODES) {
> - pr_warn("Warning: invalid memblk node %d [mem
> %pap-%pap]\n",
> - mblk_nid, &start, &end);
> - return -EINVAL;
> - }
> - }
>
This hunk has the unfortunate side effect of killing my ThunderX
extremely early at boot time, as this sorry excuse for a machine
really relies on the kernel recognising that whatever NUMA information
the FW offers is BS.
Reverting this hunk restores happiness (sort of).
FWIW, I've posted a patch with such revert at [1].
Thanks,
M.
[1] https://lore.kernel.org/r/20241127193000.3702637-1-...@kernel.org
--
Without deviation from the norm, progress is not possible.
Re: [PATCH v6 00/15] integrity: Introduce the Integrity Digest Cache
On Wed, 2024-11-27 at 11:30 -0600, Dr. Greg wrote: > On Tue, Nov 19, 2024 at 11:49:07AM +0100, Roberto Sassu wrote: > > Hi Roberto, I hope the week is going well for you. > > > From: Roberto Sassu > > > > Integrity detection and protection has long been a desirable feature, to > > reach a large user base and mitigate the risk of flaws in the software > > and attacks. > > > > However, while solutions exist, they struggle to reach a large user base, > > due to requiring higher than desired constraints on performance, > > flexibility and configurability, that only security conscious people are > > willing to accept. > > > > For example, IMA measurement requires the target platform to collect > > integrity measurements, and to protect them with the TPM, which introduces > > a noticeable overhead (up to 10x slower in a microbenchmark) on frequently > > used system calls, like the open(). > > > > IMA Appraisal currently requires individual files to be signed and > > verified, and Linux distributions to rebuild all packages to include file > > signatures (this approach has been adopted from Fedora 39+). Like a TPM, > > also signature verification introduces a significant overhead, especially > > if it is used to check the integrity of many files. > > > > This is where the new Integrity Digest Cache comes into play, it offers > > additional support for new and existing integrity solutions, to make > > them faster and easier to deploy. > > > > The Integrity Digest Cache can help IMA to reduce the number of TPM > > operations and to make them happen in a deterministic way. If IMA knows > > that a file comes from a Linux distribution, it can measure files in a > > different way: measure the list of digests coming from the distribution > > (e.g. RPM package headers), and subsequently measure a file if it is not > > found in that list. > > > > The performance improvement comes at the cost of IMA not reporting which > > files from installed packages were accessed, and in which temporal > > sequence. This approach might not be suitable for all use cases. > > > > The Integrity Digest Cache can also help IMA for appraisal. IMA can simply > > lookup the calculated digest of an accessed file in the list of digests > > extracted from package headers, after verifying the header signature. It is > > sufficient to verify only one signature for all files in the package, as > > opposed to verifying a signature for each file. > > Roberto, a big picture question for you, our apologies if we > completely misunderstand your patch series. Hi Greg no need to apologise, happy to answer your questions. > The performance benefit comes from the fact that the kernel doesn't > have to read a file and calculate the cryptographic digest when the > file is accessed. The 'trusted' digest value comes from a signed list > of digests that a packaging entity provides and the kernel validates. > So there is an integrity guarantee that the supplied digests were the > same as when the package was built. The performance benefit (for appraisal with my benchmark: 65% with sequential file access and 43% with parallel file access) comes from verifying the ECDSA signature of 303 digest lists, as opposed to the ECDSA signature of 12312 files. The additional performance boost due to switching from file data digest to fsverity digests is on top of that. > Is there a guarantee implemented, that we missed, that the on-disk > file actually has the digest value that was initially generated by the > packaging entity when the file is accessed operationally? Yes, the guarantee is provided by IMA by measuring the actual file digest and searching it in a digest cache. The integration in IMA of the Integrity Digest Cache is done in a separate patch set: https://lore.kernel.org/linux-security-module/20241119110103.2780453-1-roberto.sa...@huaweicloud.com/ The integrity evaluation result is invalidated when the file is modified, or when the digest list used to verify the file is modified too. For fsverity, the guarantee similarly comes from searching the fsverity digest in a digest cache, but as opposed of IMA the integrity evaluation result does not need to be invalidated for a file write, since fsverity-protected files are accessible only in read-only mode. However, the result still needs to be invalidated if the digest list changes. > Secondly, and in a related issue, what happens in a container > environment when a pathname is accessed that is actually a different > file but with the same effective pathname as a file that is in the > vendor validated digest list? At the moment nothing, only the file data are evaluated. Currently, the Integrity Digest Cache does not store the pathnames associated to a digest. It can be done as an extension, if desired, and the pathnames can be compared. Roberto > Once again, apologies, if we completely misinterpret the issues > involved. > > Have a good remainder of the week. > > As always, > Dr. Greg > > The
Re: [PATCH] docs: media: document media multi-committers rules and process
On Wed, Nov 27, 2024 at 04:09:23PM +0100, Mauro Carvalho Chehab wrote: > Em Wed, 27 Nov 2024 15:39:38 +0200 Laurent Pinchart escreveu: > > On Wed, Nov 27, 2024 at 12:54:15PM +0100, Mauro Carvalho Chehab wrote: > > > Em Wed, 27 Nov 2024 10:39:48 +0100 Mauro Carvalho Chehab escreveu: > > > > > > > > This workflow doesn't apply to patch submitters who are not allowed to > > > > > send pull requests and who don't have direct commit access. I thought > > > > > these submitters are the main audience of this document. In that > > > > > case, I > > > > > think moving the next section that explains the e-mail workflow before > > > > > the "Media development workflow" section (which should likely be > > > > > renamed > > > > > to make it clear that it is about merging patches, not developing > > > > > them) > > > > > would be best. The "Review Cadence" section could also be folded in > > > > > there, to give a full view of what a submitter can expect. > > > > > > > > > > This would also have the advantage of introducing the linuvtv.org > > > > > patchwork instance, which you reference above. Documents are more > > > > > readable when they introduce concepts first before using them. > > > > > > > > Will try to do such change at v2. > > > > > > Actually, both workflows (a) and (b) apply to the ones that can't > > > send pull requests or push at media-committers.git: > > > > > > --- > > > > > > a. Normal workflow: patches are handled by subsystem maintainers:: > > > > > > +--+ +-+ +---+ +---+ > > > +-+ > > > |e-mail|-->|patchwork|-->|pull |-->|maintainers merge > > > |-->|media.git| > > > +--+ +-+ |request| |in media-committers.git| > > > +-+ > > > +---+ +---+ > > > > > >For this workflow, pull requests can be generated by a committer, > > >a previous committer, subsystem maintainers or by a couple of trusted > > >long-time contributors. If you are not in such group, please don't > > > submit > > >pull requests, as they will likely be ignored. > > > > > > b. Committers' workflow: patches are handled by media committers:: > > > > > > +--+ +-+ ++ +---+ > > > +-+ > > > |e-mail|-->|patchwork|-->|committers merge at > > > |-->|maintainers|-->|media.git| > > > +--+ +-+ |media-committers.git| |approval | > > > +-+ > > > ++ +---+ > > > > > > --- > > > > > > No matter who sent an e-mail, this will be picked by patchwork and either > > > be part of a PR or a MR, depending on who picked it. > > > > Today the "normal" workflow for contributors who don't send pull > > requests is that you or Hans will pick their patches from the list. > > True, but we've been following process (b) since the last merge window: we > are generating merges at the media-committers.git. As we're maintainers, > the "maintainers approval" step is also handled by us, by the one that > submitted the MR, after checking the media-ci results. > > > That's why I mentioned that neither of the above workflows apply there. > > Now, if we consider that you and Hans will keep doing that for some > > patches, and merge them using the committers workflow (where you would > > handle both steps of merging in the shared tree and giving the > > maintainer approval), it's true that the normal workflow would be one of > > the two above. > > Yes, that's the case. > > > Looking at the pull requests sent to the list over the past twelve > > months, we have > > > > 32 Sakari Ailus > > 24 Hans Verkuil > > 22 Laurent Pinchart > > 21 Sebastian Fricke > > 7 Sean Young > > 7 Hans de Goede > > 4 Stanimir Varbanov > > 1 Shuah Khan > > > > I expect people in that list to get commit rights either from the very > > beginning or very soon after. The committer workflow (if we consider it > > as including how you and Hans will continue picking patches from the > > list) will be the new norm. how about flipping things and listing it as > > a), and then name b) the "Pull request workflow" instead of the "Normal > > workflow" ? I would even go as far as proposing documenting the pull > > request workflow as legacy. > > Renaming from Normal work flow to Pull request workflow makes sense. > > The pull request workflow won't be legacy. Even with major contributors > using the new workflow for "normal work", pull requests will still be > generated for API changes. OK, let's not mark it as deprecated, we can just rename it to "Pull request workflow". I'd still prefer to list it as b) but won't make that a casus belli. -- Regards, Laurent Pinchart
Re: [PATCH v4 0/9] mm: workingset reporting
On Wed, Nov 27, 2024 at 12:26 AM Johannes Weiner wrote: > > On Tue, Nov 26, 2024 at 06:57:19PM -0800, Yuanchu Xie wrote: > > This patch series provides workingset reporting of user pages in > > lruvecs, of which coldness can be tracked by accessed bits and fd > > references. However, the concept of workingset applies generically to > > all types of memory, which could be kernel slab caches, discardable > > userspace caches (databases), or CXL.mem. Therefore, data sources might > > come from slab shrinkers, device drivers, or the userspace. > > Another interesting idea might be hugepage workingset, so that we can > > measure the proportion of hugepages backing cold memory. However, with > > architectures like arm, there may be too many hugepage sizes leading to > > a combinatorial explosion when exporting stats to the userspace. > > Nonetheless, the kernel should provide a set of workingset interfaces > > that is generic enough to accommodate the various use cases, and extensible > > to potential future use cases. > > Doesn't DAMON already provide this information? Yuanchu might be able to answer this question a lot better than I do, since he studied DAMON and tried to leverage it in our fleet. My impression is that there are some fundamental differences in access detection and accounting mechanisms between the two, i.e., sampling vs scanning-based detection and non-lruvec vs lruvec-based accounting. > CCing SJ. > > > Use cases > > == > > Job scheduling > > On overcommitted hosts, workingset information improves efficiency and > > reliability by allowing the job scheduler to have better stats on the > > exact memory requirements of each job. This can manifest in efficiency by > > landing more jobs on the same host or NUMA node. On the other hand, the > > job scheduler can also ensure each node has a sufficient amount of memory > > and does not enter direct reclaim or the kernel OOM path. With workingset > > information and job priority, the userspace OOM killing or proactive > > reclaim policy can kick in before the system is under memory pressure. > > If the job shape is very different from the machine shape, knowing the > > workingset per-node can also help inform page allocation policies. > > > > Proactive reclaim > > Workingset information allows the a container manager to proactively > > reclaim memory while not impacting a job's performance. While PSI may > > provide a reactive measure of when a proactive reclaim has reclaimed too > > much, workingset reporting allows the policy to be more accurate and > > flexible. > > I'm not sure about more accurate. Agreed. This is a (very) poor argument, unless there are facts to back this up. > Access frequency is only half the picture. Whether you need to keep > memory with a given frequency resident depends on the speed of the > backing device. Along a similar line, we also need to consider use cases that don't involve backing storage, e.g., far memory (remote node). More details below. > There is memory compression; there is swap on flash; swap on crappy > flash; swapfiles that share IOPS with co-located filesystems. There is > zswap+writeback, where avg refault speed can vary dramatically. > > You can of course offload much more to a fast zswap backend than to a > swapfile on a struggling flashdrive, with comparable app performance. > > So I think you'd be hard pressed to achieve a high level of accuracy > in the usecases you list without taking the (often highly dynamic) > cost of paging / memory transfer into account. > > There is a more detailed discussion of this in a paper we wrote on > proactive reclaim/offloading - in 2.5 Hardware Heterogeneity: > > https://www.cs.cmu.edu/~dskarlat/publications/tmo_asplos22.pdf > > > Ballooning (similar to proactive reclaim) > > The last patch of the series extends the virtio-balloon device to report > > the guest workingset. > > Balloon policies benefit from workingset to more precisely determine the > > size of the memory balloon. On end-user devices where memory is scarce and > > overcommitted, the balloon sizing in multiple VMs running on the same > > device can be orchestrated with workingset reports from each one. > > On the server side, workingset reporting allows the balloon controller to > > inflate the balloon without causing too much file cache to be reclaimed in > > the guest. > > > > Promotion/Demotion > > If different mechanisms are used for promition and demotion, workingset > > information can help connect the two and avoid pages being migrated back > > and forth. > > For example, given a promotion hot page threshold defined in reaccess > > distance of N seconds (promote pages accessed more often than every N > > seconds). The threshold N should be set so that ~80% (e.g.) of pages on > > the fast memory node passes the threshold. This calculation can be done > > with workingset reports. > > To be directly useful for promotion policies, the workingset report > > interfaces need to be ext
Re: [PATCH v4 9/9] virtio-balloon: add workingset reporting
On Tue, Nov 26, 2024 at 7:00 PM Yuanchu Xie wrote:
[...]
> diff --git a/include/linux/workingset_report.h
> b/include/linux/workingset_report.h
> index f6bbde2a04c3..1074b89035e9 100644
> --- a/include/linux/workingset_report.h
> +++ b/include/linux/workingset_report.h
[...]
> diff --git a/include/uapi/linux/virtio_balloon.h
> b/include/uapi/linux/virtio_balloon.h
> index ee35a372805d..668eaa39c85b 100644
> --- a/include/uapi/linux/virtio_balloon.h
> +++ b/include/uapi/linux/virtio_balloon.h
> @@ -25,6 +25,7 @@
> * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
> * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> * SUCH DAMAGE. */
> +#include "linux/workingset_report.h"
> #include
> #include
> #include
This seems to be including a non-uapi header
(include/linux/workingset_report.h) from a uapi header
(include/uapi/linux/virtio_balloon.h), which won't compile outside the
kernel. Does anything in the uapi actually need declarations from
workingset_report.h?
> @@ -37,6 +38,7 @@
> #define VIRTIO_BALLOON_F_FREE_PAGE_HINT3 /* VQ to report free pages
> */
> #define VIRTIO_BALLOON_F_PAGE_POISON 4 /* Guest is using page poisoning */
> #define VIRTIO_BALLOON_F_REPORTING 5 /* Page reporting virtqueue */
> +#define VIRTIO_BALLOON_F_WS_REPORTING 6 /* Working Set Size reporting */
>
> /* Size of a PFN in the balloon interface. */
> #define VIRTIO_BALLOON_PFN_SHIFT 12
> @@ -128,4 +130,32 @@ struct virtio_balloon_stat {
> __virtio64 val;
> } __attribute__((packed));
>
> +/* Operations from the device */
> +#define VIRTIO_BALLOON_WS_OP_REQUEST 1
> +#define VIRTIO_BALLOON_WS_OP_CONFIG 2
> +
> +struct virtio_balloon_working_set_notify {
> + /* REQUEST or CONFIG */
> + __le16 op;
> + __le16 node_id;
> + /* the following fields valid iff op=CONFIG */
> + __le32 report_threshold;
> + __le32 refresh_interval;
> + __le32 idle_age[WORKINGSET_REPORT_MAX_NR_BINS];
> +};
> +
> +struct virtio_balloon_working_set_report_bin {
> + __le64 idle_age;
> + /* bytes in this bucket for anon and file */
> + __le64 anon_bytes;
> + __le64 file_bytes;
> +};
> +
> +struct virtio_balloon_working_set_report {
> + __le32 error;
> + __le32 node_id;
> + struct virtio_balloon_working_set_report_bin
> + bins[WORKINGSET_REPORT_MAX_NR_BINS];
> +};
> +
> #endif /* _LINUX_VIRTIO_BALLOON_H */
Have the spec changes been discussed in the virtio TC?
Thanks,
-- Daniel
Re: [PATCH v4 9/9] virtio-balloon: add workingset reporting
On Wed, Nov 27, 2024 at 3:15 PM Daniel Verkamp wrote:
> > * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY
> > WAY
> > * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> > * SUCH DAMAGE. */
> > +#include "linux/workingset_report.h"
> > #include
> > #include
> > #include
>
> This seems to be including a non-uapi header
> (include/linux/workingset_report.h) from a uapi header
> (include/uapi/linux/virtio_balloon.h), which won't compile outside the
> kernel. Does anything in the uapi actually need declarations from
> workingset_report.h?
Good point. I should move the relevant constants over.
> > +
> > +struct virtio_balloon_working_set_notify {
> > + /* REQUEST or CONFIG */
> > + __le16 op;
> > + __le16 node_id;
> > + /* the following fields valid iff op=CONFIG */
> > + __le32 report_threshold;
> > + __le32 refresh_interval;
> > + __le32 idle_age[WORKINGSET_REPORT_MAX_NR_BINS];
> > +};
> > +
> > +struct virtio_balloon_working_set_report_bin {
> > + __le64 idle_age;
> > + /* bytes in this bucket for anon and file */
> > + __le64 anon_bytes;
> > + __le64 file_bytes;
> > +};
> > +
> > +struct virtio_balloon_working_set_report {
> > + __le32 error;
> > + __le32 node_id;
> > + struct virtio_balloon_working_set_report_bin
> > + bins[WORKINGSET_REPORT_MAX_NR_BINS];
> > +};
> > +
> > #endif /* _LINUX_VIRTIO_BALLOON_H */
>
> Have the spec changes been discussed in the virtio TC?
They have not. Thanks for bringing this up. I'll post in the VIRTIO TC.
Thanks,
Yuanchu
Re: [PATCH] docs: media: document media multi-committers rules and process
Em Tue, 26 Nov 2024 17:19:30 +0200 Laurent Pinchart escreveu: > Hi Mauro and Hans, > > On Mon, Nov 25, 2024 at 02:28:58PM +0100, Mauro Carvalho Chehab wrote: > > As the media subsystem will experiment with a multi-committers model, > > update the Maintainer's entry profile to the new rules, and add a file > > documenting the process to become a committer and to maintain such > > rights. > > > > Signed-off-by: Mauro Carvalho Chehab > > Signed-off-by: Hans Verkuil > > --- > > Documentation/driver-api/media/index.rst | 1 + > > .../media/maintainer-entry-profile.rst| 193 ++ > > .../driver-api/media/media-committer.rst | 252 ++ > > .../process/maintainer-pgp-guide.rst | 2 + > > 4 files changed, 398 insertions(+), 50 deletions(-) > > create mode 100644 Documentation/driver-api/media/media-committer.rst > > > > diff --git a/Documentation/driver-api/media/index.rst > > b/Documentation/driver-api/media/index.rst > > index d5593182a3f9..d0c725fcbc67 100644 > > --- a/Documentation/driver-api/media/index.rst > > +++ b/Documentation/driver-api/media/index.rst > > @@ -26,6 +26,7 @@ Documentation/userspace-api/media/index.rst > > :numbered: > > > > maintainer-entry-profile > > +media-committer > > > > v4l2-core > > dtv-core > > diff --git a/Documentation/driver-api/media/maintainer-entry-profile.rst > > b/Documentation/driver-api/media/maintainer-entry-profile.rst > > index ffc712a5f632..90c6c0d9cf17 100644 > > --- a/Documentation/driver-api/media/maintainer-entry-profile.rst > > +++ b/Documentation/driver-api/media/maintainer-entry-profile.rst > > @@ -27,19 +27,128 @@ It covers, mainly, the contents of those directories: > > Both media userspace and Kernel APIs are documented and the documentation > > must be kept in sync with the API changes. It means that all patches that > > add new features to the subsystem must also bring changes to the > > -corresponding API files. > > +corresponding API documentation files. > > I would have split this kind of small changes to a separate patch to > make reviews easier, but that's not a big deal. > > > > > -Due to the size and wide scope of the media subsystem, media's > > -maintainership model is to have sub-maintainers that have a broad > > -knowledge of a specific aspect of the subsystem. It is the sub-maintainers' > > -task to review the patches, providing feedback to users if the patches are > > +Due to the size and wide scope of the media subsystem, the media's > > +maintainership model is to have committers that have a broad knowledge of > > +a specific aspect of the subsystem. It is the committers' task to > > +review the patches, providing feedback to users if the patches are > > following the subsystem rules and are properly using the media kernel and > > userspace APIs. > > This sounds really like a maintainer definition. I won't bikeshed too > much on the wording though, we will always be able to adjust it later to > reflect the reality of the situation as it evolves. I do like the > removal of the "sub-maintainer" term though, as I've always found it > demeaning. The main goal here was to replace sub-maintainers by committers. Other changes can go later on, or if you have a better way to define the paper of committers, be welcomed to propose it. > > -Patches for the media subsystem must be sent to the media mailing list > > -at linux-me...@vger.kernel.org as plain text only e-mail. Emails with > > -HTML will be automatically rejected by the mail server. It could be wise > > -to also copy the sub-maintainer(s). > > +Media committers > > + > > + > > +In the media subsystem, there are experienced developers that can commit > > s/that/who/ > s/commit/push/ to standardize the vocabulary (below you use "upload" to > mean the same thing) > > > +patches directly on a development tree. These developers are called > > s/on a/to the/ I won't comment here any below about trivial changes like that: I'll just incorporate at v2. I'll focus my reply on your the comments about the text contents itself. > > > +Media committers and are divided into the following categories: > > + > > +- Committers: responsible for one or more drivers within the media > > subsystem. > > + They can upload changes to the tree that do not affect the core or ABI. > > s/upload/push/ > > > + > > +- Core committers: responsible for part of the media core. They are > > typically > > + responsible for one or more drivers within the media subsystem, but, > > besides > > + that, they can also merge patches that change the code common to multiple > > + drivers, including the kernel internal API/ABI. > > I would write "API" only here. Neither the kernel internal API nor its > internal ABI are stable, and given that lack of stability, the ABI > concept doesn't really apply within the kernel. It does for distros, but this is a separate matter ;-) From my side, I
Re: [PATCH v6 07/15] digest_cache: Allow registration of digest list parsers
On Tue, 2024-11-26 at 11:04 -0800, Luis Chamberlain wrote: > On Tue, Nov 26, 2024 at 11:25:07AM +0100, Roberto Sassu wrote: > > On Mon, 2024-11-25 at 15:53 -0800, Luis Chamberlain wrote: > > > > Firmware, eBPF programs and so on are supposed > > Keyword: "supposed". It depends if they are in a policy. They can also be verified with other methods, such as file signatures. For eBPF programs we are also in a need for a better way to measure/appraise them. > > As far as the LSM infrastructure is concerned, I'm not adding new LSM > > hooks, nor extending/modifying the existing ones. The operations the > > Integrity Digest Cache is doing match the usage expectation by LSM (net > > denying access, as discussed with Paul Moore). > > If modules are the only proven exception to your security model you are > not making the case for it clearly. The Integrity Digest Cache is not implementing any security model, this is demanded to other LSMs which might decide to use the Integrity Digest Cache based on a policy. If we want to be super picky, the ksys_finit_module() helper is not calling security_kernel_module_request(), which is done when using request_module(). On the other hand, ksys_finit_module() is not triggering user space, as the description of the function states (anyway, apologies for not bringing up this earlier). Net this, and we can discuss if it is more appropriate to call the LSM hook, the helper does not introduce any exception since security_file_open() is called when the kernel opens the file descriptor, and security_kernel_read_file() and security_kernel_post_read_file() are called in the same way regardless if it is user space doing insmod or the kernel calling ksys_finit_module(). The only exception is that the Integrity Digest Cache is unable to verify the kernel modules containing the parsers, but I believe this is fine because they are verified with their appended signature. If there are any other concerns I'm missing, please let me know. > > The Integrity Digest Cache is supposed to be used as a supporting tool > > for other LSMs to do regular access control based on file data and > > metadata integrity. In doing that, it still needs the LSM > > infrastructure to notify about filesystem changes, and to store > > additional information in the inode and file descriptor security blobs. > > > > The kernel_post_read_file LSM hook should be implemented by another LSM > > to verify the integrity of a digest list, when the Integrity Digest > > Cache calls kernel_read_file() to read that digest list. > > If LSM folks *do* agree that this work is *suplementing* LSMS then sure, > it was not clear from the commit logs. But then you need to ensure the > parsers are special snowflakes which won't ever incur other additional > kernel_read_file() calls. The Integrity Digest Cache was originally called digest_cache LSM, but was renamed due to Paul's concern that it is not a proper LSM enforcing a security model. If you are interested, I gave a talk at LSS NA 2024: https://www.youtube.com/watch?v=aNwlKYSksg8 Given that the Integrity Digest Cache could not be standalone and use the LSM infrastructure facilities, it is going to be directly integrated in IMA, although it is not strictly necessary. I planned to support IPE and BPF LSM as other users. Uhm, let me clarify your last sentence a bit. Let's assume that IMA is asked to verify a parser, when invoked through the kernel_post_read_file hook. IMA is not handling the exception, and is calling digest_cache_get() as usual. Normally, this would succeed, but because digest_cache_get() realizes that the file descriptor passed as argument is marked (i.e. it was opened by the Integrity Digest Cache itself), it returns NULL. That means that IMA falls back on another verification method, which is verifying the appended signature. The most important design principle that I introduced is that users of the Integrity Digest Cache don't need to be aware of any exception, everything is handled by the Integrity Digest Cache itself. The same occurs when a kernel read occurs with file ID READING_DIGEST_LIST (introduced in this patch set). Yes, I forbid specifying an IMA policy which requires the Integrity Digest Cache to verify digest lists, but due to the need of handling kernel modules I decided to handle the exceptions in the Integrity Digest Cache itself (this is why now I'm passing a file descriptor to digest_cache_get() instead of a dentry). Now, I'm trying to follow you on the additional kernel_read_file() calls. I agree with you, if a parser tries to open again the file that is being verified it would cause a deadlock in IMA (since the inode mutex is already locked for verifying the original file). In the Integrity Digest Cache itself, this is not going to happen, since the file being verified with a digest cache is known and an internal open of the same file fails. If it is really necessary, we can pass the information to the parsers so that they
Re: [PATCH v11 00/14] riscv: Add support for xtheadvector
Tested-by: Yangyu Chen I applied and tested it on Nezha hardware, and it works fine. Cheers, Yangyu Chen On 11/14/24 10:21, Charlie Jenkins wrote: xtheadvector is a custom extension that is based upon riscv vector version 0.7.1 [1]. All of the vector routines have been modified to support this alternative vector version based upon whether xtheadvector was determined to be supported at boot. vlenb is not supported on the existing xtheadvector hardware, so a devicetree property thead,vlenb is added to provide the vlenb to Linux. There is a new hwprobe key RISCV_HWPROBE_KEY_VENDOR_EXT_THEAD_0 that is used to request which thead vendor extensions are supported on the current platform. This allows future vendors to allocate hwprobe keys for their vendor. Support for xtheadvector is also added to the vector kselftests. Signed-off-by: Charlie Jenkins [1] https://github.com/T-head-Semi/thead-extension-spec/blob/95358cb2cca9489361c61d335e03d3134b14133f/xtheadvector.adoc --- This series is a continuation of a different series that was fragmented into two other series in an attempt to get part of it merged in the 6.10 merge window. The split-off series did not get merged due to a NAK on the series that added the generic riscv,vlenb devicetree entry. This series has converted riscv,vlenb to thead,vlenb to remedy this issue. The original series is titled "riscv: Support vendor extensions and xtheadvector" [3]. I have tested this with an Allwinner Nezha board. I used SkiffOS [1] to manage building the image, but upgraded the U-Boot version to Samuel Holland's more up-to-date version [2] and changed out the device tree used by U-Boot with the device trees that are present in upstream linux and this series. Thank you Samuel for all of the work you did to make this task possible. [1] https://github.com/skiffos/SkiffOS/tree/master/configs/allwinner/nezha [2] https://github.com/smaeul/u-boot/commit/2e89b706f5c956a70c989cd31665f1429e9a0b48 [3] https://lore.kernel.org/all/20240503-dev-charlie-support_thead_vector_6_9-v6-0-cb7624e65...@rivosinc.com/ [4] https://lore.kernel.org/lkml/20240719-support_vendor_extensions-v3-4-0af7587bb...@rivosinc.com/T/ --- Changes in v11: - Fix an issue where the mitigation was not being properly skipped when requested - Fix vstate_discard issue - Fix issue when -1 was passed into __riscv_isa_vendor_extension_available() - Remove some artifacts from being placed in the test directory - Link to v10: https://lore.kernel.org/r/20240911-xtheadvector-v10-0-8d3930091...@rivosinc.com Changes in v10: - In DT probing disable vector with new function to clear vendor extension bits for xtheadvector - Add ghostwrite mitigations for c9xx CPUs. This disables xtheadvector unless mitigations=off is set as a kernel boot arg - Link to v9: https://lore.kernel.org/r/20240806-xtheadvector-v9-0-62a56d2da...@rivosinc.com Changes in v9: - Rebase onto palmer's for-next - Fix sparse error in arch/riscv/kernel/vendor_extensions/thead.c - Fix maybe-uninitialized warning in arch/riscv/include/asm/vendor_extensions/vendor_hwprobe.h - Wrap some long lines - Link to v8: https://lore.kernel.org/r/20240724-xtheadvector-v8-0-cf043168e...@rivosinc.com Changes in v8: - Rebase onto palmer's for-next - Link to v7: https://lore.kernel.org/r/20240724-xtheadvector-v7-0-b741910ad...@rivosinc.com Changes in v7: - Add defs for has_xtheadvector_no_alternatives() and has_xtheadvector() when vector disabled. (Palmer) - Link to v6: https://lore.kernel.org/r/20240722-xtheadvector-v6-0-c9af0130f...@rivosinc.com Changes in v6: - Fix return type of is_vector_supported()/is_xthead_supported() to be bool - Link to v5: https://lore.kernel.org/r/20240719-xtheadvector-v5-0-4b485fc7d...@rivosinc.com Changes in v5: - Rebase on for-next - Link to v4: https://lore.kernel.org/r/20240702-xtheadvector-v4-0-2bad6820d...@rivosinc.com Changes in v4: - Replace inline asm with C (Samuel) - Rename VCSRs to CSRs (Samuel) - Replace .insn directives with .4byte directives - Link to v3: https://lore.kernel.org/r/20240619-xtheadvector-v3-0-bff39eb96...@rivosinc.com Changes in v3: - Add back Heiko's signed-off-by (Conor) - Mark RISCV_HWPROBE_KEY_VENDOR_EXT_THEAD_0 as a bitmask - Link to v2: https://lore.kernel.org/r/20240610-xtheadvector-v2-0-97a48613a...@rivosinc.com Changes in v2: - Removed extraneous references to "riscv,vlenb" (Jess) - Moved declaration of "thead,vlenb" into cpus.yaml and added restriction that it's only applicable to thead cores (Conor) - Check CONFIG_RISCV_ISA_XTHEADVECTOR instead of CONFIG_RISCV_ISA_V for thead,vlenb (Jess) - Fix naming of hwprobe variables (Evan) - Link to v1: https://lore.kernel.org/r/20240609-xtheadvector-v1-0-3fe591d7f...@rivosinc.com --- Charlie Jenkins (13): dt-bindings: riscv: Add xtheadvector ISA extension description dt-bindings: cpus: add a thead vlen register length property riscv: dts: allwinner: Add xtheadvector to the D1/D1s devicetree
Re: [PATCH] docs: media: document media multi-committers rules and process
Hi Laurent, Thank you for your constructive feedback! In my reply I didn't comment on your trivial changes like grammar/spelling, unless otherwise noted I'm OK with those. On 26/11/2024 16:19, Laurent Pinchart wrote: > Hi Mauro and Hans, > > On Mon, Nov 25, 2024 at 02:28:58PM +0100, Mauro Carvalho Chehab wrote: >> As the media subsystem will experiment with a multi-committers model, >> update the Maintainer's entry profile to the new rules, and add a file >> documenting the process to become a committer and to maintain such >> rights. >> >> Signed-off-by: Mauro Carvalho Chehab >> Signed-off-by: Hans Verkuil >> --- >> Documentation/driver-api/media/index.rst | 1 + >> .../media/maintainer-entry-profile.rst| 193 ++ >> .../driver-api/media/media-committer.rst | 252 ++ >> .../process/maintainer-pgp-guide.rst | 2 + >> 4 files changed, 398 insertions(+), 50 deletions(-) >> create mode 100644 Documentation/driver-api/media/media-committer.rst >> >> diff --git a/Documentation/driver-api/media/index.rst >> b/Documentation/driver-api/media/index.rst >> index d5593182a3f9..d0c725fcbc67 100644 >> --- a/Documentation/driver-api/media/index.rst >> +++ b/Documentation/driver-api/media/index.rst >> @@ -26,6 +26,7 @@ Documentation/userspace-api/media/index.rst >> :numbered: >> >> maintainer-entry-profile >> +media-committer >> >> v4l2-core >> dtv-core >> diff --git a/Documentation/driver-api/media/maintainer-entry-profile.rst >> b/Documentation/driver-api/media/maintainer-entry-profile.rst >> index ffc712a5f632..90c6c0d9cf17 100644 >> --- a/Documentation/driver-api/media/maintainer-entry-profile.rst >> +++ b/Documentation/driver-api/media/maintainer-entry-profile.rst >> @@ -27,19 +27,128 @@ It covers, mainly, the contents of those directories: >> Both media userspace and Kernel APIs are documented and the documentation >> must be kept in sync with the API changes. It means that all patches that >> add new features to the subsystem must also bring changes to the >> -corresponding API files. >> +corresponding API documentation files. > > I would have split this kind of small changes to a separate patch to > make reviews easier, but that's not a big deal. > >> >> -Due to the size and wide scope of the media subsystem, media's >> -maintainership model is to have sub-maintainers that have a broad >> -knowledge of a specific aspect of the subsystem. It is the sub-maintainers' >> -task to review the patches, providing feedback to users if the patches are >> +Due to the size and wide scope of the media subsystem, the media's >> +maintainership model is to have committers that have a broad knowledge of >> +a specific aspect of the subsystem. It is the committers' task to >> +review the patches, providing feedback to users if the patches are >> following the subsystem rules and are properly using the media kernel and >> userspace APIs. > > This sounds really like a maintainer definition. I won't bikeshed too > much on the wording though, we will always be able to adjust it later to > reflect the reality of the situation as it evolves. I do like the > removal of the "sub-maintainer" term though, as I've always found it > demeaning. Yeah, that was never a great name. > >> >> -Patches for the media subsystem must be sent to the media mailing list >> -at linux-me...@vger.kernel.org as plain text only e-mail. Emails with >> -HTML will be automatically rejected by the mail server. It could be wise >> -to also copy the sub-maintainer(s). >> +Media committers >> + >> + >> +In the media subsystem, there are experienced developers that can commit > > s/that/who/ > s/commit/push/ to standardize the vocabulary (below you use "upload" to > mean the same thing) > >> +patches directly on a development tree. These developers are called > > s/on a/to the/ > >> +Media committers and are divided into the following categories: >> + >> +- Committers: responsible for one or more drivers within the media >> subsystem. >> + They can upload changes to the tree that do not affect the core or ABI. > > s/upload/push/ > >> + >> +- Core committers: responsible for part of the media core. They are >> typically >> + responsible for one or more drivers within the media subsystem, but, >> besides >> + that, they can also merge patches that change the code common to multiple >> + drivers, including the kernel internal API/ABI. > > I would write "API" only here. Neither the kernel internal API nor its > internal ABI are stable, and given that lack of stability, the ABI > concept doesn't really apply within the kernel. I agree. > >> + >> +- Subsystem maintainers: responsible for the subsystem as a whole, with >> + access to the entire subsystem. >> + >> + Only subsystem maintainers can change the userspace API/ABI. > > This can give the impression that only subsystem maintainers are allowed > to work on the API. I would w
Re: [PATCH] docs: media: document media multi-committers rules and process
On Wed, Nov 27, 2024 at 10:39:48AM +0100, Mauro Carvalho Chehab wrote: > Em Tue, 26 Nov 2024 17:19:30 +0200 Laurent Pinchart escreveu: > > On Mon, Nov 25, 2024 at 02:28:58PM +0100, Mauro Carvalho Chehab wrote: > > > As the media subsystem will experiment with a multi-committers model, > > > update the Maintainer's entry profile to the new rules, and add a file > > > documenting the process to become a committer and to maintain such > > > rights. > > > > > > Signed-off-by: Mauro Carvalho Chehab > > > Signed-off-by: Hans Verkuil > > > --- > > > Documentation/driver-api/media/index.rst | 1 + > > > .../media/maintainer-entry-profile.rst| 193 ++ > > > .../driver-api/media/media-committer.rst | 252 ++ > > > .../process/maintainer-pgp-guide.rst | 2 + > > > 4 files changed, 398 insertions(+), 50 deletions(-) > > > create mode 100644 Documentation/driver-api/media/media-committer.rst > > > > > > diff --git a/Documentation/driver-api/media/index.rst > > > b/Documentation/driver-api/media/index.rst > > > index d5593182a3f9..d0c725fcbc67 100644 > > > --- a/Documentation/driver-api/media/index.rst > > > +++ b/Documentation/driver-api/media/index.rst > > > @@ -26,6 +26,7 @@ Documentation/userspace-api/media/index.rst > > > :numbered: > > > > > > maintainer-entry-profile > > > +media-committer > > > > > > v4l2-core > > > dtv-core > > > diff --git a/Documentation/driver-api/media/maintainer-entry-profile.rst > > > b/Documentation/driver-api/media/maintainer-entry-profile.rst > > > index ffc712a5f632..90c6c0d9cf17 100644 > > > --- a/Documentation/driver-api/media/maintainer-entry-profile.rst > > > +++ b/Documentation/driver-api/media/maintainer-entry-profile.rst > > > @@ -27,19 +27,128 @@ It covers, mainly, the contents of those directories: > > > Both media userspace and Kernel APIs are documented and the documentation > > > must be kept in sync with the API changes. It means that all patches that > > > add new features to the subsystem must also bring changes to the > > > -corresponding API files. > > > +corresponding API documentation files. > > > > I would have split this kind of small changes to a separate patch to > > make reviews easier, but that's not a big deal. > > > > > > > > -Due to the size and wide scope of the media subsystem, media's > > > -maintainership model is to have sub-maintainers that have a broad > > > -knowledge of a specific aspect of the subsystem. It is the > > > sub-maintainers' > > > -task to review the patches, providing feedback to users if the patches > > > are > > > +Due to the size and wide scope of the media subsystem, the media's > > > +maintainership model is to have committers that have a broad knowledge of > > > +a specific aspect of the subsystem. It is the committers' task to > > > +review the patches, providing feedback to users if the patches are > > > following the subsystem rules and are properly using the media kernel and > > > userspace APIs. > > > > This sounds really like a maintainer definition. I won't bikeshed too > > much on the wording though, we will always be able to adjust it later to > > reflect the reality of the situation as it evolves. I do like the > > removal of the "sub-maintainer" term though, as I've always found it > > demeaning. > > The main goal here was to replace sub-maintainers by committers. > > Other changes can go later on, or if you have a better way to define > the paper of committers, be welcomed to propose it. I think we would waste time arguing about this section. It's not the main goal of this patch, so I'm fine handling it later. > > > -Patches for the media subsystem must be sent to the media mailing list > > > -at linux-me...@vger.kernel.org as plain text only e-mail. Emails with > > > -HTML will be automatically rejected by the mail server. It could be wise > > > -to also copy the sub-maintainer(s). > > > +Media committers > > > + > > > + > > > +In the media subsystem, there are experienced developers that can commit > > > > > > > s/that/who/ > > s/commit/push/ to standardize the vocabulary (below you use "upload" to > > mean the same thing) > > > > > +patches directly on a development tree. These developers are called > > > > s/on a/to the/ > > I won't comment here any below about trivial changes like that: I'll > just incorporate at v2. I'll focus my reply on your the comments about > the text contents itself. Fine with me. I actually considered sending the trivial comments as a separate reply. > > > +Media committers and are divided into the following categories: > > > + > > > +- Committers: responsible for one or more drivers within the media > > > subsystem. > > > + They can upload changes to the tree that do not affect the core or > > > ABI. > > > > s/upload/push/ > > > > > + > > > +- Core committers: responsible for part of the media core. They are > > > typically > > > + respon
Re: [PATCH] docs: media: document media multi-committers rules and process
Em Wed, 27 Nov 2024 10:39:48 +0100 Mauro Carvalho Chehab escreveu: > > This workflow doesn't apply to patch submitters who are not allowed to > > send pull requests and who don't have direct commit access. I thought > > these submitters are the main audience of this document. In that case, I > > think moving the next section that explains the e-mail workflow before > > the "Media development workflow" section (which should likely be renamed > > to make it clear that it is about merging patches, not developing them) > > would be best. The "Review Cadence" section could also be folded in > > there, to give a full view of what a submitter can expect. > > > > This would also have the advantage of introducing the linuvtv.org > > patchwork instance, which you reference above. Documents are more > > readable when they introduce concepts first before using them. > > Will try to do such change at v2. Actually, both workflows (a) and (b) apply to the ones that can't send pull requests or push at media-committers.git: --- a. Normal workflow: patches are handled by subsystem maintainers:: +--+ +-+ +---+ +---+ +-+ |e-mail|-->|patchwork|-->|pull |-->|maintainers merge |-->|media.git| +--+ +-+ |request| |in media-committers.git| +-+ +---+ +---+ For this workflow, pull requests can be generated by a committer, a previous committer, subsystem maintainers or by a couple of trusted long-time contributors. If you are not in such group, please don't submit pull requests, as they will likely be ignored. b. Committers' workflow: patches are handled by media committers:: +--+ +-+ ++ +---+ +-+ |e-mail|-->|patchwork|-->|committers merge at |-->|maintainers|-->|media.git| +--+ +-+ |media-committers.git| |approval | +-+ ++ +---+ --- No matter who sent an e-mail, this will be picked by patchwork and either be part of a PR or a MR, depending on who picked it. Thanks, Mauro
Re: [PATCH] docs: media: document media multi-committers rules and process
Hi Hans, On Wed, Nov 27, 2024 at 12:59:58PM +0100, Hans Verkuil wrote: > Hi Laurent, > > Thank you for your constructive feedback! In my reply I didn't comment on your > trivial changes like grammar/spelling, unless otherwise noted I'm OK with > those. Some of the points you raised are also discussed in the other part of the mail thread. I'll elaborate here on the ones that are unique, and just mention for the other ones that we can discuss them in my reply to Mauro to avoid splitting the discussion. > On 26/11/2024 16:19, Laurent Pinchart wrote: > > Hi Mauro and Hans, > > > > On Mon, Nov 25, 2024 at 02:28:58PM +0100, Mauro Carvalho Chehab wrote: > >> As the media subsystem will experiment with a multi-committers model, > >> update the Maintainer's entry profile to the new rules, and add a file > >> documenting the process to become a committer and to maintain such > >> rights. > >> > >> Signed-off-by: Mauro Carvalho Chehab > >> Signed-off-by: Hans Verkuil > >> --- > >> Documentation/driver-api/media/index.rst | 1 + > >> .../media/maintainer-entry-profile.rst| 193 ++ > >> .../driver-api/media/media-committer.rst | 252 ++ > >> .../process/maintainer-pgp-guide.rst | 2 + > >> 4 files changed, 398 insertions(+), 50 deletions(-) > >> create mode 100644 Documentation/driver-api/media/media-committer.rst > >> > >> diff --git a/Documentation/driver-api/media/index.rst > >> b/Documentation/driver-api/media/index.rst > >> index d5593182a3f9..d0c725fcbc67 100644 > >> --- a/Documentation/driver-api/media/index.rst > >> +++ b/Documentation/driver-api/media/index.rst > >> @@ -26,6 +26,7 @@ Documentation/userspace-api/media/index.rst > >> :numbered: > >> > >> maintainer-entry-profile > >> +media-committer > >> > >> v4l2-core > >> dtv-core > >> diff --git a/Documentation/driver-api/media/maintainer-entry-profile.rst > >> b/Documentation/driver-api/media/maintainer-entry-profile.rst > >> index ffc712a5f632..90c6c0d9cf17 100644 > >> --- a/Documentation/driver-api/media/maintainer-entry-profile.rst > >> +++ b/Documentation/driver-api/media/maintainer-entry-profile.rst > >> @@ -27,19 +27,128 @@ It covers, mainly, the contents of those directories: > >> Both media userspace and Kernel APIs are documented and the documentation > >> must be kept in sync with the API changes. It means that all patches that > >> add new features to the subsystem must also bring changes to the > >> -corresponding API files. > >> +corresponding API documentation files. > > > > I would have split this kind of small changes to a separate patch to > > make reviews easier, but that's not a big deal. > > > >> > >> -Due to the size and wide scope of the media subsystem, media's > >> -maintainership model is to have sub-maintainers that have a broad > >> -knowledge of a specific aspect of the subsystem. It is the > >> sub-maintainers' > >> -task to review the patches, providing feedback to users if the patches are > >> +Due to the size and wide scope of the media subsystem, the media's > >> +maintainership model is to have committers that have a broad knowledge of > >> +a specific aspect of the subsystem. It is the committers' task to > >> +review the patches, providing feedback to users if the patches are > >> following the subsystem rules and are properly using the media kernel and > >> userspace APIs. > > > > This sounds really like a maintainer definition. I won't bikeshed too > > much on the wording though, we will always be able to adjust it later to > > reflect the reality of the situation as it evolves. I do like the > > removal of the "sub-maintainer" term though, as I've always found it > > demeaning. > > Yeah, that was never a great name. > > >> > >> -Patches for the media subsystem must be sent to the media mailing list > >> -at linux-me...@vger.kernel.org as plain text only e-mail. Emails with > >> -HTML will be automatically rejected by the mail server. It could be wise > >> -to also copy the sub-maintainer(s). > >> +Media committers > >> + > >> + > >> +In the media subsystem, there are experienced developers that can commit > > > > s/that/who/ > > s/commit/push/ to standardize the vocabulary (below you use "upload" to > > mean the same thing) > > > >> +patches directly on a development tree. These developers are called > > > > s/on a/to the/ > > > >> +Media committers and are divided into the following categories: > >> + > >> +- Committers: responsible for one or more drivers within the media > >> subsystem. > >> + They can upload changes to the tree that do not affect the core or ABI. > > > > s/upload/push/ > > > >> + > >> +- Core committers: responsible for part of the media core. They are > >> typically > >> + responsible for one or more drivers within the media subsystem, but, > >> besides > >> + that, they can also merge patches that change the code common to > >> multiple > >> + drivers, in
