Pete Wyckoff wrote: > dorons@xxxxxxxxxxxx wrote on Thu, 02 Oct 2008 13:30 +0300: >> dynamically link libibverbs and librdma for using >> stgt without having userspace IB (e.g tcp mode). > > A couple of comments. > >> @@ -1,4 +1,6 @@ >> mandir = /usr/share/man >> +libdir = /usr/lib64 >> +bindir = /usr/sbin > > Fine for most 64-bit machines, not sure if we are worried about others. > >> tgtd: $(TGTD_OBJS) >> - $(CC) $^ -o $@ $(LIBS) >> + $(CC) -Xlinker -E $^ -o $@ $(LIBS) > > You may need to do something like: > > $(LD) -o $@ -Wl,-whole-archive libtgtrdma.so -Wl,-no-whole-archive $(LIBS) > > to make sure the constructor functions in the .so get called properly. I've tried to use: $(CC) -o $@ -Wl,-whole-archive libtgtrdma.so -Wl,-no-whole-archive $(LIBS) but I received the following errors: libtgtrdma.so: undefined reference to `ibv_ack_cq_events' libtgtrdma.so: undefined reference to `rdma_reject' libtgtrdma.so: undefined reference to `ibv_alloc_pd' libtgtrdma.so: undefined reference to `ibv_destroy_qp' libtgtrdma.so: undefined reference to `rdma_create_qp' libtgtrdma.so: undefined reference to `ibv_dereg_mr' libtgtrdma.so: undefined reference to `rdma_destroy_id' libtgtrdma.so: undefined reference to `ibv_query_device' libtgtrdma.so: undefined reference to `ibv_reg_mr' libtgtrdma.so: undefined reference to `rdma_bind_addr' libtgtrdma.so: undefined reference to `rdma_get_cm_event' libtgtrdma.so: undefined reference to `rdma_create_event_channel' libtgtrdma.so: undefined reference to `rdma_create_id' libtgtrdma.so: undefined reference to `ibv_create_comp_channel' libtgtrdma.so: undefined reference to `rdma_accept' libtgtrdma.so: undefined reference to `rdma_disconnect' libtgtrdma.so: undefined reference to `ibv_get_cq_event' libtgtrdma.so: undefined reference to `rdma_listen' libtgtrdma.so: undefined reference to `rdma_ack_cm_event' libtgtrdma.so: undefined reference to `ibv_create_cq' > >> +$(SO_LIBS): $(ISER_OBJS) >> + rm -f $@ $(SO_NAME) >> + $(LD) -shared -soname $(SO_LIBS) -o $(SO_LIBS) $(ISER_OBJS) >> + ln -s $(SO_LIBS) $(SO_NAME) >> + > > You must put -lrdmacm -libverbs on this $(LD) line. I learned that > the hard way once or twice. Else the symbol versions don't work out > at runtime. > As far as I've seen it works without the -lrdmacm -libverbs. Adding this causes the rpm be depended on libibverbs and librdmacm and this is exactly what we wanted to avoid from. [root@cumin tgt_with_dynamic]# rpm -qR scsi-target-utils /bin/sh /bin/sh /bin/sh /usr/bin/perl libc.so.6()(64bit) libc.so.6(GLIBC_2.2.5)(64bit) libc.so.6(GLIBC_2.3)(64bit) libc.so.6(GLIBC_2.3.2)(64bit) libc.so.6(GLIBC_2.3.3)(64bit) libcrypto.so.6()(64bit) libdl.so.2()(64bit) libdl.so.2(GLIBC_2.2.5)(64bit) libibverbs.so.1()(64bit) libibverbs.so.1(IBVERBS_1.0)(64bit) libibverbs.so.1(IBVERBS_1.1)(64bit) libpthread.so.0()(64bit) libpthread.so.0(GLIBC_2.2.5)(64bit) libpthread.so.0(GLIBC_2.3.2)(64bit) librdmacm.so.1()(64bit) librdmacm.so.1(RDMACM_1.0)(64bit) perl(Config::General) perl(Getopt::Long) perl(strict) rpmlib(CompressedFileNames) <= 3.0.4-1 rpmlib(PayloadFilesHavePrefix) <= 4.0-1 rtld(GNU_HASH) >> +install_lib: $(SO_LIBS) >> + rm -f $(DESTDIR)$(libdir)/$(SO_NAME) >> + install -m 755 $(SO_LIBS) $(DESTDIR)$(libdir) >> + ln -s $(DESTDIR)/usr/lib64/$(SO_LIBS) $(DESTDIR)$(libdir)/$(SO_NAME) > > maybe use $(libdir) in here too Yes, you are correct, it should be $(libdir). > >> +/* >> + * iSCSI extensions for RDMA (iSER) data path >> + * >> + * Copyright (C) 2007 Dennis Dalessandro (dennis@xxxxxxx) >> + * Copyright (C) 2007 Ananth Devulapalli (ananth@xxxxxxx) >> + * Copyright (C) 2007 Pete Wyckoff (pw@xxxxxxx) > > Actually you wrote this new file, in 2008. And it needs a better > description. > >> +#include <stdio.h> >> +#include <dlfcn.h> >> +#include <syslog.h> >> + >> +static void *pverbs; >> +static void *prdma; >> +static void *ptgtrdma; >> + >> +__attribute__((constructor)) static void iser_transport_init(void) >> +{ >> + void (*iser_init)(void); >> + char *error; >> + >> + pverbs = dlopen("libibverbs.so",RTLD_NOW|RTLD_GLOBAL); >> + if (!pverbs) { >> + goto Exit; /* do not register iser transport */ >> + } >> + >> + prdma = dlopen("librdmacm.so",RTLD_NOW|RTLD_GLOBAL); >> + if (!prdma) { >> + goto Exit; /* do not register iser transport */ >> + } >> + >> + ptgtrdma = dlopen("libtgtrdma.so",RTLD_NOW|RTLD_GLOBAL); >> + if (!ptgtrdma) { >> + goto Exit; >> + } >> + >> + dlerror(); /* Clear any existing error */ >> + iser_init = dlsym(ptgtrdma, "iser_transport_init"); >> + if ((error = dlerror()) != NULL) { >> + syslog(LOG_ERR, "%s\n", error); >> + goto Exit; >> + } >> + >> + (*iser_init)(); > > My understanding is that iser_transport_init() would have been > called by the dlopen() of libtgtrdma.so, as it is already marked > ((constructor)). Did you verify that this doesn't happen? Maybe > this requires the whole-archive bit above. > > Also I figured dlopen("libtgtrdma.so") would pull in the other two > libs on its own. Please check, as it would be much cleaner just > to have the single dlopen of libtgtrdma.so, and no dlsym or other > libs. > >> +__attribute__((destructor)) static void iser_transport_close(void) >> +{ >> + syslog(LOG_INFO, "iser transport cleanup"); >> + if (pverbs) >> + dlclose(pverbs); >> + if (prdma) >> + dlclose(prdma); >> + if (ptgtrdma) >> + dlclose(ptgtrdma); >> +} > > Interesting. I guess tgtd knows how to exit cleanly these days. > But this stuff isn't strictly necessary; you can just exit and the > OS will clean it all. > >> diff --git a/usr/iscsi/libtgt_rdma.c b/usr/iscsi/libtgt_rdma.c >> index d3b5147..d6ba5fa 100644 >> --- a/usr/iscsi/libtgt_rdma.c >> +++ b/usr/iscsi/libtgt_rdma.c >> @@ -30,6 +30,8 @@ >> #include <sys/epoll.h> >> #include <infiniband/verbs.h> >> #include <rdma/rdma_cma.h> >> +#include <dlfcn.h> >> +#include <syslog.h> >> >> #include "util.h" >> #include "iscsid.h" >> @@ -1754,7 +1756,9 @@ static struct iscsi_transport iscsi_iser = { >> .ep_getpeername = iscsi_rdma_getpeername, >> }; >> >> -__attribute__((constructor)) static void iser_transport_init(void) >> +void iser_transport_init(void) >> { >> + syslog(LOG_INFO, "iser transport register"); >> iscsi_transport_register(&iscsi_iser); >> + return; >> } > > Oh! You removed the constructor. Now I see why it doesn't work. > Maybe try my suggestion instead? > > -- Pete After reviewing all the patch do you still think that all the changes are mandatory? Thanks, Doron -- To unsubscribe from this list: send the line "unsubscribe stgt" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html