| Message ID | 20220308141437.144919-6-david@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: COW fixes part 2: reliable GUP pins of anonymous pages | expand |
> On Mar 8, 2022, at 6:14 AM, David Hildenbrand <david@redhat.com> wrote: > > We want to pass the flags to more than one anon rmap function, getting > rid of special "do_page_add_anon_rmap()". So let's pass around a distinct > __bitwise type and refine documentation. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > include/linux/rmap.h | 22 ++++++++++++++++++---- > mm/memory.c | 6 +++--- > mm/rmap.c | 7 ++++--- > 3 files changed, 25 insertions(+), 10 deletions(-) > > diff --git a/include/linux/rmap.h b/include/linux/rmap.h > index 92c3585b8c6a..49f6b208938c 100644 > --- a/include/linux/rmap.h > +++ b/include/linux/rmap.h > @@ -158,9 +158,23 @@ static inline void anon_vma_merge(struct vm_area_struct *vma, > > struct anon_vma *page_get_anon_vma(struct page *page); > > -/* bitflags for do_page_add_anon_rmap() */ > -#define RMAP_EXCLUSIVE 0x01 > -#define RMAP_COMPOUND 0x02 > +/* RMAP flags, currently only relevant for some anon rmap operations. */ > +typedef int __bitwise rmap_t; > + > +/* > + * No special request: if the page is a subpage of a compound page, it is > + * mapped via a PTE. The mapped (sub)page is possibly shared between processes. > + */ > +#define RMAP_NONE ((__force rmap_t)0) > + > +/* The (sub)page is exclusive to a single process. */ > +#define RMAP_EXCLUSIVE ((__force rmap_t)BIT(0)) > + > +/* > + * The compound page is not mapped via PTEs, but instead via a single PMD and > + * should be accounted accordingly. > + */ > +#define RMAP_COMPOUND ((__force rmap_t)BIT(1)) I was once shouted at for a similar suggestion, but I am going to try once more… If you already define a new type, why not to use bitfields? It would be much easier to read. The last time I made such a suggestion, Ingo said "I personally like bitfields in theory … [but] older versions of GCC did a really poor job of optimizing them.” At the time (2014), I looked at GCC-4.4 and GCC-4.8 and there were some differences in the quality of the generated code. Is it still the case?
On 08.03.22 18:15, Nadav Amit wrote: > > >> On Mar 8, 2022, at 6:14 AM, David Hildenbrand <david@redhat.com> wrote: >> >> We want to pass the flags to more than one anon rmap function, getting >> rid of special "do_page_add_anon_rmap()". So let's pass around a distinct >> __bitwise type and refine documentation. >> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> include/linux/rmap.h | 22 ++++++++++++++++++---- >> mm/memory.c | 6 +++--- >> mm/rmap.c | 7 ++++--- >> 3 files changed, 25 insertions(+), 10 deletions(-) >> >> diff --git a/include/linux/rmap.h b/include/linux/rmap.h >> index 92c3585b8c6a..49f6b208938c 100644 >> --- a/include/linux/rmap.h >> +++ b/include/linux/rmap.h >> @@ -158,9 +158,23 @@ static inline void anon_vma_merge(struct vm_area_struct *vma, >> >> struct anon_vma *page_get_anon_vma(struct page *page); >> >> -/* bitflags for do_page_add_anon_rmap() */ >> -#define RMAP_EXCLUSIVE 0x01 >> -#define RMAP_COMPOUND 0x02 >> +/* RMAP flags, currently only relevant for some anon rmap operations. */ >> +typedef int __bitwise rmap_t; >> + >> +/* >> + * No special request: if the page is a subpage of a compound page, it is >> + * mapped via a PTE. The mapped (sub)page is possibly shared between processes. >> + */ >> +#define RMAP_NONE ((__force rmap_t)0) >> + >> +/* The (sub)page is exclusive to a single process. */ >> +#define RMAP_EXCLUSIVE ((__force rmap_t)BIT(0)) >> + >> +/* >> + * The compound page is not mapped via PTEs, but instead via a single PMD and >> + * should be accounted accordingly. >> + */ >> +#define RMAP_COMPOUND ((__force rmap_t)BIT(1)) > Hi Nadav, > I was once shouted at for a similar suggestion, but I am going to try > once more… If you already define a new type, why not to use bitfields? I don't have a strong opinion, however, I'd prefer keeping it consistent with existing ways of passing flags. Personally, I like __bitwise because it just behave the way we're used to pass flags -- with additional type safety. Especially once eventually passing many flags (like we do with GFP), bitfields might turn out rather nasty -- IMHO. Thanks!
On Tue, Mar 8, 2022 at 9:15 AM Nadav Amit <namit@vmware.com> wrote: > > It would be much easier to read. The last time I made such a suggestion, > Ingo said "I personally like bitfields in theory … [but] older versions of > GCC did a really poor job of optimizing them.” Yeah, even not that old versions had serious issues, iirc. Bitfields can look nice, but they have some _serious_ syntax issues. In particular, they are nice when you want to *test* one single field (ie bit in this case), but basically atrociously bad in almost all other circumstances. For example, passing a bitfield aggregate as an argument is just crazy. Oh, you can do it, with syntax like (struct type) { .field1 = 1, .field3 = 1 } as the argument but when you say "much easier to read" I laugh in your face and call your mother a hamster. And that's ignoring all the issues when you want to combine two bitfields. You can't do it. There is nothing like the binary "or" operator. Again, it's easy to modify *one* field, but taking two bitfields and merging them? Not going to happen. So no. Bitfields have their place, but they are close to useless as "flags" type things that get passed around as arguments, unless you have very very specific and limited use. Linus
> On Mar 8, 2022, at 10:09 AM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Tue, Mar 8, 2022 at 9:15 AM Nadav Amit <namit@vmware.com> wrote: >> >> It would be much easier to read. The last time I made such a suggestion, >> Ingo said "I personally like bitfields in theory … [but] older versions of >> GCC did a really poor job of optimizing them.” > > Yeah, even not that old versions had serious issues, iirc. > > Bitfields can look nice, but they have some _serious_ syntax issues. > In particular, they are nice when you want to *test* one single field > (ie bit in this case), but basically atrociously bad in almost all > other circumstances. > > For example, passing a bitfield aggregate as an argument is just > crazy. Oh, you can do it, with syntax like > > (struct type) { .field1 = 1, .field3 = 1 } > > as the argument but when you say "much easier to read" I laugh in your > face and call your mother a hamster. > > And that's ignoring all the issues when you want to combine two > bitfields. You can't do it. There is nothing like the binary "or" > operator. Again, it's easy to modify *one* field, but taking two > bitfields and merging them? Not going to happen. > > So no. Bitfields have their place, but they are close to useless as > "flags" type things that get passed around as arguments, unless you > have very very specific and limited use. I see your point regarding passing an arg. The or’ing of bitfields can easily be resolved, unless I am missing something, with a union that holds the aggregate value and an anonymous struct that holds the individual flags. At the time, I thought that bitfields are much better fit for cpuid fields (which are not just flags). Anyhow, I will refrain from using bitfields for flags, if only for the sake of my mother. :)
On Tue, Mar 8, 2022 at 10:24 AM Nadav Amit <namit@vmware.com> wrote: > > I see your point regarding passing an arg. The or’ing of bitfields > can easily be resolved, unless I am missing something, with a union > that holds the aggregate value and an anonymous struct that holds > the individual flags. I think that falls under the same heading as passing them as arguments: it's certainly doable, but it requires special work that is hidden by helper macros/functions/types. I mean, even the "pass as arguments" can certainly work. It's not impossible to hide the odd syntax behind a macro, particularly if you only ever have a couple of specific cases. So you can do typedef struct rmap_flags { unsigned int exclusive:1, compound:1; } rmap_t; #define RMAP_EXCLUSIVE (rmap_t) { .exclusive = 1 } #define RMAP_COMPOUND (rmap_t) { .compound = 1 } and now you can use RMAP_EXCLUSIVE when you pass it as an argument, and in the functions themselves you can use if (flags.exclusive) {... which is certainly not unreadable. But it does mean that you basically have one syntax for testing "is this exclusive" and another for passing that value in. And you can't do RMAP_EXCLUSIVE | RMAP_COMPOUND to say "I want to pass in both exclusive and compound", but you *can* do flags.exclusive = 1; to set the exclusive bit. Again - that is certainly not unreadable on its own, but it's an example of how inconsistent and inconvenient the interface gets once you do anything outside of some very specific cases. And yes, you can solve these cases by simply always limiting yourself to specific syntax (in particular, just make the rule be that you can never create values out of thin air, you always have a variable that gets set. The bitfield thing does have the advantage that it ends up having very strict type checking. But in general, I'd say that the disadvantages are huge enough that you should never use a bitfield "on its own". Once it's part of a structure that you have to pass around and initialize as a structure *anyway*, then most of the problems go away. So bitfields as part of structures are fine - and we obviously use them extensively in that form. Even then they can have problems if there are any kinds of atomicity issues (ie think "page flags" or anything like that) but that's obviously a different thing, and using a union to get both ways to access things isn't out of the question. Of course, if you use unions to get "both as a bitfield and as a value" things working, you suddenly have "bit order issues", so that can be really problematic too. Bit endianness doesn't even have to follow byte endianness. End result: bitfields are actually often more complex than you think they are. Linus
diff --git a/include/linux/rmap.h b/include/linux/rmap.h index 92c3585b8c6a..49f6b208938c 100644 --- a/include/linux/rmap.h +++ b/include/linux/rmap.h @@ -158,9 +158,23 @@ static inline void anon_vma_merge(struct vm_area_struct *vma, struct anon_vma *page_get_anon_vma(struct page *page); -/* bitflags for do_page_add_anon_rmap() */ -#define RMAP_EXCLUSIVE 0x01 -#define RMAP_COMPOUND 0x02 +/* RMAP flags, currently only relevant for some anon rmap operations. */ +typedef int __bitwise rmap_t; + +/* + * No special request: if the page is a subpage of a compound page, it is + * mapped via a PTE. The mapped (sub)page is possibly shared between processes. + */ +#define RMAP_NONE ((__force rmap_t)0) + +/* The (sub)page is exclusive to a single process. */ +#define RMAP_EXCLUSIVE ((__force rmap_t)BIT(0)) + +/* + * The compound page is not mapped via PTEs, but instead via a single PMD and + * should be accounted accordingly. + */ +#define RMAP_COMPOUND ((__force rmap_t)BIT(1)) /* * rmap interfaces called when adding or removing pte of page @@ -169,7 +183,7 @@ void page_move_anon_rmap(struct page *, struct vm_area_struct *); void page_add_anon_rmap(struct page *, struct vm_area_struct *, unsigned long, bool); void do_page_add_anon_rmap(struct page *, struct vm_area_struct *, - unsigned long, int); + unsigned long, rmap_t); void page_add_new_anon_rmap(struct page *, struct vm_area_struct *, unsigned long, bool); void page_add_file_rmap(struct page *, bool); diff --git a/mm/memory.c b/mm/memory.c index b9602d41d907..bbce3ca72974 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -3515,10 +3515,10 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) struct vm_area_struct *vma = vmf->vma; struct page *page = NULL, *swapcache; struct swap_info_struct *si = NULL; + rmap_t rmap_flags = RMAP_NONE; swp_entry_t entry; pte_t pte; int locked; - int exclusive = 0; vm_fault_t ret = 0; void *shadow = NULL; @@ -3693,7 +3693,7 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) pte = maybe_mkwrite(pte_mkdirty(pte), vma); vmf->flags &= ~FAULT_FLAG_WRITE; ret |= VM_FAULT_WRITE; - exclusive = RMAP_EXCLUSIVE; + rmap_flags |= RMAP_EXCLUSIVE; } flush_icache_page(vma, page); if (pte_swp_soft_dirty(vmf->orig_pte)) @@ -3709,7 +3709,7 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) page_add_new_anon_rmap(page, vma, vmf->address, false); lru_cache_add_inactive_or_unevictable(page, vma); } else { - do_page_add_anon_rmap(page, vma, vmf->address, exclusive); + do_page_add_anon_rmap(page, vma, vmf->address, rmap_flags); } set_pte_at(vma->vm_mm, vmf->address, vmf->pte, pte); diff --git a/mm/rmap.c b/mm/rmap.c index f825aeef61ca..3d7028d100ea 100644 --- a/mm/rmap.c +++ b/mm/rmap.c @@ -1139,7 +1139,8 @@ static void __page_check_anon_rmap(struct page *page, void page_add_anon_rmap(struct page *page, struct vm_area_struct *vma, unsigned long address, bool compound) { - do_page_add_anon_rmap(page, vma, address, compound ? RMAP_COMPOUND : 0); + do_page_add_anon_rmap(page, vma, address, + compound ? RMAP_COMPOUND : RMAP_NONE); } /* @@ -1148,7 +1149,7 @@ void page_add_anon_rmap(struct page *page, * Everybody else should continue to use page_add_anon_rmap above. */ void do_page_add_anon_rmap(struct page *page, - struct vm_area_struct *vma, unsigned long address, int flags) + struct vm_area_struct *vma, unsigned long address, rmap_t flags) { bool compound = flags & RMAP_COMPOUND; bool first; @@ -1189,7 +1190,7 @@ void do_page_add_anon_rmap(struct page *page, /* address might be in next vma when migration races vma_adjust */ if (first) __page_set_anon_rmap(page, vma, address, - flags & RMAP_EXCLUSIVE); + !!(flags & RMAP_EXCLUSIVE)); else __page_check_anon_rmap(page, vma, address); }
We want to pass the flags to more than one anon rmap function, getting rid of special "do_page_add_anon_rmap()". So let's pass around a distinct __bitwise type and refine documentation. Signed-off-by: David Hildenbrand <david@redhat.com> --- include/linux/rmap.h | 22 ++++++++++++++++++---- mm/memory.c | 6 +++--- mm/rmap.c | 7 ++++--- 3 files changed, 25 insertions(+), 10 deletions(-)