Samuel Thibault <samuel.thiba...@gnu.org> writes:
> Maksym Planeta, le Sun 08 Apr 2012 00:17:43 +0300, a écrit :
>> Samuel Thibault <samuel.thiba...@gnu.org> writes:
>> > Maksym Planeta, le Sat 07 Apr 2012 23:20:13 +0300, a écrit :
>> >> > No: as I said, allocate an empty map, so that the existing code can poke
>> >> > at it without testing for its presence or not.
>> >> >
>> >> >> Purpose of this conditions is checking whether map (or submap) is
>> >> >> already empty.
>> >> >
>> >> > Not empty, but allocated.
>> >>
>> >> So, if, for instance, only one page of large object (that needs indirect
>> >> mapping) was evicted, the whole map would be allocated? And what the
>> >> purpose of two-level system in this case?
>> >
>> > I'm not saying to fill the whole two-level system.
>> >
>> > I'm saying to allocate the one-level system when truncation brings from
>> > two-level to one-level while the first map of the two-level happens to
>> > be NULL.
>> >
>>
>> But thus you will not avoid conditions, because most of them are applied
>> to second level.
>
> Existing conditions, agreed. But the conditions you introduce are about
> one-level only.
>
Really, but I can replace 3 conditions from the beginning of commit with
one. Would it fit?
>> Also you will need to allocate memory map for every object is created
>> even it would not ever use pageout.
>
> Well, that's already what we do in pager_alloc, don't we?
>
Than I'd rather removed initialization of map from pager_alloc, because,
I think, there is no sense to consume space for map if object doesn't
use it.
>> >> >> >> > There is also an issue with
>> >> >> >> >
>> >> >> >> > + if (!pager->map) {
>> >> >> >> > + invalidate_block (pager_offset);
>> >> >> >> > + goto done;
>> >> >> >> > + }
>> >> >> >> > pager_offset = pager->map[f_page];
>> >> >> >> >
>> >> >> >> > at that point, pager_offset is not initialized yet...
>> >> >> >> >
>> >> >> >>
>> >> >> >> invalidate_block is a macro that sets pager_offset, so, really,
>> >> >> >> pager_offset shouldn't been initialized yet.
>> >> >> >
>> >> >> > It sets the *content* pointed by pager_offset. It does not set the
>> >> >> > pager_offset pointer.
>> >> >>
>> >> >> pager_offset is not a pointer, it is a union.
>> >> >
>> >> > Oops, indeed, sorry about that. I'm still wondering, however: rather
>> >> > than a goto, why not just putting pager_offset = pager->map[f_page] in
>> >> > the else part?
>> >> >
>> >>
>> >> pager_offset = pager->map[f_page] could be just put in else block, but I
>> >> did so because "goto done" shows clearer that everything was done and
>> >> execution could be move to finalization part. Just a matter of style. Do
>> >> you think that this should be changed?
>> >
>> > In both cases pager_offset is filled with something, so I don't see why
>> > it should be different. In the indirect map case, it's a mere else.
>>
>> But pager_offset is filled anyway: either in condition or after.
>
> Yes, so why using a "goto" and not a mere "else"? AIUI, the whole point
> of that part of the code is to set pager_offset.
>
I just wanted to emphasis that everything was done... But, never mind,
I'll remake this.
Regards,
Maksym Planeta.
