Messages in this thread | | | Date | Fri, 24 Jun 2005 11:17:29 +1000 | From | Nick Piggin <> | Subject | Re: [patch][rfc] 5/5: core remove PageReserved |
| |
William Lee Irwin III wrote:
> On Fri, Jun 24, 2005 at 09:21:31AM +1000, Nick Piggin wrote: > >>This patch doesn't care how it works, that would be for a later patch. > > > The general gist of all this is that the patch doesn't cover anywhere > near enough ground, and so the above illustrates that the usage > in/around bootmem is among the easiest of usages to remove. I have > implemented what I'm talking about on several independent occasions. >
I don't think you understand what ground the patch covers. It removes PageReserved() queries from core code, leaving all PG_reserved flags and other uses of it intact so it doesn't cause mass breakage.
Things can then be looked at and fixed up properly at a slower pace while the patch is in -mm, for example.
> > William Lee Irwin III wrote: > >>>This is called in the interior of a loop, which may be beneficial to >>>terminate if this intended semantic is to be enforced. Furthermore, no >>>error is propagated to the caller, which is not the desired effect in >>>the stated error reporting scheme. So the code is inconsistent with >>>explicitly stated intention. > > > On Fri, Jun 24, 2005 at 09:21:31AM +1000, Nick Piggin wrote: > >>No, the error reporting scheme says it doesn't handle any error, >>that is all. What we have here in terms of behaviour is exactly >>what used to happen, that is - do something saneish on error. >>Changing behaviour would be outside the scope of this patch, but >>be my guest. > > > Some places BUG(), some back out with an error return, others blithely > proceed. This kind of inconsistency will broadly confuse callers of > the API's. >
I don't think any places BUG(), but regardless, this patch doesn't deal with changing behaviour, just reporting of the problem.
> > William Lee Irwin III wrote: > >>>You are going on about the fact that install_page() can't be used on >>>memory outside mem_map[] as it requires a page structure, and can't be >>>used on reserved pages because page_add_file_rmap() will BUG. This case >>>is not being discussed. > > > On Fri, Jun 24, 2005 at 09:21:31AM +1000, Nick Piggin wrote: > >>And that it isn't allowed to touch struct page of physical pages >>in a VM_RESERVED region. > > > non sequitur >
Huh? That is why it is broken. Previously, PageReserved pages *were not* accounted with the normal rmap and friends in core code, so you can't just do it in here and hope it works.
> > William Lee Irwin III wrote: > >>>The issue at stake is inserting normal pages into a VM_RESERVED vma. >>>These will arise as e.g. kernel-allocated buffers managed by normal >>>reference counting. remap_pfn_range() can't do it; it refuses to >>>operate on "valid memory". install_page() now won't do it; it refuses >>>to touch a VM_RESERVED vma. So this creates a giant semantic hole, >>>and potentially breaks working code (i.e. if you were going to do >>>this you would need not only a replacement but also a sweep to adjust >>>all the drivers doing it or prove their nonexistence). > > > On Fri, Jun 24, 2005 at 09:21:31AM +1000, Nick Piggin wrote: > >>I think you'll find that remap_pfn_range will be happy to operate >>on valid memory, and that any driver trying to use install_page >>on VM_RESERVED probably needs fixing anyway. > > > install_page() of a !PageReserved() page in a VM_RESERVED vma is > neither broken now nor does it merit going BUG(). >
Hugh says it is broken, so you can ask him the details.
> /dev/mem was disallowed from mapping ordinary kernel memory for a > reason (though I disagree with it), so the removal of the > pfn_valid()/PageReserved() checks can't be blithely done like in > your patch. Other arrangements must be made. > > > William Lee Irwin III wrote: > >>>Unfortunately for this scheme, it's very much a case of putting the >>>cart before the horse. PG_reserved is toggled at random in this driver >>>after this change, to no useful effect (debugging or otherwise). And >>>this really goes for the whole affair. Diddling the core first is just >>>going to create bugs. Converting the users first is the way these >>>things need to be done. When complete, nothing needs the core flags >>>twiddling anymore and you just nuke the flag twiddling from the core. > > > On Fri, Jun 24, 2005 at 09:21:31AM +1000, Nick Piggin wrote: > >>I'm sorry, I don't see how 'diddling' the core will create bugs. >>This is a fine way to do it, and "converting" users first (whatever >>that means) > > [cut text included in full in the following quote block] > > This is going way too far. Someone please deal with this. >
No, just tell me how it might magically create bugs in drivers that didn't exist in the first place?
> > On Fri, Jun 24, 2005 at 09:21:31AM +1000, Nick Piggin wrote: > [continued] > >>This is a fine way to do it, and "converting" users first (whatever >>that means) is not possible because VM_RESERVED handling in core >>code is not up to the task of replacing PageReserved without this >>patch. > > > You aren't replacing any of the PG_reserved usage with VM_RESERVED > in any of these patches. The primary motive of all this is AFAICT > little more than getting the PG_reserved check out of put_page(), > drivers and arches be damned. >
Excuse me? drivers work, arches work.
And how might you "fix" them first, without the necessary core code in place?
Send instant messages to your online friends http://au.messenger.yahoo.com
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |