Move libfabric-refactor-traits into libfabric-refactor
#36
Conversation
| /home/ubuntu/rfaas-refactor/rfaas/lib/resources.cpp:43:40: warning: unused parameter ‘cores’ [-Wunused-parameter] | ||
| 43 | std::vector<int> servers::select(int cores) | ||
| | ~~~~^~~~~ | ||
| In file included from /home/ubuntu/rfaas-refactor/server/executor/server.hpp:15, |
There was a problem hiding this comment.
Does this code even compile? What's the purpose of this file?
There was a problem hiding this comment.
I didn't mean to commit this file.
|
|
||
| #ifdef USE_LIBFABRIC | ||
| #include <rdma/fabric.h> |
There was a problem hiding this comment.
Shouldn't this include be behind #ifdef? What if we compile without libfabric but with ibverbs?
There was a problem hiding this comment.
Since the code contains symbols from this header, I was getting compiler issues when it was not included. I discussed this issue with Marcin earlier in the summer and he said that this was okay for now.
There was a problem hiding this comment.
@mattnappo The problem is not inclusion - the problem is that the include should be guarded by #ifdef. If I compile just with verbs but not libfabric, then will it work?
| { | ||
| ar(CEREAL_NVP(addr), CEREAL_NVP(rkey), CEREAL_NVP(size)); | ||
| } | ||
| }; | ||
|
|
||
| template<typename T> | ||
| struct Buffer : impl::Buffer{ | ||
| struct LibfabricRemoteBuffer : RemoteBuffer<libfabric> |
There was a problem hiding this comment.
Is there any difference between both implementations when it comes to RemoteBufer?
There was a problem hiding this comment.
No. You are correct, this doesn't need to be a template.
| using sge_t = typename library_traits<Library>::sge_t; | ||
| using lkey_t = typename library_traits<Library>::lkey_t; | ||
|
|
||
| mutable std::vector<sge_t> _sges; |
There was a problem hiding this comment.
What's the point of mutable here?
There was a problem hiding this comment.
That was in the original codebase.
| return static_cast<Derived *>(this)->array(); | ||
| } | ||
|
|
||
| size_t size() const; |
There was a problem hiding this comment.
Shouldn't this function be defined here? A function member of a template class should not go into a .cpp file IMO.
There was a problem hiding this comment.
Correct. I will fix this.
| } | ||
|
|
||
| namespace rdmalib |
There was a problem hiding this comment.
Yes, will delete
| #ifdef USE_LIBFABRIC | ||
| _allocation_buffer.register_memory(_active.pd(), FI_WRITE | FI_REMOTE_WRITE); | ||
| #else | ||
| _allocation_buffer.register_memory(_active.pd(), IBV_ACCESS_LOCAL_WRITE | IBV_ACCESS_REMOTE_WRITE); |
There was a problem hiding this comment.
Shouldn't this just use the new refactored traits?
There was a problem hiding this comment.
Yes, I must have missed this one.
| {} | ||
|
|
||
| executor::~executor() | ||
| rdmalib::Buffer<char, libfabric> libfabric_executor::load_library(std::string path) |
There was a problem hiding this comment.
Is this function any different from ibverbs implementation?
There was a problem hiding this comment.
Well, it is different in the sense that they return different kinds of buffers. I guess I can make it return the parent buffer class?
There was a problem hiding this comment.
Okay, in that context it's fine - unless you can just make it a template in a parent class (depends if anything changes beyond the type itself).
| bool execute(std::string fname, const std::vector<rdmalib::Buffer<T, Library>> & in, std::vector<rdmalib::Buffer<T, Library>> & out); | ||
| }; | ||
|
|
||
| struct libfabric_executor : executor<libfabric_executor, libfabric> { |
There was a problem hiding this comment.
Why are separate implementations for ibverbs and libfabric necessary? This is the executor from rfaas - shouldn't they just use the concepts from rdmalib? Why the need to duplicate executors?
|
|
||
| int main(int argc, char ** argv) | ||
| { | ||
| // Register a SIGINT handler so that we can gracefully exit | ||
| //server::SignalHandler sighandler; | ||
|
|
||
| auto opts = server::opts(argc, argv); | ||
|
|
||
| #ifdef USE_LIBFABRIC |
There was a problem hiding this comment.
It would be better for this to be a runtime switch - we can compile with both implementations enabled.
All of the recent development of the refactor this summer was done on
libfabric-refactor-traits. It would probably suffice to delete libfabric-refactor and just rename libfabric-refactor-traits to libfabric-refactor, but opening this PR for completion.