DataLinkLayer Experiment

[we-make-software]

Body

Hello everyone,

I’m experimenting with a kernel-level DataLinkLayer in C.
You can view the full code here: DataLinkLayer.c

I’m especially interested in feedback on:

• Buffer pool management
• Magic number usage
• Mutex and workqueue handling
• General code style and readability

Any suggestions for improvements or potential issues would be highly appreciated.

Thanks!

Guidelines

• I have read and understood this category's guidelines before making this post.

[we-make-software]

Thx sir.

you gave me a great idea.

Thx sir.

i will update whit the new idea.. later.

[tmilost]

---

1. Buffer Pool Management

• Design:

  • Using per-destination and per-source buffer pools, each with multiple SocketBufferPool sub-pools, is a scalable model.
  • Buffer pools are dynamically resized (creator/decrease), which is good for memory management under varying workloads.

• Strengths:

  • Use of BUFFER_INCREMENT and increaser factor for controlling pool growth is reasonable.
  • Pool cleanup via list_for_each_entry_safe and freeing each sk_buff looks thorough.

• Suggestions:

  • Fragmentation/Preallocation: In certain workloads, rapid resizing of pools up and down can cause memory fragmentation or allocate unnecessarily. Consider hysteresis (smaller increments for trimming, larger for growth).
  • Reinitialization: Ensure that after cleaning, pools are reinitialized to a valid (but minimal) state. The code resets counts but may benefit from recreating minimal initial buffers.
  • Error Handling: When buffer allocation (alloc_skb) fails, you properly break/return NULL, but it might be worth logging such events for debugging under memory pressure.

2. Magic Number Usage

• Good Practice:
  • The use of well-named macros (e.g., SOCKET_POOLS, BUFFER_INCREMENT, SKB_SIZE) makes the code readable and maintainable.
  • The only potential "magic" values are +1 offsets and increments in calculations (e.g., ((pool->increaser+1)*BUFFER_INCREMENT)+1). Consider breaking these into further macros or at least code comments, clarifying the reasoning behind the +1.
• Suggestion:
  • Document the rationale for +1 everywhere it's used (capacity, zero-indexing, etc.).
  • Timeout values: SOURCE_TIMEOUT_MS, DEST_TIMEOUT_MS, etc., should likewise be documented (why those durations?).

3. Mutex and Workqueue Handling

• Strengths:

  • Fine-grained locking: Mutex per SocketBufferPool and per BufferPool allows for parallelism and minimizes contention.
  • Use of mutex_trylock is appropriate for "opportunistically" taking buffers.
  • Proper synchronization for canceling queued/delayed work before freeing (avoids use-after-free).

• Suggestions:

  • Error Handling: Always check for lock acquisition success before proceeding with sensitive operations.
  • Unlock on All Paths: Double-check all lock acquisitions followed by unconditional unlocks for paths that may early-return or break.
  • Workqueue Reuse: You INIT_WORK with NULL after canceling, which is good for safety, but ensure no other references remain.

4. General Code Style and Readability

• Strengths:
  • Structure/Field naming is generally clear and consistent.
  • Macros make the code easier to read and change.
  • Each function shows its intent (with occasional exceptions due to macro-generated function signatures).
• Suggestions:
  • Commenting: Add more comments documenting why (not just what) — explain choices for increments, pool resizing, timeouts, buffer splitting.
  • Magic Values: As above, explain any “+1”, 6 (Ethernet MAC address size), and 12.
  • Error Flows: Consider adding tracepoints or debug prints for critical state changes, especially buffer exhaustion or workqueue scheduling behaviors.
  • Consistent Bracing: Ensure consistent formatting for control structures (if, for), especially in multi-line blocks.

5. General Suggestions and Potential Issues

• Memory Pressure: If buffer allocations fail frequently, you may fragment memory or run out of sk_buffs. Consider fallback and reporting/metrics for such events.
• Race Conditions:
  • Delayed work and buffer cleanup are generally well synchronized, but be wary of timing edge cases where delayed work runs just after pool is freed. Your use of cancel_*_work_sync() addresses much of this, but it's an area to monitor under stress.
• Workqueue Management:
  • You use delayed work for trimming/expanding pools, which is a good pattern in the kernel, but ensure no unhandled overlap if timers race.
• Scalability:
  • For a small/medium set of sources/destinations, this model works well. If you expect thousands, profiling/memory consideration may be needed.
• Testing:
  • Stress tests—simulate rapid connect/disconnect and allocation exhaustion to ensure stability.

---

Summary Table

Aspect	Strengths	Suggestions
Buffer Pool Management	Dynamic, per-dest/source, proper freeing	Hysteresis, error/event logging, init minimal state
Magic Numbers	Good use of macros	Explain "+1", timeouts, constants
Mutex/Workqueue	Granular, proper cancel/init	Double-check locking paths, no lingering references
Code Style/Readability	Naming, macro use, function intent clear	More comments (why), handle error flows, formatting
Improvements/Pitfalls	Scalable, potential for kernel integration	Monitor memory/race, add metrics/tracing, test edges

---

[Ralphbaer]

Hey! This is some seriously ambitious stuff, kernel networking code is tough. I've been looking through your DataLinkLayer implementation and have some thoughts that might help:

Stuff That Needs Attention

The skb reference thing looks sketchy

When you do this in DirectReceiver:

skb_get(skb);
INIT_WORK(&frw->wt,ContinueReceiver);
schedule_work(&frw->wt);

You're grabbing a reference to the skb, but there's no guarantee the work scheduler sees that increment before it runs. Toss in smp_wmb() after the skb_get() to be safe. Also, you're never actually freeing the skb in ContinueReceiver - that's a leak waiting to happen. Add kfree_skb(skb) at the end.

Double device lookup is wasteful

In HardwareReceiver you're calling SearchDevice twice - once there and again inside InitDevice. Just call InitDevice directly since it already handles the "device exists" case. No point walking the list twice.

GFP_KERNEL in packet receive path? Nah

struct ForwardReceiverContent*frc=kmem_cache_alloc(ForwardReceiverContentCachie, GFP_KERNEL);

This can sleep, which you really don't want in RX context. Change it to GFP_ATOMIC. Yeah it might fail more under memory pressure, but that's better than sleeping in softirq.

The Locking Situation

Your mutex usage in InitDevice is holding the lock way too long. You're doing kmalloc, memcpy, all that stuff while holding h->l. Problem is, between when you drop the lock and when you add to the list, someone else could add the same device. Classic TOCTOU bug.

Try something like:

// Do your allocations first
struct Device*d=kmalloc(sizeof(*d), GFP_ATOMIC);
if (!d) return;

// Set up the device
memcpy(d->dll.Address, eth_hdr(skb)->h_source, 6);
INIT_LIST_HEAD(&d->lh);

// NOW lock and double-check
mutex_lock(&h->l);
struct Device*existing = SearchDevice(skb,h);
if (existing) {
    mutex_unlock(&h->l);
    kfree(d);
    // use existing instead
    return;
}
list_add(&d->lh, &h->dlh);
mutex_unlock(&h->l);

Honestly though, for device lookups you might want RCU instead of mutexes. It's mostly reads, right? Mutexes are kinda heavy for that.

Couple Other Things

Your module exit doesn't wait for work items to finish. Add flush_scheduled_work() before you start tearing things down, otherwise you'll have use-after-free if work items are still running.

The timeout being a static global is fine for now, but you might want to make it a module parameter so people can tune it without recompiling:

static uint device_timeout_minute = 10;
module_param(device_timeout_minute, uint, 0644);

Overall this is pretty cool work. Kernel networking is genuinely hard and you're clearly getting the core concepts. Just needs some polish on the concurrency stuff. Have you tested this under load? I'd be curious what happens if you hammer it with packets while devices are timing out.