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

On 8/10/23 17:48, Aurelien Aptel wrote:

Sagi Grimberg [off-list ref] writes:

quoted

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

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

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?

Hmm, no you're right.

This makes me wonder if we have some places to convert test_bit to
test_bit_acquire where we must not make forward progress.

This particular condition is fine I think...