Hi Sasha, On Mon, 27 Jul 2009 18:47:22 +0300 Sasha Khapyorsky <[email protected]> wrote:
> 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. Well in fact I poorly chose the subject for this mail. It should have read: 'Allow multiple agents per process' There's nothing that needs to be thread safe in there. The thing is, if you want multiple threads from a single process to register their own agents, then the agents cannot be attached to the umad2sim_dev instance, but rather to a file instance opened on that device. That way each thread that wants to register an agent does it's own umad_open_port(). > > > 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. No, it's all the same thing. Sorry for the confusion. More below... > > > > > 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. That was my first thought, but gettid() is pretty much Linux specific so I tried another approach. I agree though that conn_num and it's use should be protected somehow. Will add a spinlock for that. > > > + > > + 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? We need it here too in umad2sim_dev_create() and children to create the sysfs tree. > > > - 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). Right, will change those. > > > + 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. This is now handled in umad2sim_close() called from close(). close() did nothing previously. Hope this shed some light in the intents of the patch. Thanks for your comments, Sebastien. > > 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
