Re: [PATCH v12 09/26] nvme-tcp: RX DDGST offload

Sagi Grimberg [off-list ref] writes:

grr.. wondering if this is something we want to support (crc without
ddp).

We agree, we don't want to support it. We will remove it and check it
doesn't happen in is_netdev_offload_active().

quoted

+     req->ddp.sg_table.sgl = req->ddp.first_sgl;

Why is this assignment needed? why not pass req->ddp.first_sgl ?

Correct, this assignment is not needed we will remove it.

quoted

  static void nvme_tcp_error_recovery(struct nvme_ctrl *ctrl)

@@ -1047,7 +1126,8 @@ static int nvme_tcp_recv_pdu(struct nvme_tcp_queue *queue, struct sk_buff *skb,
      size_t rcv_len = min_t(size_t, *len, queue->pdu_remaining);
      int ret;

-     if (test_bit(NVME_TCP_Q_OFF_DDP, &queue->flags))
+     if (test_bit(NVME_TCP_Q_OFF_DDP, &queue->flags) ||
+         test_bit(NVME_TCP_Q_OFF_DDGST_RX, &queue->flags))

This now becomes two atomic bitops to check for each capability, where
its more likely that neighther are on...

Is this really racing with anything? maybe just check with bitwise AND?
or a local variable (or struct member)
I don't think that we should add any more overhead for the normal path
than we already have.

Are you sure test_bit() is atomic? The underlying definitions seems
non-atomic (constant_test_bit or const_test_bit), are we missing
anything?

We were also following a similar implementation to NVME_TCP_Q_POLLING
which was using test_bit(). Should we move to a regular bool flag like
queue->data_digest?

quoted

+     if (queue->data_digest &&
+         test_bit(NVME_TCP_Q_OFF_DDGST_RX, &queue->flags))

And a third atomic bitop..

See above

quoted

+                 !test_bit(NVME_TCP_Q_OFF_DDGST_RX, &queue->flags))

and a 4'th atomic bitop...

See above

quoted

+     if (test_bit(NVME_TCP_Q_OFF_DDGST_RX, &queue->flags))
+             nvme_tcp_ddp_ddgst_update(queue, skb);

and a 5'th atomic bitop...

See above

quoted

+     if (test_bit(NVME_TCP_Q_OFF_DDGST_RX, &queue->flags)) {

and a 6'th... ok this is just spraying atomic bitops on the data
path. Please find a better solution to this.

See above

Thanks