Message-ID: <2ddfad15-394c-241f-b120-5b297ed5356f@oracle.com>
Date:   Thu, 30 Jun 2022 09:39:24 -0600
From:   Khalid Aziz <khalid.aziz@...cle.com>
To:     Mark Hemment <markhemm@...glemail.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        "Matthew Wilcox (Oracle)" <willy@...radead.org>,
        aneesh.kumar@...ux.ibm.com, arnd@...db.de, 21cnbao@...il.com,
        corbet@....net, dave.hansen@...ux.intel.com, david@...hat.com,
        ebiederm@...ssion.com, hagen@...u.net, jack@...e.cz,
        Kees Cook <keescook@...omium.org>, kirill@...temov.name,
        kucharsk@...il.com, linkinjeon@...nel.org,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        Linux-MM <linux-mm@...ck.org>, longpeng2@...wei.com,
        luto@...nel.org, pcc@...gle.com, rppt@...nel.org,
        sieberf@...zon.com, sjpark@...zon.de,
        Suren Baghdasaryan <surenb@...gle.com>, tst@...oebel-theuer.de,
        yzaikin@...gle.com
Subject: Re: [PATCH v2 0/9] Add support for shared PTEs across processes

On 6/30/22 05:57, Mark Hemment wrote:
> Hi Khalid,
> 
> On Wed, 29 Jun 2022 at 23:54, Khalid Aziz <khalid.aziz@...cle.com> wrote:
>>
>>
>> Memory pages shared between processes require a page table entry
>> (PTE) for each process. Each of these PTE consumes consume some of
>> the memory and as long as number of mappings being maintained is
>> small enough, this space consumed by page tables is not
>> objectionable. When very few memory pages are shared between
>> processes, the number of page table entries (PTEs) to maintain is
>> mostly constrained by the number of pages of memory on the system.
>> As the number of shared pages and the number of times pages are
>> shared goes up, amount of memory consumed by page tables starts to
>> become significant. This issue does not apply to threads. Any number
>> of threads can share the same pages inside a process while sharing
>> the same PTEs. Extending this same model to sharing pages across
>> processes can eliminate this issue for sharing across processes as
>> well.
>>
>> Some of the field deployments commonly see memory pages shared
>> across 1000s of processes. On x86_64, each page requires a PTE that
>> is only 8 bytes long which is very small compared to the 4K page
>> size. When 2000 processes map the same page in their address space,
>> each one of them requires 8 bytes for its PTE and together that adds
>> up to 8K of memory just to hold the PTEs for one 4K page. On a
>> database server with 300GB SGA, a system crash was seen with
>> out-of-memory condition when 1500+ clients tried to share this SGA
>> even though the system had 512GB of memory. On this server, in the
>> worst case scenario of all 1500 processes mapping every page from
>> SGA would have required 878GB+ for just the PTEs. If these PTEs
>> could be shared, amount of memory saved is very significant.
>>
>> This patch series implements a mechanism in kernel to allow
>> userspace processes to opt into sharing PTEs. It adds a new
>> in-memory filesystem - msharefs. A file created on msharefs creates
>> a new shared region where all processes sharing that region will
>> share the PTEs as well. A process can create a new file on msharefs
>> and then mmap it which assigns a starting address and size to this
>> mshare'd region. Another process that has the right permission to
>> open the file on msharefs can then mmap this file in its address
>> space at same virtual address and size and share this region through
>> shared PTEs. An unlink() on the file marks the mshare'd region for
>> deletion once there are no more users of the region. When the mshare
>> region is deleted, all the pages used by the region are freed.
> 
>    Noting the flexibility of 'mshare' has been reduced from v1.  The
> earlier version allowed msharing of named mappings, while this patch
> is only for anonymous mappings.
>    Any plans to support named mappings?  If not, I guess *someone* will
> want it (eventually).  Minor, as the patch does not introduce new
> syscalls, but having an API which is flexible for both named and anon
> mappings would be good (this is a nit, not a strong suggestion).

I apologize for not clarifying this. The initial mmap() call looks like an anonymous mapping but one could easily call 
mremap later and map any other objects in the same address space which remains shared until the mshare region is torn 
down. It is my intent to support mapping any objects in mshare region.

