[stgt] [PATCH 2/2] dynamically link libibverbs and librdma

[Doron Shoham]

Pete Wyckoff wrote:
> dorons at Voltaire.COM 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 at 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 at osc.edu)
>> + * Copyright (C) 2007 Ananth Devulapalli (ananth at osc.edu)
>> + * Copyright (C) 2007 Pete Wyckoff (pw at osc.edu)
> 
> 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 at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html