Skip to content

[Store] Add Go dummy client benchmark support#3015

Draft
BGQ99 wants to merge 5 commits into
kvcache-ai:mainfrom
BGQ99:pr29-clean
Draft

[Store] Add Go dummy client benchmark support#3015
BGQ99 wants to merge 5 commits into
kvcache-ai:mainfrom
BGQ99:pr29-clean

Conversation

@BGQ99

@BGQ99 BGQ99 commented Jul 21, 2026

Copy link
Copy Markdown

Description

Related RFC: #3068

This PR manually ports the Go DummyClient benchmark and supporting Store APIs
from zchuango/Mooncake#29
onto the latest kvcache-ai/Mooncake:main.

It allows independent Go worker processes to use DummyClient and registered
shared-memory buffers while a standalone mooncake_client owns the RealClient
and performs the actual Store operations.

What changed

  • Preserve the existing C API entry points for RealClient:
    mooncake_store_create() and the seven-argument
    mooncake_store_setup(...).
  • Add mooncake_store_create_with_type(...) and
    mooncake_store_setup_dummy(...) for DummyClient.
  • Add a Go-native ClientType and NewWithType without exposing CGo types
    through the public Go API.
  • Preserve the existing seven-argument Go Setup API for RealClient.
  • Add the dedicated Go
    DummySetup(memPoolSize, localBufferSize, serverAddress, ipcSocketPath) API.
  • Add DummyClient registered-buffer enumeration and hot-cache pointer queries
    through the C and Go APIs.
  • Implement single-buffer DummyClient put_from RPC forwarding and register
    the handler in the standalone client and test RPC server.
  • Add a multi-process Go DummyClient write/read benchmark.
  • Pass replica-num through ReplicateConfig and remove benchmark options
    that were not reflected in the executed workload.
  • Propagate operation, verification, and worker failures to a non-zero
    benchmark exit status.
  • Add buffer overflow, capacity, and memory-access validation.
  • Remove the unused mmap implementation and unnecessary dependencies.

Scope and differences from the source PR

  • mooncake-store/benchmarks/stress_cluster_bench.cpp is intentionally
    excluded.
  • Source commits 6e9a856 and c86d3df only modify the excluded stress
    benchmark, so they contribute no files here.
  • The Go setup API is split into Setup and DummySetup instead of extending
    the existing Setup signature.
  • The existing C API is preserved, with additional entry points for
    DummyClient.
  • No CI, workflow, Claude, documentation, Transfer Engine, EP, or unrelated
    repository changes are included.

Remaining RFC questions

  • Whether registered-buffer introspection should remain part of the public API,
    including its ownership, lifetime, concurrency, and invalidation contract.

Module

  • Transfer Engine (mooncake-transfer-engine)
  • Mooncake Store (mooncake-store)
  • Mooncake EP (mooncake-ep)
  • Mooncake PG (mooncake-pg)
  • Integration (mooncake-integration)
  • P2P Store (mooncake-p2p-store)
  • Python Wheel (mooncake-wheel)
  • Common (mooncake-common)
  • Mooncake RL (mooncake-rl)
  • CI/CD
  • Docs
  • Other

Type of Change

  • Bug fix
  • New feature
  • Refactor
  • Breaking change
  • Documentation update
  • Performance improvement
  • Other

The existing C and Go RealClient setup APIs remain source-compatible.
DummyClient support is exposed through additional C API entry points and the
dedicated Go DummySetup(...) method.

How Has This Been Tested?

cmake -S . -B build \
  -DWITH_TE=ON -DWITH_STORE=ON -DWITH_STORE_RUST=OFF \
  -DBUILD_EXAMPLES=ON -DBUILD_UNIT_TESTS=OFF \
  -DUSE_TCP=ON -DUSE_HTTP=ON -DUSE_ETCD=OFF \
  -DSTORE_USE_ETCD=OFF -DUSE_CUDA=OFF -DUSE_UB=OFF -DUSE_TENT=OFF

cmake --build build \
  --target mooncake_store mooncake_master mooncake_client -j3

mkdir -p build/go

source mooncake-store/go/build.sh build build/go OFF OFF ON OFF

