[ofa-general] Re: [PATCH] ibsim: Support short RMPP packets
Hal Rosenstock
hrosenstock at xsigo.com
Mon Jun 16 06:45:42 PDT 2008
On Mon, 2008-06-16 at 11:52 +0300, Sasha Khapyorsky wrote:
> On 14:40 Thu 12 Jun , Hal Rosenstock wrote:
> > > > @@ -1144,9 +1145,10 @@ int process_packet(Client * cl, void *p, int size, Client ** dcl)
> > > >
> > > > *dcl = cl;
> > > >
> > > > - DEBUG("client %d, size %d", cl->id, size);
> > > > - if (size != sizeof(*r)) {
> > > > - IBWARN("bad packet size %d (!= %zu)", size, sizeof(*r));
> > > > + DEBUG("client %d size %d", cl->id, size);
> > > > + if (size > sizeof(*r) + MAD_BLOCK_SIZE) {
> > >
> > > What about partial write?
> >
> > This change was needed for short RMPP.
>
> Yes, but it is risky. Of course 'ibsim' is for testing, and not really
> production code, but anyway..
>
> Another solution would be to encode packet (or payload) length into
> header.
Yes, that was an approach I entertained.
> For this we even don't need to break ABI - there is unused
> 64-bit context field.
> > > The socket is non-blocked datagram.
> >
> > Is a framing layer needed ?
>
> I don't know. In my old implementation I switched to STREAM socket in
> order to not reinvent a wheel,
but overall performance suffered, right ?
> but it is mostly due to long packets.
Right; so maybe an additional (reliable) socket for only these
request/response types ?
> > > > diff --git a/include/ibsim.h b/include/ibsim.h
> > > > index 84568e6..14a3f90 100644
> > > > --- a/include/ibsim.h
> > > > +++ b/include/ibsim.h
> > > > @@ -1,5 +1,6 @@
> > > > /*
> > > > * Copyright (c) 2006,2007 Voltaire, Inc. All rights reserved.
> > > > + * Copyright (c) 2008 Xsigo Systems Inc. All rights reserved.
> > > > *
> > > > * This file is part of ibsim.
> > > > *
> > > > @@ -61,6 +62,8 @@ struct sim_port {
> > > > #define SIM_MAGIC 0xdeadbeef
> > > > #define SIM_CTL_MAX_DATA 64
> > > >
> > > > +#define MAD_BLOCK_SIZE 256
> > > > +
> > > > struct sim_request {
> > > > uint32_t dlid;
> > > > uint32_t slid;
> > > > @@ -68,7 +71,17 @@ struct sim_request {
> > > > uint32_t sqp;
> > > > uint32_t status;
> > > > uint64_t context;
> > > > - char mad[256];
> > > > + char mad[0];
> > > > +};
> > > > +
> > > > +struct sim_req256 {
> > > > + uint32_t dlid;
> > > > + uint32_t slid;
> > > > + uint32_t dqp;
> > > > + uint32_t sqp;
> > > > + uint32_t status;
> > > > + uint64_t context;
> > > > + char mad[MAD_BLOCK_SIZE];
> > > > };
> > >
> > > Why do we need two separate structures?
> >
> > The second one was just to eliminate the memory allocation (and use the
> > stack). Only forwarded packets have the variability in size.
>
> Then I think it would be cleaner to have something like 'struct
> sim_req_header'
Sure.
> and leave a lot of code unmodified.
It's a minor amount of code currently.
> > > > enum SIM_CTL_TYPES {
> > > > diff --git a/umad2sim/umad2sim.c b/umad2sim/umad2sim.c
> > > > index 4cbf8da..9c69fb5 100644
> > > > --- a/umad2sim/umad2sim.c
> > > > +++ b/umad2sim/umad2sim.c
> > > > @@ -1,5 +1,6 @@
> > > > /*
> > > > * Copyright (c) 2006,2007 Voltaire, Inc. All rights reserved.
> > > > + * Copyright (c) 2008 Xsigo Systems Inc. All rights reserved.
> > > > *
> > > > * This file is part of ibsim.
> > > > *
> > > > @@ -376,7 +377,7 @@ static int dev_sysfs_create(struct umad2sim_dev *dev)
> > > >
> > > > static ssize_t umad2sim_read(struct umad2sim_dev *dev, void *buf, size_t count)
> > > > {
> > > > - struct sim_request req;
> > > > + struct sim_req256 req;
> > > > ib_user_mad_t *umad = (ib_user_mad_t *) buf;
> > > > unsigned mgmt_class;
> > > > int cnt;
> > > > @@ -385,11 +386,6 @@ static ssize_t umad2sim_read(struct umad2sim_dev *dev, void *buf, size_t count)
> > > >
> > > > cnt = real_read(dev->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");
> > > > - umad->status = EAGAIN;
> > > > - return umad_size();
> > > > - }
> > > >
> > > > mgmt_class = mad_get_field(req.mad, 0, IB_MAD_MGMTCLASS_F);
> > >
> > > This checked that we got at least req header before decode this.
> >
> > No; I think it checked that the entire packet existed as req was the
> > entire packet not just the header.
> >
> > > Why is it removed?
> >
> > Because of short RMPP. It can add back at least the header check part
> > but I think the partial write (long writes) is the fundamental issue.
>
> Yes, some check is needed. And above length encoded in sim_request header
> can be useful.
A header check can be left there. It's insufficient IMO though.
> >
> > > > @@ -411,7 +407,7 @@ static ssize_t umad2sim_read(struct umad2sim_dev *dev, void *buf, size_t count)
> > > > umad->status = ntohl(req.status);
> > > > umad->timeout_ms = 0;
> > > > umad->retries = 0;
> > > > - umad->length = umad_size() + sizeof(req.mad);
> > > > + umad->length = umad_size() + cnt;
> > >
> > > 'cnt' is normally actual MAD length + sim_request header size. Is it
> > > correct to put it as is in umad->length (shouldn't be umad_size() + cnt
> > > - sizeof(struct sim_request))?
> >
> > Not sure about this. I'll need to get back on this.
> >
> > > >
> > > > umad->addr.qpn = req.sqp;
> > > > umad->addr.qkey = 0; // agent->qkey;
> > > > @@ -431,9 +427,9 @@ static ssize_t umad2sim_read(struct umad2sim_dev *dev, void *buf, size_t count)
> > > > static ssize_t umad2sim_write(struct umad2sim_dev *dev,
> > > > const void *buf, size_t count)
> > > > {
> > > > - struct sim_request req;
> > > > + struct sim_request *req;
> > > > ib_user_mad_t *umad = (ib_user_mad_t *) buf;
> > > > - int cnt;
> > > > + int cnt, ocnt;
> > > >
> > > > #ifdef SIMULATE_SEND_ERRORS
> > > > { static int err_count;
> > > > @@ -464,25 +460,32 @@ static ssize_t umad2sim_write(struct umad2sim_dev *dev,
> > > > mad_get_field(umad_get_mad(umad), 0, IB_MAD_ATTRMOD_F)
> > > > );
> > > >
> > > > - req.dlid = umad->addr.lid;
> > > > - req.slid = req.dlid == 0xffff ? 0xffff : 0; /* 0 - means auto
> > > > + cnt = count - umad_size();
> > > > + if (cnt > MAD_BLOCK_SIZE)
> > > > + cnt = MAD_BLOCK_SIZE;
> > > > + req = malloc(sizeof(*req) + cnt);
> > >
> > > So why malloc() if size is limited by struct sim_request anyway?
> >
> > Because long RMPP isn't handled yet.
>
> Yes, and it is why I asked :). I don't think we need to touch this
> function yet for "for future" reason - long RMPP will require much more
> massive changes.
I guess at this point I'm unclear about moving forward. The only clear
thing to me is that with long RMPP it might be accepted.
-- Hal
> Sasha
> _______________________________________________
> general mailing list
> general at lists.openfabrics.org
> http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
>
> To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
More information about the general
mailing list