[openib-general] [PATCH 3/6] [RFC] iser initiator

[Or Gerlitz]

Christoph Hellwig wrote:
>> +#define ISER_HDR_LEN            sizeof (struct iser_hdr)
>> +#define ISER_PDU_BHS_LENGTH     sizeof (struct iscsi_hdr)

> these two macros are just use in ISER_TOTAL_HEADERS_LEN below,
> just kill them.

OK

>> +#define USE_OFFSET(offset)      (offset)
>> +#define USE_NO_OFFSET           0
>> +#define USE_SIZE(size)          (size)
>> +#define USE_ENTIRE_SIZE         0

> please kill these macros.

OK

> Please kill the p_ prefix for pointer types all over the code.

OK

>> +static int iser_post_receive_control(struct iscsi_iser_conn *p_iser_conn)
...
>> +	rx_desc = kmem_cache_alloc(ig.desc_cache,
>> +				      GFP_KERNEL | __GFP_NOFAIL);

> __GFP_NOFAIL doesn't work for slab (kmem_cache_alloc/kmalloc/kzalloc/kcalloc)
> allocations

I see. The code has runtime memory allocations for the following cases:
+1 rx descriptor
+2 rx control buffer
+3 tx unsolicited dataout descriptor
+4 data buffer for the copy of SG which is an unaligned for rdma
+5 page vec to be used for the FMR mapping on an SG

The original thought was that with the GFP_NOFAIL we can simplify
the code (eg less error paths). Anyway, re thinking it:

Having types +1 and +2 being done at conn setup time with GFP_KERNEL can 
be done without much pain.

As for +3 (unsolicited dataouts) iscsi_tcp solved it by using a mempool.
I have few ideas how to solve it here, so this way or another +3 will
be also modified not to assume a _NOFAIL

type +5 is easy to remove, anyway commands processing is serializes
so the connection can hold a page vec of the maximum size (128) to be
used for all our fmr mappings.

As for type +4, i am looking for the simplest solution since it is
very rare case. But again, OK, we will not assume NOFAIL also here.

>> +send_data_out_error:
>> +	if (p_send_dto != NULL)
>> +		iser_dto_buffs_release(p_send_dto);
>> +	if (tx_desc != NULL)
>> +		kmem_cache_free(ig.desc_cache, tx_desc);

> could you please do the same goto-unwinding style we use elsewhere
> in the kernel?  That is one label before each unwind step and jump
> directly to that instead of adding tons of conditionals in the error path.

OK