go build \
  -o ../../build/go/dummy-clients-test \
  ./examples/dummy_clients_test

Sourcing build.sh preserves its CGo include and library settings and changes
the working directory to mooncake-store/go, allowing the following dummy
benchmark build to use the same configuration.

The benchmark was run against an isolated master and client:

MC_SERVERADDRESS=127.0.0.1:51052 \
MC_IPCSOCKETPATH='@mooncake_client_51052.sock' \
../../build/go/dummy-clients-test \
  -workers=1 -value-size=4096 -num-keys=2 -replica-num=1 \
  -local-buffer-size=4194304 -wait-seconds=0 -phase=write

MC_SERVERADDRESS=127.0.0.1:51052 \
MC_IPCSOCKETPATH='@mooncake_client_51052.sock' \
../../build/go/dummy-clients-test \
  -workers=1 -value-size=4096 -num-keys=2 -replica-num=1 \
  -local-buffer-size=4194304 -wait-seconds=0 -phase=read

Test results:

  • Unit tests pass
  • Integration tests pass (if applicable)
  • Manual testing done

Manual results:

  • mooncake_store, mooncake_master, and mooncake_client built
    successfully.
  • The existing Go examples/basic program built successfully.
  • Basic runtime completed connection, Put, Exists, GetSize, and Get.
  • Dummy write completed 2/2 operations with 0 failures.
  • Dummy read completed 2/2 operations with 0 failures and 0 verification
    errors.
  • Successful benchmark runs returned exit status 0.
  • Put/Get failures and worker failures produced a non-zero exit status.
  • Deliberately corrupted data produced Verify Errors: 1 and non-zero worker
    and master exit statuses.
  • Rust cargo check --locked passed against the compatibility-preserving
    C API.
  • git diff --check upstream/main...HEAD passed.
  • The final PR diff contains only the intended 11 files.

Review Follow-up

The draft has been updated to address automated and maintainer review feedback:

  • kept CGo types internal to the Go binding package;
  • fixed registered-buffer unregistration so index shifts cannot skip buffers;
  • removed unused mmap code and dependencies;
  • validated buffer sizes, multiplication overflow, and platform integer limits;
  • replaced fixed-size unsafe array conversions with unsafe.Slice;
  • replaced the platform-dependent CPU mask with a byte-based mask;
  • passed replica-num through ReplicateConfig;
  • removed benchmark options that did not affect the workload;
  • propagated operation, verification, and worker failures to the process exit
    status;
  • preserved the existing C API and added separate DummyClient entry points.

Checklist

  • I have performed a self-review of my own code
  • I have formatted my code using ./scripts/code_format.sh
  • I have run pre-commit run --all-files and all hooks pass
  • I have updated the documentation (if applicable)
  • I have added tests to prove my changes are effective
  • For changes >500 LOC: I have filed an RFC issue

gofmt, targeted pre-commit hooks, and
./scripts/code_format.sh --check -b upstream/main passed. A full
pre-commit run --all-files currently reports unrelated repository-wide
baseline issues and was not treated as a passing check.

AI Assistance Disclosure

  • No AI tools were used
  • AI tools were used (specify below)

AI assistance was used to inspect the source PR, manually port the selected
changes onto the current upstream base, isolate unrelated branch history,
diagnose build and runtime issues, address review feedback, run local
validation, and draft the RFC and PR description.

