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

[Hal Rosenstock]

On Mon, 2008-06-16 at 17:54 +0300, Sasha Khapyorsky wrote:
> 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.

I thought it might too and didn't see how to cleanly do this.

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

It's only a partial measure. It doesn't account for dropped or
duplicated packets. It's UDP, right ?

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

OK; that's easy to add for short RMPP and things won't be any worse than
they are now as the issues aren't likely to show there. Long RMPP is a
whole 'nother matter...

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