On Fri, 13 Apr 2018 14:33:09 -0700
Kenneth Graunke <kenn...@whitecape.org> wrote:
> On Friday, April 13, 2018 2:08:40 PM PDT James Xiong wrote:
> > On Fri, 13 Apr 2018 13:51:02 -0700
> > Kenneth Graunke <kenn...@whitecape.org> wrote:
> > > On Friday, April 13, 2018 1:35:45 PM PDT Kenneth Graunke wrote:
> > > > On Monday, April 9, 2018 4:06:16 PM PDT James Xiong wrote:
> > > > > From: "Xiong, James" <james.xi...@intel.com>
> > > > >
> > > > > On non-LLC platforms, we malloc shadow batch/state buffers
> > > > > of the same sizes as our batch/state buffers' GEM allocations.
> > > > > However the buffer allocator reuses similar-sized gem objects,
> > > > > it returns a buffer larger than we asked for in some cases
> > > > > and we end up with smaller shadow buffers. If we utilize the
> > > > > full-size of the over-allocated batch/state buffers, we may
> > > > > wind up accessing beyond the bounds of the shadow buffers and
> > > > > cause segmentation fault and/or memory corruption.
> > > >
> > > > Oh, good catch! We do indeed malloc too little if the bufmgr
> > Thank you for taking time to review, Kenneth.
> > > >
> > > > > A few examples:
> > > > > case batch state
> > > > > request bo shadow request bo shadow
> > > > > init 0 20K 20K 20K 16K 16K 16K
> > > > > grow_buffer 1 30K 32K 30K 24K 24K 24K
> > > > > grow_buffer 2 48K 48K 48K 36K 40K 36K
> > > > > grow_buffer 3 72K 80K 72K 60K 64K 60K
> > > > > grow_buffer 4 120K 128K 120K - - -
> > > > >
> > > > > batch #1, #3, #4; state #2 and #3 are problematic. We can
> > > > > change the order to allocate the bo first, then allocate the
> > > > > shadow buffer using the bo's size so that the shadow buffer
> > > > > have at least an equivalent size of the gem allocation.
> > > > >
> > > > > Another problem: even though the state/batch buffer could
> > > > > grow, when checking if it runs out space, we always compare
> > > > > with the initial batch/state sizes. To utilize the entire
> > > > > buffers, change to compare with the actual sizes.
> > > >
> > > > This is actually intentional. Our goal is to flush batches
> > > > when the amount of commands or state reaches those thresholds.
> > > > Occasionally, we'll be in the middle of emitting a draw, and
> > > > unable to stop. In that case, we grow the batch and keep
> > > > going. But after that, we're beyond our original target, so we
> > > > flush next time. We don't want to grow without bounds...it's
> > > > meant more for emergencies, or if we've badly estimated the
> > > > size of the draw call.
> > I am not sure I get it. Let me give an example: the state buffer
> > gets grown once from 16K to 24K in brw_state_batch(), the used_size
> > becomes 20K, then brw_require_statebuffer_space(1024) gets called to
> > ask for 1K space, with the original logical, it compares the used
> > size with 16K and flush the batch even though the state buffer
> > still has 4K space available?
> Yes, the idea is to flush at around 16K of state. If we happen to be
> in the middle of a draw and run out of space, we'll grow to 24K.
> Once it's over 16K, we flush as soon as we can.
> We'd like to be fairly consistent on our batch size. Running larger
> batches can lead to differences in performance, and large batches can
> lead to degradation in the interactivity of the system (especially on
> GPUs without preemption).
> The hope is to grow once, at most. If we check against the BO size,
> we might grow repeatedly, which would lead to really large batches and
> things would get out of hand.
I see, thanks for the explanation, Kenneth.
> > > >
> > > > I've sent a simpler patch which I think should hopefully fix
> > > > your bug: https://patchwork.freedesktop.org/patch/217107/
> > >
> > > Lionel noticed that I botched that patch. Here's an actual one:
> > >
> > > https://patchwork.freedesktop.org/patch/217108/
> > Yes it will fix the existing bug. However the assumption here is
> > that the init allocation size will NOT be rounded up as it happens
> > to be the bucket size.
> > I am working on an optimization to improve memory usage(that's how
> > I find out this bug), this assumption is no longer true.
> > Essentially the bufmgr could return a buffer with the same or
> > larger size whether it is same as the bucket's or not. Anyway I
> > guess I can send the fix later along with the optimization
> > patches.
> Ah, that's a good point. Your patch also tries to use the BO size
> for the initial malloc as well, which is a good idea...
So what do you want? you want me to change this patch and sent for
review or take yours for now.
mesa-dev mailing list