[WIP] add cqe unpack#985
Conversation
yumingyue624
requested review from
FangRun2,
Infinite666,
Tarrei,
Wwwzff,
mag1c-h and
ygwpz
as code owners
yumingyue624
force-pushed
the
protocol_pack
branch
from
55012ca to
fb43dc8
Compare
| static constexpr std::size_t kAlignment = 512; | ||
| static constexpr std::size_t kMaxCIDCount = 65536; // 16-bit CID range | ||
| static constexpr std::size_t kMaxROBEntries = 65536; | ||
| static constexpr std::size_t kInvalidROBIndex = SIZE_MAX; |
There was a problem hiding this comment.
Consider defining this magic number (65536) as a named constant for better readability.
|
|
||
| // Dwords 12-15: key[15:0] - 16 bytes key, low-byte aligned | ||
| std::size_t key_len = std::min(r.key.size(), static_cast<std::size_t>(16)); | ||
| if (key_len > 0) { std::memcpy(&dwords[12], r.key.data(), key_len); } | ||
| if (key_len > 0) { std::memcpy(&target[12], r.key.data(), key_len); } | ||
| return Status::OK(); |
There was a problem hiding this comment.
Potential buffer overflow risk. Consider using safer alternatives like strncpy or checking buffer sizes.
|
|
||
| // Dwords 12-15: key[15:0] - 16 bytes key, low-byte aligned | ||
| std::size_t key_len = std::min(r.key.size(), static_cast<std::size_t>(16)); | ||
| if (key_len > 0) { std::memcpy(&dwords[12], r.key.data(), key_len); } | ||
| if (key_len > 0) { std::memcpy(&target[12], r.key.data(), key_len); } | ||
| return Status::OK(); |
There was a problem hiding this comment.
Potential buffer overflow risk. Consider using safer alternatives like strncpy or checking buffer sizes.
| { | ||
| auto& r = static_cast<const KvBatchStoreRequest&>(req); | ||
| return (kSqeDwordCount + r.batch_number * kBatchEntryDwordCount) * sizeof(std::uint32_t); | ||
| } |
There was a problem hiding this comment.
Consider using size_t or uint64_t for size calculations to avoid integer overflow.
| std::size_t PackedSize(const SqeRequest& req) const override | ||
| { | ||
| return kSqeDwordCount * sizeof(std::uint32_t); | ||
| } |
There was a problem hiding this comment.
Consider using size_t or uint64_t for size calculations to avoid integer overflow.
| std::size_t ResponseSize(const SqeRequest& req) const override | ||
| { | ||
| return kCqeDwordCount * sizeof(std::uint32_t); | ||
| } |
There was a problem hiding this comment.
Consider using size_t or uint64_t for size calculations to avoid integer overflow.
| std::size_t PackedSize(const SqeRequest& req) const override | ||
| { | ||
| return kSqeDwordCount * sizeof(std::uint32_t); | ||
| } |
There was a problem hiding this comment.
Consider using size_t or uint64_t for size calculations to avoid integer overflow.
| } | ||
|
|
||
| Status KvStoreSqe::Validate() const | ||
| Status KvStoreSqe::Validate(const std::uint32_t* data) const |
There was a problem hiding this comment.
💡 Suggestion: After removing the size check from Validate, consider adding a null pointer check at the beginning: if (!data) return Status::Error(StatusCode::INVALID_ARGUMENT, "data pointer is null");. This prevents crashes instead of returning proper errors when called with null data.
| class KvRetrieveRequest : public SqeRequest { | ||
| public: | ||
| std::uint32_t cid{0}; | ||
| std::uint16_t cid{0}; |
There was a problem hiding this comment.
💡 Suggestion: KvRetrieveRequest declares std::uint16_t cid{0}; but it inherits from SqeRequest which already has this field at line 43. This duplication is unnecessary and could cause confusion. Consider removing the redundant declaration.
| if (!sqe) { | ||
| return Status::Error(StatusCode::INVALID_ARGUMENT, "unknown SQE opcode"); | ||
| } | ||
|
|
There was a problem hiding this comment.
sge.addr is std::uint64_t (a virtual address), but it's cast directly to std::uint32_t* via reinterpret_cast. On 64-bit systems, ensure the address fits in the pointer type. The proper cast sequence should be: reinterpret_cast<std::uint32_t*>(static_cast<uintptr_t>(sge.addr))
yumingyue624
force-pushed
the
protocol_pack
branch
3 times, most recently
from
8f8e29b to
a411b7d
Compare
yumingyue624
changed the title
yumingyue624
force-pushed
the
protocol_pack
branch
2 times, most recently
from
2d47e4f to
338d295
Compare
yumingyue624
force-pushed
the
protocol_pack
branch
2 times, most recently
from
1a5a156 to
4e95153
Compare
yumingyue624
changed the title
yumingyue624
force-pushed
the
protocol_pack
branch
from
4e95153 to
78c128d
Compare
yumingyue624
force-pushed
the
protocol_pack
branch
from
78c128d to
430c908
Compare
2 participants
Purpose
Implement BufferManager for slot-based memory management with IndexPool,
and unify SQE packing/CQE unpacking into a single KvProtocol abstraction.
Modifications
HOST (malloc), HOST_PINNED (aclrtMallocHost + aclrtHostRegister),
and ASCEND_DEVICE (aclrtMalloc) memory types via AscendBuffer.
interfaces, replacing separate SqeManager and CqeManager.
with PackRequest, UnpackResponse, and GetCid interfaces.
Test
BufferManagerTest (17 cases) and KvProtocolPackTest (10 cases) all passed.