Re: [PATCH 2/4] IB/isert: convert to new CQ API

Christoph Hellwig
2016-02-17T14:18:52Z

On Tue, Feb 16, 2016 at 08:04:40PM +0200, Max Gurtovoy wrote:
>> @@ -977,7 +959,10 @@ isert_post_recvm(struct isert_conn *isert_conn, u32 count)
>>
>> for (rx_wr = isert_conn->rx_wr, i = 0; i < count; i++, rx_wr++) {
>> rx_desc = &isert_conn->rx_descs[i];
>> - rx_wr->wr_id = (uintptr_t)rx_desc;
>> +
>> + rx_desc->rx_cqe.done = isert_recv_done;
>
> perhaps we can do it once in isert_alloc_rx_descriptors ?

Ok.

>> +static void
>> +isert_recv_done(struct ib_cq *cq, struct ib_wc *wc)
>> {
>> + struct isert_conn *isert_conn = wc->qp->qp_context;
>> + struct ib_device *ib_dev = isert_conn->cm_id->device;
>> + struct iser_rx_desc *rx_desc =
>> + container_of(wc->wr_cqe, struct iser_rx_desc, rx_cqe);
>
> maybe we can define it as:
> static inline struct iser_rx_desc *
> isert_rx(struct ib_cqe *rx_cqe)
> {
> return container_of(rx_cqe, struct iser_rx_desc, rx_cqe);
> }
> in the .h file ?

There is just a single use of this pattern, but if everyone prefers
the helper I can add it.

>> + struct iscsi_hdr *hdr = &rx_desc->iscsi_header;
>> struct iser_ctrl *iser_ctrl = &rx_desc->iser_header;
>> uint64_t read_va = 0, write_va = 0;
>
>
>
>> + if (unlikely(wc->status != IB_WC_SUCCESS)) {
>> + isert_print_wc(wc);
>> + if (!--isert_conn->post_recv_buf_count)
>
> maybe we can do isert_conn->post_recv_buf_count-- once at the beginning of
> the function?

I'd rather not move the assignment around in this patch. If you
think this can sensible be done please send a follow up patch that
also explains why it's safe.

> maybe we can do isert_conn->post_recv_buf_count-- once at the beginning of
> the function?

Same as above.

>> + struct iser_tx_desc *desc =
>> + container_of(wc->wr_cqe, struct iser_tx_desc, tx_cqe);
>
> maybe we can define it as:
> static inline struct iser_tx_desc *
> isert_tx(struct ib_cqe *tx_cqe)
> {
> return container_of(tx_cqe, struct iser_tx_desc, tx_cqe);
> }

We've got a couple more instances of this one, so this might be
worth it.