Replace size_t with uint64_t in ringbuffer.

Adds overflow check in RingBufferAllocator + unit-test.
Also, update clients to use uint64_t to avoid casts or narrowing.

BUG=dawn:233

Change-Id: I652e3142407006d082491add600371f95d44741a
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/12380
Reviewed-by: Austin Eng <enga@chromium.org>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Commit-Queue: Bryan Bernhart <bryan.bernhart@intel.com>
This commit is contained in:
Bryan Bernhart 2019-10-18 16:19:00 +00:00 committed by Commit Bot service account
parent 4794168ef8
commit f603903da7
7 changed files with 58 additions and 42 deletions

View File

@ -18,7 +18,7 @@
namespace dawn_native { namespace dawn_native {
DynamicUploader::DynamicUploader(DeviceBase* device, size_t size) : mDevice(device) { DynamicUploader::DynamicUploader(DeviceBase* device, uint64_t size) : mDevice(device) {
mRingBuffers.emplace_back( mRingBuffers.emplace_back(
std::unique_ptr<RingBuffer>(new RingBuffer{nullptr, RingBufferAllocator(size)})); std::unique_ptr<RingBuffer>(new RingBuffer{nullptr, RingBufferAllocator(size)}));
} }
@ -28,7 +28,7 @@ namespace dawn_native {
mDevice->GetPendingCommandSerial()); mDevice->GetPendingCommandSerial());
} }
ResultOrError<UploadHandle> DynamicUploader::Allocate(size_t allocationSize, Serial serial) { ResultOrError<UploadHandle> DynamicUploader::Allocate(uint64_t allocationSize, Serial serial) {
// Note: Validation ensures size is already aligned. // Note: Validation ensures size is already aligned.
// First-fit: find next smallest buffer large enough to satisfy the allocation request. // First-fit: find next smallest buffer large enough to satisfy the allocation request.
RingBuffer* targetRingBuffer = mRingBuffers.back().get(); RingBuffer* targetRingBuffer = mRingBuffers.back().get();
@ -36,7 +36,7 @@ namespace dawn_native {
const RingBufferAllocator& ringBufferAllocator = ringBuffer->mAllocator; const RingBufferAllocator& ringBufferAllocator = ringBuffer->mAllocator;
// Prevent overflow. // Prevent overflow.
ASSERT(ringBufferAllocator.GetSize() >= ringBufferAllocator.GetUsedSize()); ASSERT(ringBufferAllocator.GetSize() >= ringBufferAllocator.GetUsedSize());
const size_t remainingSize = const uint64_t remainingSize =
ringBufferAllocator.GetSize() - ringBufferAllocator.GetUsedSize(); ringBufferAllocator.GetSize() - ringBufferAllocator.GetUsedSize();
if (allocationSize <= remainingSize) { if (allocationSize <= remainingSize) {
targetRingBuffer = ringBuffer.get(); targetRingBuffer = ringBuffer.get();
@ -44,7 +44,7 @@ namespace dawn_native {
} }
} }
size_t startOffset = RingBufferAllocator::kInvalidOffset; uint64_t startOffset = RingBufferAllocator::kInvalidOffset;
if (targetRingBuffer != nullptr) { if (targetRingBuffer != nullptr) {
startOffset = targetRingBuffer->mAllocator.Allocate(allocationSize, serial); startOffset = targetRingBuffer->mAllocator.Allocate(allocationSize, serial);
} }

View File

@ -25,13 +25,13 @@ namespace dawn_native {
struct UploadHandle { struct UploadHandle {
uint8_t* mappedBuffer = nullptr; uint8_t* mappedBuffer = nullptr;
size_t startOffset = 0; uint64_t startOffset = 0;
StagingBufferBase* stagingBuffer = nullptr; StagingBufferBase* stagingBuffer = nullptr;
}; };
class DynamicUploader { class DynamicUploader {
public: public:
DynamicUploader(DeviceBase* device, size_t size = kBaseUploadBufferSize); DynamicUploader(DeviceBase* device, uint64_t size = kBaseUploadBufferSize);
~DynamicUploader() = default; ~DynamicUploader() = default;
// We add functions to Release StagingBuffers to the DynamicUploader as there's // We add functions to Release StagingBuffers to the DynamicUploader as there's
@ -40,12 +40,12 @@ namespace dawn_native {
// implemented. // implemented.
void ReleaseStagingBuffer(std::unique_ptr<StagingBufferBase> stagingBuffer); void ReleaseStagingBuffer(std::unique_ptr<StagingBufferBase> stagingBuffer);
ResultOrError<UploadHandle> Allocate(size_t allocationSize, Serial serial); ResultOrError<UploadHandle> Allocate(uint64_t allocationSize, Serial serial);
void Deallocate(Serial lastCompletedSerial); void Deallocate(Serial lastCompletedSerial);
private: private:
// TODO(bryan.bernhart@intel.com): Figure out this value. // TODO(bryan.bernhart@intel.com): Figure out this value.
static constexpr size_t kBaseUploadBufferSize = 64000; static constexpr uint64_t kBaseUploadBufferSize = 64000;
struct RingBuffer { struct RingBuffer {
std::unique_ptr<StagingBufferBase> mStagingBuffer; std::unique_ptr<StagingBufferBase> mStagingBuffer;

View File

@ -29,7 +29,7 @@
// TODO(bryan.bernhart@intel.com): Follow-up with ringbuffer optimization. // TODO(bryan.bernhart@intel.com): Follow-up with ringbuffer optimization.
namespace dawn_native { namespace dawn_native {
RingBufferAllocator::RingBufferAllocator(size_t maxSize) : mMaxBlockSize(maxSize) { RingBufferAllocator::RingBufferAllocator(uint64_t maxSize) : mMaxBlockSize(maxSize) {
} }
void RingBufferAllocator::Deallocate(Serial lastCompletedSerial) { void RingBufferAllocator::Deallocate(Serial lastCompletedSerial) {
@ -43,11 +43,11 @@ namespace dawn_native {
mInflightRequests.ClearUpTo(lastCompletedSerial); mInflightRequests.ClearUpTo(lastCompletedSerial);
} }
size_t RingBufferAllocator::GetSize() const { uint64_t RingBufferAllocator::GetSize() const {
return mMaxBlockSize; return mMaxBlockSize;
} }
size_t RingBufferAllocator::GetUsedSize() const { uint64_t RingBufferAllocator::GetUsedSize() const {
return mUsedSize; return mUsedSize;
} }
@ -62,7 +62,7 @@ namespace dawn_native {
// queue, which identifies an existing (or new) frames-worth of resources. Internally, the // queue, which identifies an existing (or new) frames-worth of resources. Internally, the
// ring-buffer maintains offsets of 3 "memory" states: Free, Reclaimed, and Used. This is done // ring-buffer maintains offsets of 3 "memory" states: Free, Reclaimed, and Used. This is done
// in FIFO order as older frames would free resources before newer ones. // in FIFO order as older frames would free resources before newer ones.
size_t RingBufferAllocator::Allocate(size_t allocationSize, Serial serial) { uint64_t RingBufferAllocator::Allocate(uint64_t allocationSize, Serial serial) {
// Check if the buffer is full by comparing the used size. // Check if the buffer is full by comparing the used size.
// If the buffer is not split where waste occurs (e.g. cannot fit new sub-alloc in front), a // If the buffer is not split where waste occurs (e.g. cannot fit new sub-alloc in front), a
// subsequent sub-alloc could fail where the used size was previously adjusted to include // subsequent sub-alloc could fail where the used size was previously adjusted to include
@ -71,7 +71,13 @@ namespace dawn_native {
return kInvalidOffset; return kInvalidOffset;
} }
size_t startOffset = kInvalidOffset; // Ensure adding allocationSize does not overflow.
const uint64_t remainingSize = (mMaxBlockSize - mUsedSize);
if (allocationSize > remainingSize) {
return kInvalidOffset;
}
uint64_t startOffset = kInvalidOffset;
// Check if the buffer is NOT split (i.e sub-alloc on ends) // Check if the buffer is NOT split (i.e sub-alloc on ends)
if (mUsedStartOffset <= mUsedEndOffset) { if (mUsedStartOffset <= mUsedEndOffset) {
@ -86,7 +92,7 @@ namespace dawn_native {
} else if (allocationSize <= mUsedStartOffset) { // Try to sub-alloc at front. } else if (allocationSize <= mUsedStartOffset) { // Try to sub-alloc at front.
// Count the space at the end so that a subsequent // Count the space at the end so that a subsequent
// sub-alloc cannot not succeed when the buffer is full. // sub-alloc cannot not succeed when the buffer is full.
const size_t requestSize = (mMaxBlockSize - mUsedEndOffset) + allocationSize; const uint64_t requestSize = (mMaxBlockSize - mUsedEndOffset) + allocationSize;
startOffset = 0; startOffset = 0;
mUsedEndOffset = allocationSize; mUsedEndOffset = allocationSize;

View File

@ -26,32 +26,32 @@ namespace dawn_native {
class RingBufferAllocator { class RingBufferAllocator {
public: public:
RingBufferAllocator() = default; RingBufferAllocator() = default;
RingBufferAllocator(size_t maxSize); RingBufferAllocator(uint64_t maxSize);
~RingBufferAllocator() = default; ~RingBufferAllocator() = default;
size_t Allocate(size_t allocationSize, Serial serial); uint64_t Allocate(uint64_t allocationSize, Serial serial);
void Deallocate(Serial lastCompletedSerial); void Deallocate(Serial lastCompletedSerial);
size_t GetSize() const; uint64_t GetSize() const;
bool Empty() const; bool Empty() const;
size_t GetUsedSize() const; uint64_t GetUsedSize() const;
static constexpr size_t kInvalidOffset = std::numeric_limits<size_t>::max(); static constexpr uint64_t kInvalidOffset = std::numeric_limits<uint64_t>::max();
private: private:
struct Request { struct Request {
size_t endOffset; uint64_t endOffset;
size_t size; uint64_t size;
}; };
SerialQueue<Request> mInflightRequests; // Queue of the recorded sub-alloc requests (e.g. SerialQueue<Request> mInflightRequests; // Queue of the recorded sub-alloc requests (e.g.
// frame of resources). // frame of resources).
size_t mUsedEndOffset = 0; // Tail of used sub-alloc requests (in bytes). uint64_t mUsedEndOffset = 0; // Tail of used sub-alloc requests (in bytes).
size_t mUsedStartOffset = 0; // Head of used sub-alloc requests (in bytes). uint64_t mUsedStartOffset = 0; // Head of used sub-alloc requests (in bytes).
size_t mMaxBlockSize = 0; // Max size of the ring buffer (in bytes). uint64_t mMaxBlockSize = 0; // Max size of the ring buffer (in bytes).
size_t mUsedSize = 0; // Size of the sub-alloc requests (in bytes) of the ring buffer. uint64_t mUsedSize = 0; // Size of the sub-alloc requests (in bytes) of the ring buffer.
size_t mCurrentRequestSize = uint64_t mCurrentRequestSize =
0; // Size of the sub-alloc requests (in bytes) of the current serial. 0; // Size of the sub-alloc requests (in bytes) of the current serial.
}; };
} // namespace dawn_native } // namespace dawn_native

View File

@ -25,7 +25,7 @@ namespace dawn_native { namespace d3d12 {
DescriptorHeapHandle::DescriptorHeapHandle(ComPtr<ID3D12DescriptorHeap> descriptorHeap, DescriptorHeapHandle::DescriptorHeapHandle(ComPtr<ID3D12DescriptorHeap> descriptorHeap,
uint32_t sizeIncrement, uint32_t sizeIncrement,
uint32_t offset) uint64_t offset)
: mDescriptorHeap(descriptorHeap), mSizeIncrement(sizeIncrement), mOffset(offset) { : mDescriptorHeap(descriptorHeap), mSizeIncrement(sizeIncrement), mOffset(offset) {
} }
@ -68,12 +68,11 @@ namespace dawn_native { namespace d3d12 {
DescriptorHeapInfo* heapInfo, DescriptorHeapInfo* heapInfo,
D3D12_DESCRIPTOR_HEAP_FLAGS flags) { D3D12_DESCRIPTOR_HEAP_FLAGS flags) {
const Serial pendingSerial = mDevice->GetPendingCommandSerial(); const Serial pendingSerial = mDevice->GetPendingCommandSerial();
size_t startOffset = (heapInfo->heap == nullptr) uint64_t startOffset = (heapInfo->heap == nullptr)
? RingBufferAllocator::kInvalidOffset ? RingBufferAllocator::kInvalidOffset
: heapInfo->allocator.Allocate(count, pendingSerial); : heapInfo->allocator.Allocate(count, pendingSerial);
if (startOffset != RingBufferAllocator::kInvalidOffset) { if (startOffset != RingBufferAllocator::kInvalidOffset) {
return DescriptorHeapHandle{heapInfo->heap, mSizeIncrements[type], return DescriptorHeapHandle{heapInfo->heap, mSizeIncrements[type], startOffset};
static_cast<uint32_t>(startOffset)};
} }
// If the pool has no more space, replace the pool with a new one of the specified size // If the pool has no more space, replace the pool with a new one of the specified size

View File

@ -33,7 +33,7 @@ namespace dawn_native { namespace d3d12 {
DescriptorHeapHandle(); DescriptorHeapHandle();
DescriptorHeapHandle(ComPtr<ID3D12DescriptorHeap> descriptorHeap, DescriptorHeapHandle(ComPtr<ID3D12DescriptorHeap> descriptorHeap,
uint32_t sizeIncrement, uint32_t sizeIncrement,
uint32_t offset); uint64_t offset);
ID3D12DescriptorHeap* Get() const; ID3D12DescriptorHeap* Get() const;
D3D12_CPU_DESCRIPTOR_HANDLE GetCPUHandle(uint32_t index) const; D3D12_CPU_DESCRIPTOR_HANDLE GetCPUHandle(uint32_t index) const;
@ -42,7 +42,7 @@ namespace dawn_native { namespace d3d12 {
private: private:
ComPtr<ID3D12DescriptorHeap> mDescriptorHeap; ComPtr<ID3D12DescriptorHeap> mDescriptorHeap;
uint32_t mSizeIncrement; uint32_t mSizeIncrement;
uint32_t mOffset; uint64_t mOffset;
}; };
class DescriptorHeapAllocator { class DescriptorHeapAllocator {

View File

@ -18,11 +18,11 @@
using namespace dawn_native; using namespace dawn_native;
constexpr size_t RingBufferAllocator::kInvalidOffset; constexpr uint64_t RingBufferAllocator::kInvalidOffset;
// Number of basic tests for Ringbuffer // Number of basic tests for Ringbuffer
TEST(RingBufferAllocatorTests, BasicTest) { TEST(RingBufferAllocatorTests, BasicTest) {
constexpr size_t sizeInBytes = 64000; constexpr uint64_t sizeInBytes = 64000;
RingBufferAllocator allocator(sizeInBytes); RingBufferAllocator allocator(sizeInBytes);
// Ensure no requests exist on empty buffer. // Ensure no requests exist on empty buffer.
@ -43,8 +43,8 @@ TEST(RingBufferAllocatorTests, BasicTest) {
// Tests that several ringbuffer allocations do not fail. // Tests that several ringbuffer allocations do not fail.
TEST(RingBufferAllocatorTests, RingBufferManyAlloc) { TEST(RingBufferAllocatorTests, RingBufferManyAlloc) {
constexpr size_t maxNumOfFrames = 64000; constexpr uint64_t maxNumOfFrames = 64000;
constexpr size_t frameSizeInBytes = 4; constexpr uint64_t frameSizeInBytes = 4;
RingBufferAllocator allocator(maxNumOfFrames * frameSizeInBytes); RingBufferAllocator allocator(maxNumOfFrames * frameSizeInBytes);
@ -57,8 +57,8 @@ TEST(RingBufferAllocatorTests, RingBufferManyAlloc) {
// Tests ringbuffer sub-allocations of the same serial are correctly tracked. // Tests ringbuffer sub-allocations of the same serial are correctly tracked.
TEST(RingBufferAllocatorTests, AllocInSameFrame) { TEST(RingBufferAllocatorTests, AllocInSameFrame) {
constexpr size_t maxNumOfFrames = 3; constexpr uint64_t maxNumOfFrames = 3;
constexpr size_t frameSizeInBytes = 4; constexpr uint64_t frameSizeInBytes = 4;
RingBufferAllocator allocator(maxNumOfFrames * frameSizeInBytes); RingBufferAllocator allocator(maxNumOfFrames * frameSizeInBytes);
@ -87,8 +87,8 @@ TEST(RingBufferAllocatorTests, AllocInSameFrame) {
// Tests ringbuffer sub-allocation at various offsets. // Tests ringbuffer sub-allocation at various offsets.
TEST(RingBufferAllocatorTests, RingBufferSubAlloc) { TEST(RingBufferAllocatorTests, RingBufferSubAlloc) {
constexpr size_t maxNumOfFrames = 10; constexpr uint64_t maxNumOfFrames = 10;
constexpr size_t frameSizeInBytes = 4; constexpr uint64_t frameSizeInBytes = 4;
RingBufferAllocator allocator(maxNumOfFrames * frameSizeInBytes); RingBufferAllocator allocator(maxNumOfFrames * frameSizeInBytes);
@ -164,3 +164,14 @@ TEST(RingBufferAllocatorTests, RingBufferSubAlloc) {
EXPECT_TRUE(allocator.Empty()); EXPECT_TRUE(allocator.Empty());
} }
// Checks if ringbuffer sub-allocation does not overflow.
TEST(RingBufferAllocatorTests, RingBufferOverflow) {
Serial serial = 1;
RingBufferAllocator allocator(std::numeric_limits<uint64_t>::max());
ASSERT_EQ(allocator.Allocate(1, serial), 0u);
ASSERT_EQ(allocator.Allocate(std::numeric_limits<uint64_t>::max(), serial + 1),
RingBufferAllocator::kInvalidOffset);
}