[ofa-general] Re: [PATCH] ibsim: Support short RMPP packets

[Sasha Khapyorsky]

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. 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 it is mostly due to long packets.

> > > 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' and leave a lot of code unmodified.

> 
> > >  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.

> 
> > > @@ -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.

Sasha