[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <5a166754-390a-46f8-9861-38bc90111c4f@grimberg.me>
Date: Wed, 13 Dec 2023 11:49:20 +0200
From: Sagi Grimberg <sagi@...mberg.me>
To: Max Gurtovoy <mgurtovoy@...dia.com>, Aurelien Aptel <aaptel@...dia.com>,
linux-nvme@...ts.infradead.org, netdev@...r.kernel.org, hch@....de,
kbusch@...nel.org, axboe@...com, chaitanyak@...dia.com, davem@...emloft.net,
kuba@...nel.org
Cc: Boris Pismenny <borisp@...dia.com>, aurelien.aptel@...il.com,
smalin@...dia.com, malin1024@...il.com, ogerlitz@...dia.com,
yorayz@...dia.com, galshalom@...dia.com, brauner@...nel.org
Subject: Re: [PATCH v20 05/20] nvme-tcp: Add DDP offload control path
> Hi Sagi,
>
> On 29/11/2023 15:52, Aurelien Aptel wrote:
>> Hi Sagi,
>>
>> Sagi Grimberg <sagi@...mberg.me> writes:
>>>> + ok = ulp_ddp_query_limits(netdev, &ctrl->ddp_limits,
>>>> + ULP_DDP_NVME, ULP_DDP_CAP_NVME_TCP,
>>>> + ctrl->ctrl.opts->tls);
>>>> + if (!ok) {
>>>
>>> please use a normal name (ret).
>>
>> Ok, we will rename to ret and make ulp_ddp_query_limits() return int 0
>> on success to be consistent with the name.
>>
>>> Plus, its strange that a query function receives a feature and returns
>>> true/false based on this. The query should return the limits, and the
>>> caller should look at the limits and see if it is appropriately
>>> supported.
>>
>> We are not sure how to proceed as this seems to conflict with what you
>> suggested in v12 [1] about hiding the details of checking supports in
>> the API. Limits just dictate some constants the nvme-layer should use
>> once we know it is supported.
>>
>> We can rename ulp_ddp_query_limits() to ulp_ddp_check_support(). This
>> function checks the support of the specified offload capability and also
>> returns the limitations of it.
>>
>> Alternatively, we can split it in 2 API functions (check_support
>> and query_limits).
>>
>> Let us know what you prefer.
>>
>> Thanks
>>
>> 1:
>> https://lkml.kernel.org/netdev/bc5cd2a7-efc4-e4df-cae5-5c527dd704a6@grimberg.me/
>
> We would like to submit another version by the end of this week and
> hopefully progress with this series.
> To address your comment, I was thinking about something like:
>
> + if (!ulp_ddp_is_cap_active(netdev, ULP_DDP_CAP_NVME_TCP))
> + goto err;
> +
> + ret = ulp_ddp_get_limits(netdev, &ctrl->ddp_limits, ULP_DDP_NVME);
> + if (ret)
> + goto err;
> +
> + if (ctrl->ctrl.opts->tls && !ctrl->ddp_limits.tls)
> + goto err;
>
> I would like to remind you that the community didn't want to add ulp_ddp
> caps and limits context to the netdev structure (as we have for example
> in the block layer q) and preferred the design of ops/cbs.
Seems fine to me.
Powered by blists - more mailing lists