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

[Sasha Khapyorsky]

On 06:45 Mon 16 Jun     , Hal Rosenstock wrote:
> 
> >  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 ?

Yes, but mainly it was with huge 100M-1G RMPP packets - just fing this
not very useful. Didn't investigate howevert.

> Right; so maybe an additional (reliable) socket for only these
> request/response types ?

I think having two different types will complicate a lot.

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

If length is encoded in header you can check whole packet size.

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

With short RMPP too - it is better than nothing anyway :). I just think
that we need to avoid partial read/write bugs (maybe by using packet or
payload length in sim_request header).

Sasha