plugins/posix: minimized IO queue abstraction #1051
Conversation
anton-nayshtut
requested review from
barneuman,
etoledano,
vvenkates27 and
w1ldptr
as code owners
|
👋 Hi anton-nayshtut! Thank you for contributing to ai-dynamo/nixl. Your PR reviewers will review your contribution then trigger the CI to test your changes. 🚀 |
anton-nayshtut
force-pushed
the
antonn/posix_file_api
branch
from
1aa9a5e to
a81c168
Compare
Signed-off-by: Anton Nayshtut <[email protected]>
anton-nayshtut
force-pushed
the
antonn/posix_file_api
branch
from
a81c168 to
b665245
Compare
anton-nayshtut
force-pushed
the
antonn/posix_file_api
branch
from
b665245 to
be8fe2a
Compare
anton-nayshtut
changed the title
anton-nayshtut
force-pushed
the
antonn/posix_file_api
branch
from
be8fe2a to
b607ba4
Compare
anton-nayshtut
changed the title
src/plugins/posix/io_queue.h
Outdated
| } | ||
|
|
||
| protected: | ||
| static std::map<std::string, nixlPosixIOQueueCreateFn> apis; |
There was a problem hiding this comment.
This class is implicit singleton, which is often considered as anti-pattern
Maybe these fields should not be static?
There was a problem hiding this comment.
For now, I replaced it by a static array. Please don't hesitate to suggest an alternative.
src/plugins/posix/io_queue.h
Outdated
| static const uint32_t MAX_IOS; | ||
| }; | ||
|
|
||
| #define NIXL_POSIX_IO_QUEUE_REGISTER(io_queue_name, class_name) \ |
There was a problem hiding this comment.
Since it's C++, better use template, not macro
There was a problem hiding this comment.
The macro has changed. Please re-examine.
anton-nayshtut
force-pushed
the
antonn/posix_file_api
branch
2 times, most recently
from
b131f48 to
1546c41
Compare
| virtual ~nixlPosixIOQueue() {} | ||
|
|
||
| virtual nixl_status_t | ||
| enqueue(int fd, |
There was a problem hiding this comment.
This enqueue encapsulates all possible params, even if some of them are not used by an actual impl, right?
IMO this is not very clean from design perspective
At least we can define a structure for these params, need to think more on that
The class itself may be a template, so that we can encapsulate the common structures (free_list, etc)
This way you can reuse much more than just an interface
Maybe it should be something like:
template <typename Entry>
class nixlQ {
protected:
std::list<Entry> free_list_; // reuse common logic in all impl
..
}
There was a problem hiding this comment.
This enqueue encapsulates all possible params, even if some of them are not used by an actual impl, right?
No. All params are used by each impl but they're treated differently.
I'll think about templates though. Thanks!
There was a problem hiding this comment.
On the other hand, the whole idea of this design is to properly use inheritance - have generic nixlPosixIOQueue pointer and instantiate it as a specific IO Queue type. As far as I understand, templates are incompatible with this idea. Am I right?
There was a problem hiding this comment.
Oh! I get it. I can have an additional level of inheritance, which would be a template. Will do. Thanks!
anton-nayshtut
force-pushed
the
antonn/posix_file_api
branch
from
1546c41 to
9408a79
Compare
This patch set minimizes the IO queue abstraction, making it as low- level as possible. It now only wraps the lowest-level IO-related API. It works with a set of IOs (writes or reads) while leaving the Xfer level logic to the upper layer (posix_backend). This patch set allows the posix_backend to implement the common logic, while only offloading the details of specific file access API to the IO queue abstraction. This patch set prepares the code for the POSIX plugin threading model implementation. With it, the threading will be implemented once, in the posix_backend. Signed-off-by: Anton Nayshtut <[email protected]>
anton-nayshtut
force-pushed
the
antonn/posix_file_api
branch
from
9408a79 to
5911caa
Compare
3 participants
What?
This patch set minimizes the IO queue abstraction, making it as low-level as possible.
Why?
This patch set prepares the code for the POSIX plugin threading model implementation. With it, the threading will be implemented once, in the
posix_backend.How?
The IO queue abstraction now only wraps the lowest-level IO-related API. It works with a set of IOs (writes or reads) while leaving the Xfer level logic to the upper layer (
posix_backend).This patch set allows the
posix_backendto implement the common logic, while only offloading the details of specific file access API to the IO queue abstraction.