On Wed, May 29, 2024 at 08:00:35PM +0200, Bernd Schubert wrote: > From: Bernd Schubert <bschubert@xxxxxxx> > > This adds support for uring communication between kernel and > userspace daemon using opcode the IORING_OP_URING_CMD. The basic > appraoch was taken from ublk. The patches are in RFC state, > some major changes are still to be expected. > First, thanks for tackling this, this is a pretty big project and pretty important, you've put a lot of work into this and it's pretty great. A few things that I've pointed out elsewhere, but bear repeating and keeping in mind for the entire patch series. 1. Make sure you've got changelogs. There's several patches that just don't have changelogs. I get things where it's like "add a mmap interface", but it would be good to have an explanation as to why you're adding it and what we hope to get out of that change. 2. Again as I stated elsewhere, you add a bunch of structs and stuff that aren't related to the current patch, which makes it difficult for me to work out what's needed or how it's used, so I go back and forth between the code and the patch a lot, and I've probably missed a few things. 3. Drop the CPU scheduling changes for this first pass. The performance optimizations are definitely worth pursuing, but I don't want to get hung up in waiting on the scheduler dependencies to land. Additionally what looks like it works in your setup may not necessarily be good for everybody's setup. Having the baseline stuff in and working well, and then providing patches to change the CPU stuff in a way that we can test in a variety of different environments to validate the wins would be better. As someone who has to regularly go figure out what in the scheduler changed to make all of our metrics look bad again, I'm very wary of changes that make CPU selection policy decisions in a way that aren't changeable without changing the code. Thanks, Josef
