On Jul 18 11:59, Brian Ford wrote: > On Tue, 18 Jul 2006, Corinna Vinschen wrote: > > I applied your patch to the cv-branch with some changes. The way you > > are calling search_record (see there) > > > > > + long record_idx = map_list->search_record ((caddr_t)addr, 1, > > > + u_addr, u_len, -1); > > > > always returns a u_len of 1. The result is that for each page in > > memory, the loop runs 4096 times in the worst case. > > I see that now :-(. > > I confess to not understanding the purpose and inner workings of either > search_record implementation. I first tried the two parameter version, > but then realized it was searching through file offsets rather than map > addresses (what is that usefull for?).
The answer is in list::try_map. It tries to find a suitable, unused record which can be reused for another mapping. The idea here is to accomodate old, non-standard implementations which assume that two consecutive mappings are using consecutive pages in memory. An example are old autoconf tests. This was more of a problem when getpagesize() was 4K. Today it will only be used on 9x due to the alignment bug I'm referring to in mmap64. > What I wanted was a function that returned the mmap record for an > arbitrary address [range]. That seems pretty basic. Why is there not > such a thing? > > > I added the necessary alignment stuff > > See, I don't understand why every caller should need to do "the necessary > alignment stuff"? It's simply working as designed. Did I claim somewhere that the code is perfect? I don't think so. If you have a better or more elegant solution, provide a patch. It's as easy as that. > > and minimized the number of calls to VirtualAlloc. > > Yeah, that was the concept I was going for. Find the map that this > address belongs to, commit the smaller of the address range for the region > or the map, repeat while there is still an uncommitted address range. You don't have to apologize. You did that, just the search_record call was incorrect. To find this requires some debugging, that's all. > > Don't be surprised that I now used getpagesize() instead of > > getsystempagesize (). [...] > > I know this dichotomy has been discussed at length before and there > doesn't seem to be any win-win compromise. I'm not sure if I agree or not > *shrug*, but my opinion doesn't matter much anyway. You know the problem of page size vs. allocation granularity, right? I was fighting for keeping the page size in Cygwin at 4K for a long time but at one point it got just too awkward to support it any longer, so I gave up. There's really no reason to go through this once again. > One minor nit though, this stuff could be moved after the check for an > empty mmap region list. Indeed. But, hey, this is the cygwin-patches list. Just provide a patch! > > Thanks for the patch. It's available for further digestion and patches > > in the cv-branch. > > I guess it'll be a while then before this hits a release then :-(. Thanks > for applying anyway. Sure. The branch will be folded back into the main line after 1.5.21 has been released which will be very soon. Corinna -- Corinna Vinschen Please, send mails regarding Cygwin to Cygwin Project Co-Leader cygwin AT cygwin DOT com Red Hat
