Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210722190747.1986614-1-hridya@google.com> <CALAqxLVLMt7rbJBQtFBw-ikBAjKrVgfS8=Nu6NFQbp_gq1m22Q@mail.gmail.com>
 <CA+wgaPOQmY4H9n302YspKuLk9iq9vBzdWBTu19EUUsiQYTUOzQ@mail.gmail.com>
 <CAO_48GFS5SsdNCwOp6Jb+nmZJ+SdNkQkq628VhxXRGSLVeP0Yg@mail.gmail.com> <YRI7cqWXM545iMzO@phenom.ffwll.local>
In-Reply-To: <YRI7cqWXM545iMzO@phenom.ffwll.local>
From:   Hridya Valsaraju <hridya@google.com>
Date:   Tue, 10 Aug 2021 10:48:26 -0700
Message-ID: <CA+wgaPNQYMd8dyCcmHfHbCXKEqpF8i8nJQvb0cD1HEPG5hDMoA@mail.gmail.com>
Subject: Re: [PATCH] dma-buf: heaps: Set allocation limit for system heap
To:     Sumit Semwal <sumit.semwal@linaro.org>,
        Hridya Valsaraju <hridya@google.com>,
        John Stultz <john.stultz@linaro.org>,
        Benjamin Gaignard <benjamin.gaignard@linaro.org>,
        Liam Mark <lmark@codeaurora.org>,
        Laura Abbott <labbott@redhat.com>,
        Brian Starkey <Brian.Starkey@arm.com>,
        =?UTF-8?Q?Christian_K=C3=B6nig?= <christian.koenig@amd.com>,
        linux-media <linux-media@vger.kernel.org>,
        dri-devel <dri-devel@lists.freedesktop.org>,
        "moderated list:DMA BUFFER SHARING FRAMEWORK" 
        <linaro-mm-sig@lists.linaro.org>,
        lkml <linux-kernel@vger.kernel.org>,
        Android Kernel Team <kernel-team@android.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Aug 10, 2021 at 1:40 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Tue, Aug 10, 2021 at 01:54:41PM +0530, Sumit Semwal wrote:
> > Hi Hridya,
> >
> > Apologies for the delay in responding.
> >
> > On Wed, 4 Aug 2021 at 03:09, Hridya Valsaraju <hridya@google.com> wrote=
:
> >
> > > On Mon, Aug 2, 2021 at 7:18 PM John Stultz <john.stultz@linaro.org> w=
rote:
> > > >
> > > > On Thu, Jul 22, 2021 at 12:07 PM Hridya Valsaraju <hridya@google.co=
m>
> > > wrote:
> > > > > This patch limits the size of total memory that can be requested =
in a
> > > > > single allocation from the system heap. This would prevent a
> > > > > buggy/malicious client from depleting system memory by requesting=
 for
> > > an
> > > > > extremely large allocation which might destabilize the system.
> > > > >
> > > > > The limit is set to half the size of the device's total RAM which=
 is
> > > the
> > > > > same as what was set by the deprecated ION system heap.
> > > > >
> > > > > Signed-off-by: Hridya Valsaraju <hridya@google.com>
> > > >
> > > > Seems sane to me, unless folks have better suggestions for allocati=
on
> > > limits.
> > > >
> > > > Reviewed-by: John Stultz <john.stultz@linaro.org>
> > >
> > > Thank you for taking a look John!
> > >
> > Looks good to me; I will apply it to drm-misc today.
>
> Please don't, this doesn't really solve anything:
> - it's easy to bypass, just allocate more buffers to get over the limit
> - resource limit plan is cgroups, not hand-rolled limits in every
>   allocator
> - the ttm "max half of system memory" is for pinned memory, to work aroun=
d
>   locking inversion issues between dma_resv_lock and core mm shrinkers. I=
t
>   does not actually impose an overall allocation limit, you can allocate
>   ttm bo until your entire memory (and swap) are full. Christian K=C3=B6n=
ig has
>   merged a patch set to lift this by reworking the shrinker interaction,
>   but it had to be reverted again because of some fallout I can't remembe=
r
>   offhand. dma_resv_lock vs shrinkers is very tricky.
>
> So if you want resource limits then you really want cgroups here.

Thanks Daniel and Sumit, that makes sense. Once the GPU memory
accounting cgroups are ready, we should be able to set the limit using
the same.

Regards,
Hridya

>
> Cheers, Daniel
>
> >
> >
> > >
> > > Regards,
> > > Hridya
> > >
> > > >
> > > > thanks
> > > > -john
> > >
> > Best,
> > Sumit.
> >
> > --
> > Thanks and regards,
> >
> > Sumit Semwal (he / him)
> > Tech Lead - LCG, Vertical Technologies
> > Linaro.org =E2=94=82 Open source software for ARM SoCs
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch