Re: [PATCH v2] net: sched: Add support for packet bursting.
From: Toke Høiland-Jørgensen <hidden>
Date: 2021-06-29 14:35:26
Niclas Hedam [off-list ref] writes:
Thanks for the valuable thoughts, Toke. The patch started with me being tasked to try and mitigate timing attacks caused by network latencies. I scouted over the current network stack and didn't find anything that fully matched my use-case. While I now understand that you can actually leverage the slots functionality for this, I would still opt for a new interface and implementation.
So what is the actual use case for this? If this is something you're actually planning to deploy this in production, I'm not sure netem is the best place for it...
I have not done any CPU benchmarks on the slots system, so I'm not approaching this from the practical performance side per se. Instead, I argue for seperation with reference to the Seperation of Concern design principle. The slots functionality is not built/designed to cater security guarantees, and my patch is not built to cater duty cycles, etc.
Separation of concerns is all well and good, but you're still adding this to an existing qdisc (and one mostly used for emulating networks, at that), in a way that will silently disable most of the other functionality. I.e., if the 'bursting' field is set, 'rate' and 'latency' will just silently stop working because you're skipping the code path that uses those. So you'll need to reject invalid combinations at configure time, which AFAICT is any other feature combined with bursting. Also, please add an unlikely() around the check for the bursting parameter to hint the compiler that this is not the most commonly used feature.
If we opt to merge these two functionalities or discard mine, we have to implement some guarantee that the slots functionality won't become significantly slower or complex, which in my opinion is less maintainable than two similar systems. Also, this patch is very limited in lines of code, so maintaining it is pretty trivial.
Maintenance is not just about lines of code, it is also about combination of features (e.g., dealing with things like "my rate limiter stopped working after I turned on this 'security' feature"). At the very least you'll need to clearly document interactions (and refuse invalid combinations as mentioned above). -Toke