The human submitter remains responsible for reviewing, understanding, and
defending the final changes.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a multi-process benchmark tool and extends the Go bindings to support the DummyClient store type, including APIs for querying and managing registered buffers. The code review identified several critical issues: a hardcoded absolute path in CFLAGS that breaks portability, exposure of internal C types in the public Go API, and an index-shifting bug in UnregisterAllBuffers that leads to skipped buffers. Additionally, the feedback highlights dead code, potential out-of-bounds panics in the benchmark tool, outdated unsafe pointer patterns, and platform-dependent portability bugs in the CPU affinity binding implementation.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +29 to +32
const (
MOONCAKE_CLIENT_REAL = C.MOONCAKE_CLIENT_REAL
MOONCAKE_CLIENT_DUMMY = C.MOONCAKE_CLIENT_DUMMY
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Exposing C types (like C.mooncake_client_type_t) in the public Go API is a major anti-pattern in cgo design. It prevents external packages from easily using or declaring variables of this type. Instead, define a custom Go type (e.g., ClientType) and map the constants to it.

Suggested change
const (
MOONCAKE_CLIENT_REAL = C.MOONCAKE_CLIENT_REAL
MOONCAKE_CLIENT_DUMMY = C.MOONCAKE_CLIENT_DUMMY
)
type ClientType int
const (
MOONCAKE_CLIENT_REAL ClientType = C.MOONCAKE_CLIENT_REAL
MOONCAKE_CLIENT_DUMMY ClientType = C.MOONCAKE_CLIENT_DUMMY
)

Comment on lines +45 to 53
// NewWithType creates a new Store instance with the specified client type.
// Use MOONCAKE_CLIENT_REAL or MOONCAKE_CLIENT_DUMMY.
func NewWithType(clientType C.mooncake_client_type_t) (*Store, error) {
h := C.mooncake_store_create(clientType)
if h == nil {
return nil, ErrStoreNil
}
return &Store{handle: h}, nil
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Update NewWithType to use the newly defined Go ClientType instead of the internal C.mooncake_client_type_t to avoid exposing cgo types in the public API.

Suggested change
// NewWithType creates a new Store instance with the specified client type.
// Use MOONCAKE_CLIENT_REAL or MOONCAKE_CLIENT_DUMMY.
func NewWithType(clientType C.mooncake_client_type_t) (*Store, error) {
h := C.mooncake_store_create(clientType)
if h == nil {
return nil, ErrStoreNil
}
return &Store{handle: h}, nil
}
// NewWithType creates a new Store instance with the specified client type.
// Use MOONCAKE_CLIENT_REAL or MOONCAKE_CLIENT_DUMMY.
func NewWithType(clientType ClientType) (*Store, error) {
h := C.mooncake_store_create(C.mooncake_client_type_t(clientType))
if h == nil {
return nil, ErrStoreNil
}
return &Store{handle: h}, nil
}

Comment on lines +607 to +626
func (s *Store) UnregisterAllBuffers() (int, error) {
if s.handle == nil {
return 0, ErrStoreNil
}
count, err := s.RegisteredBufferCount()
if err != nil {
return 0, err
}
unregistered := 0
for i := 0; i < count; i++ {
bufInfo, err := s.RegisteredBufferAt(i)
if err != nil {
continue
}
if err := s.UnregisterBuffer(bufInfo.Ptr); err == nil {
unregistered++
}
}
return unregistered, nil
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

There is an index shifting bug in UnregisterAllBuffers. When UnregisterBuffer is called, the underlying list of registered buffers shrinks, causing the remaining buffers to shift indices. Iterating forward with i++ will skip some buffers and eventually cause out-of-bounds index errors. To fix this, collect all buffer pointers first, and then unregister them.

func (s *Store) UnregisterAllBuffers() (int, error) {
	if s.handle == nil {
		return 0, ErrStoreNil
	}
	count, err := s.RegisteredBufferCount()
	if err != nil {
		return 0, err
	}
	ptrs := make([]uintptr, 0, count)
	for i := 0; i < count; i++ {
		bufInfo, err := s.RegisteredBufferAt(i)
		if err == nil {
			ptrs = append(ptrs, bufInfo.Ptr)
		}
	}
	unregistered := 0
	for _, ptr := range ptrs {
		if err := s.UnregisterBuffer(ptr); err == nil {
			unregistered++
		}
	}
	return unregistered, nil
}

Comment on lines +389 to +419
func allocateAndRegisterBufferViaMmap(s *store.Store, workerID int) (mmap.MMap, uintptr) {
bufSize := *flagBatchSize * *flagValueSize * 16
log.Printf("[WORKER-%d] Buffer size required: %d MB", workerID, bufSize/(1024*1024))

pageSize := int64(syscall.Getpagesize())
alignedSize := (int64(bufSize) + pageSize - 1) / pageSize * pageSize
log.Printf("[WORKER-%d] Aligned buffer size: %d bytes (page size: %d)", workerID, alignedSize, pageSize)

filePath := fmt.Sprintf("/dev/shm/mooncake_dummy_client%d.dat", workerID) // 每个worker独立文件
f, err := os.OpenFile(filePath, os.O_RDWR|os.O_CREATE, 0644)
if err != nil {
log.Fatalf("[WORKER-%d] Open file failed: %v", workerID, err)
}
defer f.Close()

if err := f.Truncate(alignedSize); err != nil {
log.Fatalf("[WORKER-%d] Truncate file failed: %v", workerID, err)
}

mm, err := mmap.MapRegion(f, int(alignedSize), mmap.RDWR, 0, 0)
if err != nil {
log.Fatalf("[WORKER-%d] mmap.MapRegion failed: %v", workerID, err)
}

ptr := uintptr(unsafe.Pointer(&mm[0]))
if err := s.RegisterBuffer(ptr, uint64(bufSize)); err != nil {
mm.Unmap()
log.Fatalf("[WORKER-%d] RegisterBuffer failed: %v", workerID, err)
}
return mm, ptr
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The function allocateAndRegisterBufferViaMmap is defined but never called anywhere in the codebase. If this is dead code, it should be removed along with its unused import "github.com/edsrzf/mmap-go" and the corresponding dependency in go.mod to keep the codebase clean and minimize external dependencies.

Comment on lines +437 to +440
func writePhase(s *store.Store, buf []byte, workerID int) {
keyOffset, keysPerWorker := calculateKeyRange(uint(workerID), *flagWorkers, *flagNumKeys)
valueSize := int(*flagValueSize)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

When using a pre-registered buffer, its size (bufInfo.Size) might be smaller than the required *flagBatchSize * *flagValueSize. This can cause an out-of-bounds panic when accessing buf[:valueSize]. We should add a check to ensure len(buf) is sufficient.

Suggested change
func writePhase(s *store.Store, buf []byte, workerID int) {
keyOffset, keysPerWorker := calculateKeyRange(uint(workerID), *flagWorkers, *flagNumKeys)
valueSize := int(*flagValueSize)
func writePhase(s *store.Store, buf []byte, workerID int) {
requiredSize := int(*flagBatchSize * *flagValueSize)
if len(buf) < requiredSize {
log.Fatalf("[WORKER-%d] Buffer size too small: got %d, need %d", workerID, len(buf), requiredSize)
}
keyOffset, keysPerWorker := calculateKeyRange(uint(workerID), *flagWorkers, *flagNumKeys)
valueSize := int(*flagValueSize)

Comment on lines +479 to +482
func readPhase(s *store.Store, buf []byte, workerID int) {
keyOffset, keysPerWorker := calculateKeyRange(uint(workerID), *flagWorkers, *flagNumKeys)
valueSize := int(*flagValueSize)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

When using a pre-registered buffer, its size (bufInfo.Size) might be smaller than the required *flagBatchSize * *flagValueSize. This can cause an out-of-bounds panic when accessing buf[offset:offset+valueSize]. We should add a check to ensure len(buf) is sufficient.

Suggested change
func readPhase(s *store.Store, buf []byte, workerID int) {
keyOffset, keysPerWorker := calculateKeyRange(uint(workerID), *flagWorkers, *flagNumKeys)
valueSize := int(*flagValueSize)
func readPhase(s *store.Store, buf []byte, workerID int) {
requiredSize := int(*flagBatchSize * *flagValueSize)
if len(buf) < requiredSize {
log.Fatalf("[WORKER-%d] Buffer size too small: got %d, need %d", workerID, len(buf), requiredSize)
}
keyOffset, keysPerWorker := calculateKeyRange(uint(workerID), *flagWorkers, *flagNumKeys)
valueSize := int(*flagValueSize)

Comment on lines +580 to +597
func FillBuffer(buf []byte, seed uint64) {
pattern := seed * 0x9E3779B97F4A7C15
numWords := len(buf) / 8

ptr := (*[1 << 30]uint64)(unsafe.Pointer(&buf[0]))
for i := 0; i < numWords; i++ {
ptr[i] = pattern + uint64(i)
}

// 处理剩余字节
remaining := len(buf) % 8
if remaining > 0 {
lastWord := pattern + uint64(numWords)
for i := 0; i < remaining; i++ {
buf[numWords*8+i] = byte(lastWord >> (i * 8))
}
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using (*[1 << 30]uint64)(unsafe.Pointer(&buf[0])) is an outdated and unsafe pattern for converting a slice to a pointer/array. Since Go 1.17 (and this project uses Go 1.23), you should use unsafe.Slice which is safer and more idiomatic. Additionally, we should check if len(buf) == 0 to prevent an out-of-bounds panic when accessing &buf[0].

func FillBuffer(buf []byte, seed uint64) {
	if len(buf) == 0 {
		return
	}
	pattern := seed * 0x9E3779B97F4A7C15
	numWords := len(buf) / 8

	ptr := unsafe.Slice((*uint64)(unsafe.Pointer(&buf[0])), numWords)
	for i := 0; i < numWords; i++ {
		ptr[i] = pattern + uint64(i)
	}

	// 处理剩余字节
	remaining := len(buf) % 8
	if remaining > 0 {
		lastWord := pattern + uint64(numWords)
		for i := 0; i < remaining; i++ {
			buf[numWords*8+i] = byte(lastWord >> (i * 8))
		}
	}
}

Comment on lines +600 to +631
func CheckBuffer(buf []byte, seed uint64) bool {
if len(buf) == 0 {
return false
}

pattern := seed * 0x9E3779B97F4A7C15
numWords := len(buf) / 8

ptr := (*[1 << 30]uint64)(unsafe.Pointer(&buf[0]))
for i := 0; i < numWords; i++ {
if ptr[i] != pattern+uint64(i) {
log.Printf("CheckBuffer: Mismatch at word %d: expected %d, got %d",
i, pattern+uint64(i), ptr[i])
return false
}
}

// 验证剩余字节
remaining := len(buf) % 8
if remaining > 0 {
lastWord := pattern + uint64(numWords)
for i := 0; i < remaining; i++ {
expected := byte(lastWord >> (i * 8))
if buf[numWords*8+i] != expected {
log.Printf("CheckBuffer: Mismatch at byte %d: expected %d, got %d",
numWords*8+i, expected, buf[numWords*8+i])
return false
}
}
}
return true
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using (*[1 << 30]uint64)(unsafe.Pointer(&buf[0])) is an outdated and unsafe pattern. Since Go 1.17, you should use unsafe.Slice which is safer and more idiomatic. Additionally, we should check if len(buf) == 0 to prevent an out-of-bounds panic when accessing &buf[0].

func CheckBuffer(buf []byte, seed uint64) bool {
	if len(buf) == 0 {
		return false
	}

	pattern := seed * 0x9E3779B97F4A7C15
	numWords := len(buf) / 8

	ptr := unsafe.Slice((*uint64)(unsafe.Pointer(&buf[0])), numWords)
	for i := 0; i < numWords; i++ {
		if ptr[i] != pattern+uint64(i) {
			log.Printf("CheckBuffer: Mismatch at word %d: expected %d, got %d",
				i, pattern+uint64(i), ptr[i])
			return false
		}
	}

	// 验证剩余字节
	remaining := len(buf) % 8
	if remaining > 0 {
		lastWord := pattern + uint64(numWords)
		for i := 0; i < remaining; i++ {
			expected := byte(lastWord >> (i * 8))
			if buf[numWords*8+i] != expected {
				log.Printf("CheckBuffer: Mismatch at byte %d: expected %d, got %d",
					numWords*8+i, expected, buf[numWords*8+i])
				return false
			}
		}
	}
	return true
}

Comment on lines +700 to +718
// getCurrentAffinityList 获取当前进程的 CPU 亲和性列表
func getCurrentAffinityList() (string, int, error) {
mask := make([]C.ulong, cpuSetSizeBytes/8)

result := C.get_cpu_affinity(0, &mask[0], C.size_t(cpuSetSizeBytes))
if result != 0 {
return "", 0, fmt.Errorf("sched_getaffinity failed: result=%d", result)
}

set := make(map[int]bool)
for i := 0; i < cpuSetSizeBytes*8; i++ {
wordIdx := i / (8 * 8)
bitIdx := i % (8 * 8)
if mask[wordIdx]&(1<<C.ulong(bitIdx)) != 0 {
set[i] = true
}
}
return setToString(set), len(set), nil
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using []C.ulong with size cpuSetSizeBytes/8 assumes that C.ulong is always 8 bytes. If compiled on a platform where unsigned long is 4 bytes, this will allocate only 64 bytes instead of 128, leading to a buffer overread in C.get_cpu_affinity which expects 128 bytes. To make this robust and portable, allocate a []byte of size cpuSetSizeBytes (128) and cast it to *C.ulong when calling C.

func getCurrentAffinityList() (string, int, error) {
	mask := make([]byte, cpuSetSizeBytes)

	result := C.get_cpu_affinity(0, (*C.ulong)(unsafe.Pointer(&mask[0])), C.size_t(cpuSetSizeBytes))
	if result != 0 {
		return "", 0, fmt.Errorf("sched_getaffinity failed: result=%d", result)
	}

	set := make(map[int]bool)
	for i := 0; i < cpuSetSizeBytes*8; i++ {
		wordIdx := i / 8
		bitIdx := i % 8
		if mask[wordIdx]&(1<<bitIdx) != 0 {
			set[i] = true
		}
	}
	return setToString(set), len(set), nil
}

Comment on lines +721 to +787
func bindProcessAffinity(cpuList string) cpuBindResult {
result := cpuBindResult{
Pid: os.Getpid(),
TargetList: cpuList,
}

if runtime.GOOS != "linux" {
result.ErrorMessage = "CPU binding only supported on Linux"
return result
}

if cpuList == "" {
result.ErrorMessage = "empty cpu list"
return result
}

targetSet := parseRangeToSet(cpuList)
if len(targetSet) == 0 {
result.ErrorMessage = "no valid CPU in list"
return result
}

_, beforeCount, _ := getCurrentAffinityList()
result.BeforeCount = beforeCount

mask := make([]C.ulong, cpuSetSizeBytes/8)
for cpu := range targetSet {
if cpu >= cpuSetSizeBytes*8 {
log.Printf("[CPU-BIND] Warning: CPU %d exceeds mask size %d, ignored", cpu, cpuSetSizeBytes*8)
continue
}
wordIdx := cpu / (8 * 8)
bitIdx := cpu % (8 * 8)
mask[wordIdx] |= 1 << C.ulong(bitIdx)
}

cResult := C.set_cpu_affinity(0, &mask[0], C.size_t(cpuSetSizeBytes))
if cResult != 0 {
result.ErrorMessage = fmt.Sprintf("sched_setaffinity failed: result=%d", cResult)
return result
}

afterList, afterCount, err := getCurrentAffinityList()
if err != nil {
result.ErrorMessage = fmt.Sprintf("getaffinity after failed: %v", err)
return result
}

result.Success = true
result.ActualList = afterList
result.AfterCount = afterCount

actualSet := parseRangeToSet(afterList)
match := true
for cpu := range targetSet {
if !actualSet[cpu] {
match = false
break
}
}
if len(actualSet) != len(targetSet) {
match = false
}
result.ActualMatches = match

return result
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using []C.ulong with size cpuSetSizeBytes/8 assumes that C.ulong is always 8 bytes. If compiled on a platform where unsigned long is 4 bytes, this will allocate only 64 bytes instead of 128, leading to a buffer overread in C.set_cpu_affinity which expects 128 bytes. To make this robust and portable, allocate a []byte of size cpuSetSizeBytes (128) and cast it to *C.ulong when calling C.

func bindProcessAffinity(cpuList string) cpuBindResult {
	result := cpuBindResult{
		Pid:        os.Getpid(),
		TargetList: cpuList,
	}

	if runtime.GOOS != "linux" {
		result.ErrorMessage = "CPU binding only supported on Linux"
		return result
	}

	if cpuList == "" {
		result.ErrorMessage = "empty cpu list"
		return result
	}

	targetSet := parseRangeToSet(cpuList)
	if len(targetSet) == 0 {
		result.ErrorMessage = "no valid CPU in list"
		return result
	}

	_, beforeCount, _ := getCurrentAffinityList()
	result.BeforeCount = beforeCount

	mask := make([]byte, cpuSetSizeBytes)
	for cpu := range targetSet {
		if cpu >= cpuSetSizeBytes*8 {
			log.Printf("[CPU-BIND] Warning: CPU %d exceeds mask size %d, ignored", cpu, cpuSetSizeBytes*8)
			continue
		}
		wordIdx := cpu / 8
		bitIdx := cpu % 8
		mask[wordIdx] |= 1 << bitIdx
	}

	cResult := C.set_cpu_affinity(0, (*C.ulong)(unsafe.Pointer(&mask[0])), C.size_t(cpuSetSizeBytes))
	if cResult != 0 {
		result.ErrorMessage = fmt.Sprintf("sched_setaffinity failed: result=%d", cResult)
		return result
	}

	afterList, afterCount, err := getCurrentAffinityList()
	if err != nil {
		result.ErrorMessage = fmt.Sprintf("getaffinity after failed: %v", err)
		return result
	}

	result.Success = true
	result.ActualList = afterList
	result.AfterCount = afterCount

	actualSet := parseRangeToSet(afterList)
	match := true
	for cpu := range targetSet {
		if !actualSet[cpu] {
			match = false
			break
		}
	}
	if len(actualSet) != len(targetSet) {
		match = false
	}
	result.ActualMatches = match

	return result
}

Comment on lines +69 to +83
// Global flags - 统一使用 uint64 类型避免类型不匹配问题
var (
flagWorkers = flag.Uint64("workers", 4, "Number of worker processes")
flagValueSize = flag.Uint64("value-size", 4*1024*1024, "Size of each value in bytes")
flagNumKeys = flag.Uint64("num-keys", 100, "Number of keys to write/read")
flagBatchSize = flag.Uint64("batch-size", 32, "Batch size for operations")
flagIsWorker = flag.Bool("worker", false, "Run as worker process")
flagWorkerID = flag.Uint64("worker-id", 0, "Worker process ID")
flagPhase = flag.String("phase", "write", "Phase: write/read/verify/all")
flagReplicaNum = flag.Uint64("replica-num", 1, "Number of replicas for each object")
flagWaitSeconds = flag.Uint64("wait-seconds", 5, "Seconds to wait between phases")
flagGlobalSize = flag.Uint64("global-size", 512*1024*1024, "Global segment size in bytes")
flagLocalBufferSize = flag.Uint64("local-buffer-size", 128*1024*1024, "Local buffer size in bytes")
flagPutBufferSize = flag.Uint64("put-buffer-size", 512*1024*1024, "Put buffer size in bytes")
)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Several advertised benchmark parameters are not reflected in the executed workload:
-replica-num is never placed into a ReplicateConfig; PutFrom always receives nil, so writes use the default replica count.
-batch-size does not invoke BatchPutFrom or BatchGetInto; operations remain single-key calls.
-global-size is logged and forwarded to workers but is never used during setup.
This makes the reported configuration differ from the workload actually measured. Please wire these flags into the corresponding APIs, or remove/rename them so the benchmark results are not misleading.


// 4. 根据阶段执行操作
log.Printf("[WORKER-%d] Step 4/5: Executing %s phase...", workerID, *flagPhase)
executePhase(s, bufBytes, int(workerID))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: Operation and verification failures do not affect the process exit status. writePhase and readPhase only increment counters and log failures, while the worker still returns successfully. The master also only prints a warning when workers fail.
As a result, a run where every Put/Get fails—or where data verification fails—can still exit with status 0 and be treated as a successful benchmark by scripts or CI. Please propagate phase errors and return a non-zero exit status whenever failed, failedOps, verifyErrors, or failedCount is non-zero.

@kvcache-ai kvcache-ai deleted a comment from gemini-code-assist Bot Jul 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants