Re: [PATCH] Clean index map on exit
> > Is the array always full ? No, but it has to be zero-initialized and free(NULL) is fine. idm_set() does: if (!idm->array[idx_array_index(index)]) { if (idm_grow(idm, index) < 0) (idm_grow does calloc().) So I concluded unused entries have to be zero already and I don't put a new constraint on struct index_map. > is the maximum index of the array > IDX_ARRAY_SIZE or could it be lower ? It's fixed: struct index_map { void **array[IDX_ARRAY_SIZE]; }; > And what about idm, is it free()'d somewhere else ? The owner has to take care of its struct index_map. I can only free the resources the map itself holds. For what it's worth: all struct index_maps are declared statically. Again, thank you for your quick feedback. Best regards, Hannes -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Add cleanup routines.
Am 11.04.2014 um 19:00 schrieb Yann Droneaud : > Hi, Thanks for your quick reply. > Please adds some explanation, in particular the purpose of the changes. > Your commit message only explain "how", but you should also explain > "why". Ok. > In C, empty prototype declare a function which accept any parameter. So > perhaps void ibverbs(void) is what you mean. Yup, thanks. >> +{ >> +size_t i; >> +for (i = 0; i < num_devices; i++) { >> +/* driver callback needed. May not be malloc'd memory */ > > Seems dangerous. Such interrogation must be explicitly added in the > commit message. Maybe it's better to leave it as a TODO and only free the device_list itself? >> +for (driver = head_driver; driver;) { >> +struct ibv_driver *tmp = driver; >> +driver = driver->next; >> +free(tmp); >> +} >> + > > Is it safe here to free the driver ? The struct itself is no longer needed. All the driver libraries are loaded and ibverbs_init() is call-once, so no new libraries are dlopen()ed (and call ibv_register_driver()). It's probably nicer first to dlclose() all libraries and then free all struct ibv_drivers. >> +for (list = so_list; list;) { >> +struct ibv_so_list *tmp = list; >> +list = list->next; >> +dlclose(tmp->dlhandle); >> +free(tmp); >> +} >> +} > > Why not store the dlopen() handle in the struct ibv_driver itself ? > This way only one list has to be scanned. That was my first idea, but the struct ibv_driver doesn't exist at that time. First the (driver) library is dlopen()d and calls ibv_register_driver(). Only there is the ibv_driver struct allocated, but the handle is gone. Best regards, Hannes -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Clean index map on exit
This patch adds the function idm_free() to free all entries of an index map. A call to this function is added in the ucma_cleanup destructor. The ucma_idm struct index_map is cleaned. Signed-off-by: Hannes Weisbach --- src/cma.c | 1 + src/indexer.c | 8 src/indexer.h | 1 + 3 files changed, 10 insertions(+) diff --git a/src/cma.c b/src/cma.c index 0cf4203..a533bf9 100644 --- a/src/cma.c +++ b/src/cma.c @@ -139,6 +139,7 @@ static void ucma_cleanup(void) ibv_close_device(cma_dev_array[cma_dev_cnt].verbs); } + idm_free(&ucma_idm); fastlock_destroy(&idm_lock); free(cma_dev_array); cma_dev_cnt = 0; diff --git a/src/indexer.c b/src/indexer.c index c8e8bce..4d1fd77 100644 --- a/src/indexer.c +++ b/src/indexer.c @@ -164,3 +164,11 @@ void *idm_clear(struct index_map *idm, int index) entry[idx_entry_index(index)] = NULL; return item; } + +void idm_free(struct index_map *idm) +{ + size_t i; + for (i = 0; i < IDX_ARRAY_SIZE; i++) { + free(idm->array[i]); + } +} diff --git a/src/indexer.h b/src/indexer.h index 0c5f388..829d46b 100644 --- a/src/indexer.h +++ b/src/indexer.h @@ -89,6 +89,7 @@ struct index_map int idm_set(struct index_map *idm, int index, void *item); void *idm_clear(struct index_map *idm, int index); +void idm_free(struct index_map *idm); static inline void *idm_at(struct index_map *idm, int index) { -- 1.8.5.5 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Add cleanup routines.
Dynamically loaded library handles are saved in a list and dlclosed() on exit. The list of struct ibv_driver *, as well as the global struct ibv_device ** list are free()d. Signed-off-by: Hannes Weisbach --- src/device.c | 10 ++ src/init.c | 51 ++- 2 files changed, 56 insertions(+), 5 deletions(-) diff --git a/src/device.c b/src/device.c index beb7b3c..d5b76bb 100644 --- a/src/device.c +++ b/src/device.c @@ -305,3 +305,13 @@ void __ibv_ack_async_event(struct ibv_async_event *event) } } default_symver(__ibv_ack_async_event, ibv_ack_async_event); + +FINI static void ibverbs_deinit() +{ + size_t i; + for (i = 0; i < num_devices; i++) { + /* driver callback needed. May not be malloc'd memory */ + free(device_list[i]); + } + free(device_list); +} diff --git a/src/init.c b/src/init.c index d0e4b1c..2a8bca4 100644 --- a/src/init.c +++ b/src/init.c @@ -67,6 +67,11 @@ struct ibv_driver_name { struct ibv_driver_name *next; }; +struct ibv_so_list { + void *dlhandle; + struct ibv_so_list *next; +}; + struct ibv_driver { const char *name; ibv_driver_init_funcinit_func; @@ -77,6 +82,7 @@ struct ibv_driver { static struct ibv_sysfs_dev *sysfs_dev_list; static struct ibv_driver_name *driver_name_list; static struct ibv_driver *head_driver, *tail_driver; +static struct ibv_so_list *so_list; static int find_sysfs_devs(void) { @@ -193,7 +199,14 @@ void verbs_register_driver(const char *name, verbs_driver_init_func init_func) static void load_driver(const char *name) { char *so_name; - void *dlhandle; + struct ibv_so_list *elem; + struct ibv_so_list **list; + + elem = malloc(sizeof(*elem)); + if(!elem) + return; + + elem->next = NULL; #define __IBV_QUOTE(x) #x #define IBV_QUOTE(x) __IBV_QUOTE(x) @@ -205,16 +218,25 @@ static void load_driver(const char *name) name) < 0) { fprintf(stderr, PFX "Warning: couldn't load driver '%s'.\n", name); - return; + goto err; } - dlhandle = dlopen(so_name, RTLD_NOW); - if (!dlhandle) { + elem->dlhandle = dlopen(so_name, RTLD_NOW); + if (!elem->dlhandle) { fprintf(stderr, PFX "Warning: couldn't load driver '%s': %s\n", name, dlerror()); - goto out; + goto err; } + for (list = &so_list; *list; list = &(*list)->next) + ; + + *list = elem; + + goto out; + +err: + free(elem); out: free(so_name); } @@ -573,3 +595,22 @@ out: return num_devices; } + +FINI static void unload_drivers() +{ + struct ibv_driver *driver; + struct ibv_so_list * list; + + for (driver = head_driver; driver;) { + struct ibv_driver *tmp = driver; + driver = driver->next; + free(tmp); + } + + for (list = so_list; list;) { + struct ibv_so_list *tmp = list; + list = list->next; + dlclose(tmp->dlhandle); + free(tmp); + } +} -- 1.8.5.5 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RDMA read does not update local memory
Hello, I found out that some code does madvise(MADV_DONTNEED) on the allocated memory. I think that this unmaps the memory from the user space process. If the process touches the memory again, a COW mapping or even a new, zeroed out page is mapped in the process. Either way, the relation between the physical page, where the data from the RDMA read ends up and the virtual page is broken. I realize that calling madvise(MADV_DONTNEED) doesn’t have any meaning for RDMA-registered memory. I feel however that my observed behavior is a bug and one of the following should happen: . madvise() on pinned memory/I/O memory should fail, i.e. return EINVAL, EIO or ENOMEM (I didn’t test what happens when you call madvise(MADV_DONTNEED) on I/O mem.) . RDMA actions on madvise()’d memory should fail. Best regards, Hannes Weisbach-- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RDMA read does not update local memory
Am 31.01.2014 um 15:26 schrieb Anuj Kalia : > Are you sure that the compiler knows that post_send() has side effects? No, thats why I tested with the mentioned memcmpv. I examined the emitted code with objdump and the compiler issued the mov instructions to read the bytes from memory - still didn’t work. I’ll leave the memcpyv in for the moment just to be sure. Best regards, Hannes-- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RDMA read does not update local memory
Am 30.01.2014 um 22:36 schrieb Anuj Kalia : > Hannes, > > Have you tried marking the memory that is being read as "volatile“? Thanks for the suggestion, but I don’t think that this solves the problem, because when I do: memset(…); rdma_post_read(…); /* wait */ memcmp(…); The compiler may not optimize across rdma_post_read() (or rather ibv_post_send() and finally ioctl()), because these function may have side-effects (which they do). Anyway, I implemented a little helper memcmpv(volatile unsigned char * s1, volatile unsiged char * s2, size_t size) to compare the memory as volatile, but the memory still has the preset 0x55 instead of the contents of the remote memory. May that memset() be still in the cache, so that the memory accesses of the CPU hit the cache instead of main memory? I can’t imagine the cache isn’t invalidated on reception of remote data. Best regards, Hannes-- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RDMA read does not update local memory
Hello all, I’m developing an (userspace) RDMA-application under Linux and came across a problem, I’m not able to solve. On machine A I had a chunk of memory registered with rdma_reg_read(), so others can read from that memory later. Machine B has memory which is also registered with rdma_reg_read(). Machine B tells machine A about that memory and the rkey. Machine A rdma_post_read()s then from that memory on machine B in it’s own memory. The problem now is, that - although no error is reported - the contents of machine A’s memory does not change. I memset() it to 0x55 before the rdma_post_read() for debugging purposes and instead of having the contents I expect (an ASCII string), the memory is still 0x55 after the work completion is signaled. Thus, I’ve allocated different chunks of memory using malloc() and posix_memalign() with different sizes and registered them with rdma_reg_read(), to see if that makes a difference. The rdma_post_read() works on all tested memory, except one chunk. I don’t know what is different about that memory, where my mistake might be or how to debug this. Please note, that I check all return values for errors (there are none) and also the work completion reports IBV_SUCCESS. I hope you can give me some hints on how to proceed. I apologize if my problem description was too brief; please ask for details if I left important information out. Best regards, Hannes Weisbach-- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Add const qualifier in rdma_reg*, rdma_post_send*|write and ibv_reg_mr (was: Re: non-const pointer void * addr in rdma_reg_* and rdma_post_[send|write])
Hi. > Two days ago I started to work on a kernel patchset to address similar > concerns on the verbs/RDMA APIs Nice. > > BTW, it's easier to change the kernel "internal" API than the public > userspace API of the library. But adding "const" should not break > compilation of existing userspace program. That was my thought too. However, because those functions are defined in as inline in a header, they actually do break user code. For example, rdma_reg_msgs passes a const void * to ibv_reg_mr, which takes a void *, thus resulting in a warning. If you (like me) compile with the -Werror flag, this results in broken code. The only offending libibverbs function I found so far is ibv_reg_mr(). > > PS: this issue could also be raised against libibverbs, so once you've > fixed librdmacm, why not fix libibverbs after ? This was my plan. But first I wanted to see if there even is interest in fixing this. As a user I deal with librdmacm directly, so I started to fix it first. Anyway, since patching librdmacm alone would break user code, I also included a patch for libibverbs, which adds the const qualifier to 'addr' parameter ib ibv_reg_mr(). lists.openfabrics.org says, this is also the mailing list for libibverbs (README is outdated ;)), I hope this is ok. Best regards, Hannes 0001-Make-addr-parameter-of-rdma_reg_-and-rdma_post_send-.patch Description: Binary data 0001-Make-ibv_mr_reg-parameter-void-addr-const.patch Description: Binary data
non-const pointer void * addr in rdma_reg_* and rdma_post_[send|write]
Hi, I just started working with librdmacm and I was wondering if there is a specific reason why rdma_reg_* functions and rdma_post_send/write functions take the local memory address as non-const pointer "void * addr". These functions shouldn't and don't change the memory pointed to by addr. I think this should be made explicit by using the type const void * for addr. In case you agree, I would volunteer to make the necessary changes. Best regards, Hannes Weisbach-- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html