[ofw] [RFC] winverbs implementation approach

[Tzachi Dar]

Hi,

Here are my comments to the code:

1) Once there is an error, please also trace it. This makes debugging
much easier.

2) The current code allocates memory from the heap for every operation
and frees it later.
Allocating memory is relatively an expensive task and should be avoided
if possibale.
One possibale way to do that is to use a buffer on the heap for most
ioctls and allocate only if there is a need for a bigger buffer.

3) My recommendation is not to have a return in the middle of a function
but to use the "goto cleanup" mechanism. From my experience this makes
the code more maintainable.

4) In the case of an error in allocating the memory, the call to
post_create_cq is not done. Is this by intention?

Thanks
Tzachi

> -----Original Message-----
> From: ofw-bounces at lists.openfabrics.org 
> [mailto:ofw-bounces at lists.openfabrics.org] On Behalf Of Sean Hefty
> Sent: Thursday, March 13, 2008 1:56 AM
> To: Hefty, Sean; 'Fab Tillier'; ofw at lists.openfabrics.org
> Subject: [ofw] [RFC] winverbs implementation approach
> 
> If you ignore the poorly spaced tabs from outlook, the 
> following is the first attempt at implementing the WinVerbs 
> IWVCompletionQueue::Create() routine.
> 
> A goal is to limit changes to the existing userspace or 
> kernel stacks, especially changes that may impact existing 
> stack stability.  The winverbs library communicates directly 
> with the userspace verbs provider library.
> It issues buffered IOCTLs directly to a kernel winverbs 
> filter driver that communicates directly with the kernel HCA 
> driver for verb related functionality.
> (I.e. CQ, QP, MR, MW, PD, SRQ, etc. type operations.)
> 
> The intent is that that winverbs kernel driver will implement 
> the asynchronous operations until the HCA driver is optimized 
> for this at some undetermined future point in time.
> 
> What's not shown is that default pre/post calls are provided 
> if the provider does not define one.  This simplifies the 
> code.  The ci_umv_buf_t structure is not passed directly to 
> the kernel winverb driver.  The provider data is copied to 
> the end of the IOCTL data itelf.  The winverb driver 
> reconstructs the ci_umv_buf_t before calling the HCA driver.  
> 
> - Sean
> ---
> 
> STDMETHODIMP CWVCompletionQueue::
> Create(SIZE_T *pEntries)
> {
> 	WV_IO_CQ_ATTRIBUTES	*pattr;
> 	DWORD				bytes;
> 	ib_api_status_t		stat;
> 	HRESULT			hr;
> 	ci_umv_buf_t		verbsdata;
> 
> 	stat = 
> m_pVerbs->pre_create_cq(m_pDevice->m_hVerbsDevice, (UINT32 *) 
> pEntries,
> 					   &verbsdata, &m_hVerbsCq);
> 	if (stat)
> 		return WvConvertIbStatus(stat);
> 
> 	bytes = sizeof WV_IO_CQ_ATTRIBUTES + max(verbsdata.input_size,
> 								
> verbsdata.output_size);
> 	pattr = (WV_IO_CQ_ATTRIBUTES *) 
> HeapAlloc(GetProcessHeap(), 0, bytes);
> 	if (!pattr)
> 		return WV_NO_MEMORY;
> 
> 	pattr->Id.Id = m_pDevice->m_Id;
> 	pattr->Id.VerbInfo = verbsdata.command;
> 	pattr->Id.Reserved = 0;
> 	pattr->Entries = *pEntries;
> 	RtlCopyMemory(pattr + 1, verbsdata.p_inout_buf, 
> verbsdata.input_size);
> 
> 	if (DeviceIoControl(m_hFile, WV_IOCTL_CQ_CREATE,
> 				pattr, sizeof 
> WV_IO_CQ_ATTRIBUTES + verbsdata.input_size,
> 				pattr, sizeof 
> WV_IO_CQ_ATTRIBUTES + verbsdata.output_size,
> 				&bytes, NULL)) {
> 		hr = WV_SUCCESS;
> 		m_Id = pattr->Id.Id;
> 		*pEntries = pattr->Entries;
> 	} else
> 		hr = HRESULT_FROM_WIN32(GetLastError());
> 
> 	verbsdata.status = pattr->Id.VerbInfo;
> 	RtlCopyMemory(verbsdata.p_inout_buf, pattr + 1, 
> verbsdata.output_size);
> 	HeapFree(GetProcessHeap(), 0, pattr);
> 
> 	m_pVerbs->post_create_cq(m_pDevice->m_hVerbsDevice, 
> (ib_api_status_t) hr,
> 					 (UINT32) *pEntries, 
> &m_hVerbsCq, &verbsdata);
> 	return hr;
> }
> 
> _______________________________________________
> ofw mailing list
> ofw at lists.openfabrics.org
> http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ofw
>