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 <[email protected]> > --- > 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 > _______________________________________________ general mailing list [email protected] http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