> 
>    The cover letter details the problem being solved and the API, but
> gives no details of the implementation.  A paragraph on the use of a
> mm_struct per-msharefs file would be helpful.

Good point. I will do that next time.

> 
>    I've only quickly scanned the patchset; not in enough detail to
> comment on each patch, but a few observations.
> 
>    o I was expecting to see mprotect() against a mshared vma to either
> be disallowed or code to support the splitting of a mshared vma.  I
> didn't see either.msharefs_delmm

Since mshare region is intended to support multiple objects being mapped in the region and different protections on 
different parts of region, mprotect should be supported and should handle splitting the mshare'd vmas. Until basic code 
is solid, it would make sense to prevent splitting vmas and add that on later. I will add this code.

> 
>    o For the case where the mshare file has been closed/unmmap but not
> unlinked, a 'mshare_data' structure will leaked when the inode is
> evicted.

You are right. mshare_evict_inode() needs to call msharefs_delmm() to clean up.

> 
>    o The alignment requirement is PGDIR_SIZE, which is very large.
> Should/could this be PMD_SIZE?

Yes, PGDIR_SIZE is large. It works for the database folks who requested this feature but PMD might be more versatile. I 
have been thinking about switching to PMD since that will make it easier to move hugetlbfs page table sharing code over 
to this code.

> 
>    o mshare should be a conditional feature (CONFIG_MSHARE ?).

I can do that. I was reluctant to add yet another CONFIG option. Since this feature is activated explicitly by userspace 
code, is it necessary to make it a config option?

> 
> 
>    I might get a chance do a finer grain review later/tomorrow.
> 
>> API
>> ===
>>
>> mshare does not introduce a new API. It instead uses existing APIs
>> to implement page table sharing. The steps to use this feature are:
>>
>> 1. Mount msharefs on /sys/fs/mshare -
>>          mount -t msharefs msharefs /sys/fs/mshare
>>
>> 2. mshare regions have alignment and size requirements. Start
>>     address for the region must be aligned to an address boundary and
>>     be a multiple of fixed size. This alignment and size requirement
>>     can be obtained by reading the file /sys/fs/mshare/mshare_info
>>     which returns a number in text format. mshare regions must be
>>     aligned to this boundary and be a multiple of this size.
>>
>> 3. For the process creating mshare region:
>>          a. Create a file on /sys/fs/mshare, for example -
>>                  fd = open("/sys/fs/mshare/shareme",
>>                                  O_RDWR|O_CREAT|O_EXCL, 0600);
>>
>>          b. mmap this file to establish starting address and size -
>>                  mmap((void *)TB(2), BUF_SIZE, PROT_READ | PROT_WRITE,
>>                          MAP_SHARED, fd, 0);
>>
>>          c. Write and read to mshared region normally.
>>
>> 4. For processes attaching to mshare'd region:
>>          a. Open the file on msharefs, for example -
>>                  fd = open("/sys/fs/mshare/shareme", O_RDWR);
>>
>>          b. Get information about mshare'd region from the file:
>>                  struct mshare_info {
>>                          unsigned long start;
>>                          unsigned long size;
>>                  } m_info;
>>
>>                  read(fd, &m_info, sizeof(m_info));
>>
>>          c. mmap the mshare'd region -
>>                  mmap(m_info.start, m_info.size,
>>                          PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
>>
>> 5. To delete the mshare region -
>>                  unlink("/sys/fs/mshare/shareme");
>>
>>
>>
>> Example Code
>> ============
>>
>> Snippet of the code that a donor process would run looks like below:
>>
>> -----------------
>>          fd = open("/sys/fs/mshare/mshare_info", O_RDONLY);
>>          read(fd, req, 128);
>>          alignsize = atoi(req);
>>          close(fd);
>>          fd = open("/sys/fs/mshare/shareme", O_RDWR|O_CREAT|O_EXCL, 0600);
>>          start = alignsize * 4;
>>          size = alignsize * 2;
>>          addr = mmap((void *)start, size, PROT_READ | PROT_WRITE,
>>                          MAP_SHARED | MAP_ANONYMOUS, 0, 0);
> 
> Typo, missing 'fd'; MAP_SHARED | MAP_ANONYMOUS, fd, 0)

Yes, you are right. I will fix that.

Thanks, Mark! I really appreciate your taking time to review this code.

--
Khalid

Powered by blists - more mailing lists