[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: tmpfs status

From: Maksym Planeta
Subject: Re: tmpfs status
Date: Sat, 07 Apr 2012 19:51:56 +0300
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/23.3 (gnu/linux)

Samuel Thibault <samuel.thibault@gnu.org> writes:
>> >> Correct errors in default pager and make it work with tmpfs.
>> >
>> > That needs more explanation about how it works.
>> >
>> This is quite a big commit and I made several things there:
> So it should be broken down in several commits.


>> >> Take into account that pager's map could have NULL value.
>> >
>> > When can that happen?
>> >
>> It is normal situation that pager's map could have NULL value. This
>> occurs when some parts of object haven't been pageouted.
> How so?  Which lines of code make it NULL?  Maybe you are not aware
> that I don't know the mach-defpager code at all, just vague concepts.
> What I currently see is that dpager_t structures get initialized from
> pager_alloc(), where the map field gets initialized to non-zero,
> and that is always called from pager_port_alloc, always called from
> seqnos_memory_object_create.  There is no code that explicitly sets it
> to NULL until deallocation in pager_dealloc().
>> As you can see, old_mapptr is supplied as source for new page map, in
>> memcpy, so NULL pointer will be dereferenced.
> I fully understand what bad things can happen if map happens to be NULL,
> but for now my reading of the code is that it can't become NULL, and
> thus if it becomes, there is a bug earlier, and it's a bad thing to just
> paper over it.

Here is initialization code from pager_alloc():
                if (INDIRECT_PAGEMAP(size)) {
                        alloc_size = INDIRECT_PAGEMAP_SIZE(size);
                        init_value = (dp_map_t)0;

And from pager_extend():
            for (; i < INDIRECT_PAGEMAP_ENTRIES(new_size); i++)
                new_mapptr[i].indirect = (dp_map_t)0;

As you can see instead of NULL, (dp_map_t)0 is used.

> 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.

> Also I don't understand the use of
> +               if (no_block (*pager->map))
> +                   goto done;
>                 entry = pager->map[offset];
>                 invalidate_block(pager->map[offset]);
> *pager->map would be pager->map[0], I'd believe that it is independent
> from pager->map[offset], and clearing it can only bring issues.

Sorry, didn't get you.

>> >> Return support of old pageout format.
>> >
>> > AIUI, it's meant to fix the initialization issue that you raised some
>> > time ago?
>> No, this means that I should support both formats, because old one is
>> used by kernel. And new one is needed because new page attributes are
>> needed. So since this commit both interfaces are supported.
> That's actually what I meant. You should probably rather fold it into
> the patch that moves the code from _data_write to _data_return


Maksym Planeta.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]