[ofa-general] Re: [PATCH] ibsim: Allow multi-threaded use of ibsim
Sasha Khapyorsky
sashak at voltaire.com
Mon Jul 27 08:47:22 PDT 2009
On 13:38 Mon 27 Jul , sebastien dugue wrote:
>
> Right now, the socket name used between the client and ibsim is appended
> with the client's pid. However this does not work when several threads with
> the same pid want to register their clients.
And strictly speaking umad2sim by itself is not thread safe. So enabling
a multi-threaded clients you need to keep this in a mind, may be to add
some notes about this to README.
> instead of the pid, use a combination of the thread pid along with a
> connection number incremented for each new client.
>
> Also, attach the agents to the file instance opened on the device rather
> than on the device itself.
Would be nice to have this as separate patch.
>
> Signed-off-by: Sebastien Dugue <sebastien.dugue at bull.net>
> ---
> umad2sim/sim_client.c | 11 ++-
> umad2sim/umad2sim.c | 171 +++++++++++++++++++++++++++++++++++++-----------
> 2 files changed, 140 insertions(+), 42 deletions(-)
>
> diff --git a/umad2sim/sim_client.c b/umad2sim/sim_client.c
> index eb42a7c..22a50a2 100644
> --- a/umad2sim/sim_client.c
> +++ b/umad2sim/sim_client.c
> @@ -209,6 +209,8 @@ static int sim_init(struct sim_client *sc, char *nodeid)
> char *connect_port;
> char *connect_host;
> unsigned short port;
> + static int conn_num = 0;
> + int conn_id;
>
> connect_port = getenv("IBSIM_SERVER_PORT");
> connect_host = getenv("IBSIM_SERVER_NAME");
> @@ -228,7 +230,10 @@ static int sim_init(struct sim_client *sc, char *nodeid)
> if ((ctlfd = socket(remote_mode ? PF_INET : PF_LOCAL, SOCK_DGRAM, 0)) < 0)
> IBPANIC("can't get socket (ctlfd)");
>
> - size = make_name(&name, NULL, 0, "%s:ctl%d", socket_basename, pid);
> + conn_id = ((conn_num & 0xff) << 24) | (pid & 0xffffff);
> + conn_num++;
BTW why to not use pid/tid combination? This would eliminate the needs in
yet another static (and thread unsafe) variable.
> +
> + size = make_name(&name, NULL, 0, "%s:ctl%d", socket_basename, conn_id);
>
> if (bind(ctlfd, (struct sockaddr *)&name, size) < 0)
> IBPANIC("can't bind ctl socket");
> @@ -243,7 +248,7 @@ static int sim_init(struct sim_client *sc, char *nodeid)
>
> sc->fd_ctl = ctlfd;
>
> - size = make_name(&name, NULL, 0, "%s:in%d", socket_basename, pid);
> + size = make_name(&name, NULL, 0, "%s:in%d", socket_basename, conn_id);
>
> if (bind(fd, (struct sockaddr *)&name, size) < 0)
> IBPANIC("can't bind input socket");
> @@ -254,7 +259,7 @@ static int sim_init(struct sim_client *sc, char *nodeid)
> IBPANIC("can't read data from bound socket");
> port = ntohs(name.name_i.sin_port);
>
> - sc->clientid = sim_connect(sc, remote_mode ? port : pid, 0, nodeid);
> + sc->clientid = sim_connect(sc, remote_mode ? port : conn_id, 0, nodeid);
> if (sc->clientid < 0)
> IBPANIC("connect failed");
>
> diff --git a/umad2sim/umad2sim.c b/umad2sim/umad2sim.c
> index 55440ec..71f7c67 100644
> --- a/umad2sim/umad2sim.c
> +++ b/umad2sim/umad2sim.c
> @@ -81,12 +81,17 @@ struct umad2sim_dev {
> char name[32];
> uint8_t port;
> struct sim_client sim_client;
Why do we need sim_client field here if it is going to umad2sim_devfile
structure?
> - unsigned agent_idx[256];
> - struct ib_user_mad_reg_req agents[32];
> char umad_path[256];
> char issm_path[256];
> };
>
> +struct umad2sim_devfile {
> + struct umad2sim_dev *dev;
> + struct sim_client sim_client;
> + struct ib_user_mad_reg_req agents[32];
> + unsigned agent_idx[256];
> +};
> +
> static int (*real_open) (const char *path, int flags, ...);
> static int (*real_close) (int fd);
> static ssize_t(*real_read) (int fd, void *buf, size_t count);
> @@ -113,6 +118,7 @@ static char umad2sim_sysfs_prefix[32];
>
> static unsigned umad2sim_initialized;
> static struct umad2sim_dev *devices[32];
> +static struct umad2sim_devfile *devfiles[1024];
>
> /*
> * sysfs stuff
> @@ -378,9 +384,69 @@ static int dev_sysfs_create(struct umad2sim_dev *dev)
> * umad2sim device
> *
> */
> +static int umad2sim_open(struct umad2sim_dev *dev)
> +{
> + int i;
> + int fd;
> + struct umad2sim_devfile *file;
> +
> + /* Find unused fd */
> + for (fd = 0; fd < 1024; fd++)
arrsize() is more suitable here (and in another similar places).
> + if (devfiles[fd] == NULL)
> + break;
> +
> + if (fd == 1024) {
> + ERROR("umad2sim_open: No more available files\n");
> + errno = EMFILE;
> + return -1;
> + }
>
> -static ssize_t umad2sim_read(struct umad2sim_dev *dev, void *buf, size_t count)
> + if ((file = calloc(1, sizeof(struct umad2sim_devfile))) == NULL) {
> + ERROR("umad2sim_open: Out of memory\n");
> + errno = ENOMEM;
> + return -1;
> + }
> +
> + file->dev = dev;
> +
> + for (i = 0; i < arrsize(file->agents); i++)
> + file->agents[i].id = (uint32_t)(-1);
> +
> + for (i = 0; i < arrsize(file->agent_idx); i++)
> + file->agent_idx[i] = (unsigned)(-1);
> +
> + if (sim_client_init(&file->sim_client) < 0) {
> + free(file);
> + return -1;
> + }
> +
> + devfiles[fd] = file;
> +
> + return 1024 + fd;
> +}
> +
> +static int umad2sim_close(int fd)
> {
> + struct umad2sim_devfile *file;
> +
> + if ((file = devfiles[fd - 1024]) == NULL) {
> + errno = EBADF;
> + ERROR("umad2sim_close: no such file\n");
> + return -1;
> + }
> +
> + sim_client_exit(&file->sim_client);
> +
> + free(file);
> +
> + devfiles[fd - 1024] = NULL;
> +
> + return 0;
> +}
> +
> +static ssize_t umad2sim_read(int fd, void *buf, size_t count)
> +{
> + struct umad2sim_devfile *file;
> struct sim_request req;
> ib_user_mad_t *umad = (ib_user_mad_t *) buf;
> unsigned mgmt_class;
> @@ -388,7 +454,13 @@ static ssize_t umad2sim_read(struct umad2sim_dev *dev, void *buf, size_t count)
>
> DEBUG("umad2sim_read: %zu...\n", count);
>
> - cnt = real_read(dev->sim_client.fd_pktin, &req, sizeof(req));
> + if ((file = devfiles[fd - 1024]) == NULL) {
> + errno = EBADF;
> + ERROR("umad2sim_read: no such file\n");
> + return -1;
> + }
> +
> + cnt = real_read(file->sim_client.fd_pktin, &req, sizeof(req));
> DEBUG("umad2sim_read: got %d...\n", cnt);
> if (cnt < sizeof(req)) {
> ERROR("umad2sim_read: partial request - skip.\n");
> @@ -406,7 +478,7 @@ static ssize_t umad2sim_read(struct umad2sim_dev *dev, void *buf, size_t count)
> mad_get_field(req.mad, 0, IB_MAD_ATTRID_F),
> mad_get_field(req.mad, 0, IB_MAD_ATTRMOD_F));
>
> - if (mgmt_class >= arrsize(dev->agent_idx)) {
> + if (mgmt_class >= arrsize(file->agent_idx)) {
> ERROR("bad mgmt_class 0x%x\n", mgmt_class);
> mgmt_class = 0;
> }
> @@ -415,7 +487,7 @@ static ssize_t umad2sim_read(struct umad2sim_dev *dev, void *buf, size_t count)
> uint64_t trid = mad_get_field64(req.mad, 0, IB_MAD_TRID_F);
> umad->agent_id = (trid >> 32) & 0xffff;
> } else
> - umad->agent_id = dev->agent_idx[mgmt_class];
> + umad->agent_id = file->agent_idx[mgmt_class];
>
> umad->status = ntohl(req.status);
> umad->timeout_ms = 0;
> @@ -437,9 +509,9 @@ static ssize_t umad2sim_read(struct umad2sim_dev *dev, void *buf, size_t count)
> return umad->length;
> }
>
> -static ssize_t umad2sim_write(struct umad2sim_dev *dev,
> - const void *buf, size_t count)
> +static ssize_t umad2sim_write(int fd, const void *buf, size_t count)
> {
> + struct umad2sim_devfile *file;
> struct sim_request req;
> ib_user_mad_t *umad = (ib_user_mad_t *) buf;
> int cnt;
> @@ -457,12 +529,18 @@ static ssize_t umad2sim_write(struct umad2sim_dev *dev,
>
> DEBUG("umad2sim_write: %zu...\n", count);
>
> + if ((file = devfiles[fd - 1024]) == NULL) {
> + errno = EBADF;
> + ERROR("umad2sim_write: no such file\n");
> + return -1;
> + }
> +
> DEBUG("umad2sim_write: umad: agent_id=%u, retries=%u, "
> "agent.class=%x, agent.qpn=%u, "
> "addr.qpn=%u, addr.lid=%u\n",
> umad->agent_id, umad->retries,
> - dev->agents[umad->agent_id].mgmt_class,
> - dev->agents[umad->agent_id].qpn,
> + file->agents[umad->agent_id].mgmt_class,
> + file->agents[umad->agent_id].qpn,
> htonl(umad->addr.qpn), htons(umad->addr.lid));
> DEBUG("umad2sim_write: mad: method=%x, response=%x, mgmtclass=%x, "
> "attrid=%x, attrmod=%x\n",
> @@ -476,7 +554,7 @@ static ssize_t umad2sim_write(struct umad2sim_dev *dev,
> req.slid = req.dlid == 0xffff ? 0xffff : 0; /* 0 - means auto
> (supported by ibsim) */ ;
> req.dqp = umad->addr.qpn;
> - req.sqp = htonl(dev->agents[umad->agent_id].qpn);
> + req.sqp = htonl(file->agents[umad->agent_id].qpn);
> req.status = 0;
>
> cnt = count - umad_size();
> @@ -492,7 +570,7 @@ static ssize_t umad2sim_write(struct umad2sim_dev *dev,
> mad_set_field64(req.mad, 0, IB_MAD_TRID_F, trid);
> }
>
> - cnt = write(dev->sim_client.fd_pktout, (void *)&req, sizeof(req));
> + cnt = write(file->sim_client.fd_pktout, (void *)&req, sizeof(req));
> if (cnt < 0) {
> ERROR("umad2sim_write: cannot write\n");
> return -1;
> @@ -503,19 +581,21 @@ static ssize_t umad2sim_write(struct umad2sim_dev *dev,
> return count;
> }
>
> -static int register_agent(struct umad2sim_dev *dev,
> +static int register_agent(struct umad2sim_devfile *file,
> struct ib_user_mad_reg_req *req)
> {
> - unsigned i;
> + unsigned i;
> +
> DEBUG("register_agent: id = %u, qpn = %u, mgmt_class = %u,"
> " mgmt_class_version = %u, rmpp_version = %u\n",
> req->id, req->qpn, req->mgmt_class, req->mgmt_class_version,
> req->rmpp_version);
> - for (i = 0; i < arrsize(dev->agents); i++)
> - if (dev->agents[i].id == (uint32_t)(-1)) {
> +
> + for (i = 0; i < arrsize(file->agents); i++)
> + if (file->agents[i].id == (uint32_t)(-1)) {
> req->id = i;
> - dev->agents[i] = *req;
> - dev->agent_idx[req->mgmt_class] = i;
> + file->agents[i] = *req;
> + file->agent_idx[req->mgmt_class] = i;
> DEBUG("agent registered: %d\n", i);
> return 0;
> }
> @@ -523,28 +603,36 @@ static int register_agent(struct umad2sim_dev *dev,
> return -1;
> }
>
> -static int unregister_agent(struct umad2sim_dev *dev, unsigned id)
> +static int unregister_agent(struct umad2sim_devfile *file, unsigned id)
> {
> unsigned mgmt_class;
> - if (id >= arrsize(dev->agents)) {
> + if (id >= arrsize(file->agents)) {
> errno = EINVAL;
> return -1;
> }
> - mgmt_class = dev->agents[id].mgmt_class;
> - dev->agents[id].id = (uint32_t)(-1);
> - dev->agent_idx[mgmt_class] = -1;
> + mgmt_class = file->agents[id].mgmt_class;
> + file->agents[id].id = (uint32_t)(-1);
> + file->agent_idx[mgmt_class] = -1;
> return 0;
> }
>
> -static int umad2sim_ioctl(struct umad2sim_dev *dev, unsigned long request,
> - void *arg)
> +static int umad2sim_ioctl(int fd, unsigned long request, void *arg)
> {
> + struct umad2sim_devfile *file;
> +
> DEBUG("umad2sim_ioctl: %lu, %p...\n", request, arg);
> +
> + if ((file = devfiles[fd - 1024]) == NULL) {
> + errno = EBADF;
> + ERROR("umad2sim_ioctl: no such file\n");
> + return -1;
> + }
> +
> switch (request) {
> case IB_USER_MAD_REGISTER_AGENT:
> - return register_agent(dev, arg);
> + return register_agent(file, arg);
> case IB_USER_MAD_UNREGISTER_AGENT:
> - return unregister_agent(dev, *((unsigned *)arg));
> + return unregister_agent(file, *((unsigned *)arg));
> case IB_USER_MAD_ENABLE_PKEY:
> return 0;
> default:
> @@ -556,7 +644,6 @@ static int umad2sim_ioctl(struct umad2sim_dev *dev, unsigned long request,
> static struct umad2sim_dev *umad2sim_dev_create(unsigned num, const char *name)
> {
> struct umad2sim_dev *dev;
> - unsigned i;
>
> DEBUG("umad2sim_dev_create: %s...\n", name);
>
> @@ -573,10 +660,6 @@ static struct umad2sim_dev *umad2sim_dev_create(unsigned num, const char *name)
>
> dev->port = mad_get_field(&dev->sim_client.portinfo, 0,
> IB_PORT_LOCAL_PORT_F);
> - for (i = 0; i < arrsize(dev->agents); i++)
> - dev->agents[i].id = (uint32_t)(-1);
> - for (i = 0; i < arrsize(dev->agent_idx); i++)
> - dev->agent_idx[i] = (unsigned)(-1);
>
> dev_sysfs_create(dev);
>
> @@ -637,6 +720,11 @@ static void umad2sim_cleanup(void)
> char path[1024];
> unsigned i;
> DEBUG("umad2sim_cleanup...\n");
> +
> + for (i = 0; i < 1024; i++)
> + if (devfiles[i])
> + free(devfiles[i]);
And what about sim_client disconnection? umad2sim_dev_delete() did this.
Sasha
> +
> for (i = 0; i < arrsize(devices); i++)
> if (devices[i]) {
> umad2sim_dev_delete(devices[i]);
> @@ -756,7 +844,7 @@ int open(const char *path, int flags, ...)
> if (!(dev = devices[i]))
> continue;
> if (!strncmp(path, dev->umad_path, sizeof(dev->umad_path))) {
> - return 1024 + i;
> + return umad2sim_open(dev);
> }
> if (!strncmp(path, dev->issm_path, sizeof(dev->issm_path))) {
> sim_client_set_sm(&dev->sim_client, 1);
> @@ -779,7 +867,7 @@ int close(int fd)
> sim_client_set_sm(&dev->sim_client, 0);
> return 0;
> } else if (fd >= 1024) {
> - return 0;
> + return umad2sim_close(fd);
> } else
> return real_close(fd);
> }
> @@ -791,7 +879,7 @@ ssize_t read(int fd, void *buf, size_t count)
> if (fd >= 2048)
> return -1;
> else if (fd >= 1024)
> - return umad2sim_read(devices[fd - 1024], buf, count);
> + return umad2sim_read(fd, buf, count);
> else
> return real_read(fd, buf, count);
> }
> @@ -803,7 +891,7 @@ ssize_t write(int fd, const void *buf, size_t count)
> if (fd >= 2048)
> return -1;
> else if (fd >= 1024)
> - return umad2sim_write(devices[fd - 1024], buf, count);
> + return umad2sim_write(fd, buf, count);
> else
> return real_write(fd, buf, count);
> }
> @@ -821,7 +909,7 @@ int ioctl(int fd, unsigned long request, ...)
> if (fd >= 2048)
> return -1;
> else if (fd >= 1024)
> - return umad2sim_ioctl(devices[fd - 1024], request, arg);
> + return umad2sim_ioctl(fd, request, arg);
> else
> return real_ioctl(fd, request, arg);
> }
> @@ -836,9 +924,14 @@ int poll(struct pollfd *pfds, nfds_t nfds, int timeout)
>
> for (i = 0; i < nfds; i++) {
> if (pfds[i].fd >= 1024 && pfds[i].fd < 2048) {
> - struct umad2sim_dev *dev = devices[pfds[i].fd - 1024];
> + struct umad2sim_devfile *file;
> +
> + if ((file = devfiles[pfds[i].fd - 1024]) == NULL) {
> + errno = EBADF;
> + return -1;
> + }
> saved_fds[i] = pfds[i].fd;
> - pfds[i].fd = dev->sim_client.fd_pktin;
> + pfds[i].fd = file->sim_client.fd_pktin;
> } else
> saved_fds[i] = 0;
> }
> --
> 1.6.3.1
>
More information about the general
mailing list