| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1b386103-4c6a-43ea-821f-50a570996669@amd.com>
Date: Wed, 11 Sep 2024 10:34:58 -0700
From: Brett Creeley <bcreeley@....com>
To: Taehee Yoo <ap420073@...il.com>
Cc: davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
edumazet@...gle.com, corbet@....net, michael.chan@...adcom.com,
netdev@...r.kernel.org, linux-doc@...r.kernel.org, ecree.xilinx@...il.com,
przemyslaw.kitszel@...el.com, andrew@...n.ch, hkallweit1@...il.com,
kory.maincent@...tlin.com, ahmed.zaki@...el.com, paul.greenwalt@...el.com,
rrameshbabu@...dia.com, idosch@...dia.com, maxime.chevallier@...tlin.com,
danieller@...dia.com, aleksander.lobakin@...el.com
Subject: Re: [PATCH net-next v2 4/4] bnxt_en: add support for
tcp-data-split-thresh ethtool command
On 9/11/2024 9:32 AM, Taehee Yoo wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On Thu, Sep 12, 2024 at 12:52 AM Brett Creeley <bcreeley@....com> wrote:
>
> Hi Brett,
> Thank you so much for your review!
>
>>
>>
>>
>> On 9/11/2024 7:55 AM, Taehee Yoo wrote:
>>> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>>>
>>>
>>> The bnxt_en driver has configured the hds_threshold value automatically
>>> when TPA is enabled based on the rx-copybreak default value.
>>> Now the tcp-data-split-thresh ethtool command is added, so it adds an
>>> implementation of tcp-data-split-thresh option.
>>>
>>> Configuration of the tcp-data-split-thresh is allowed only when
>>> the tcp-data-split is enabled. The default value of
>>> tcp-data-split-thresh is 256, which is the default value of rx-copybreak,
>>> which used to be the hds_thresh value.
>>>
>>> # Example:
>>> # ethtool -G enp14s0f0np0 tcp-data-split on tcp-data-split-thresh 256
>>> # ethtool -g enp14s0f0np0
>>> Ring parameters for enp14s0f0np0:
>>> Pre-set maximums:
>>> ...
>>> Current hardware settings:
>>> ...
>>> TCP data split: on
>>> TCP data split thresh: 256
>>>
>>> It enables tcp-data-split and sets tcp-data-split-thresh value to 256.
>>>
>>> # ethtool -G enp14s0f0np0 tcp-data-split off
>>> # ethtool -g enp14s0f0np0
>>> Ring parameters for enp14s0f0np0:
>>> Pre-set maximums:
>>> ...
>>> Current hardware settings:
>>> ...
>>> TCP data split: off
>>> TCP data split thresh: n/a
>>>
>>> Signed-off-by: Taehee Yoo <ap420073@...il.com>
>>> ---
>>>
>>> v2:
>>> - Patch added.
>>>
>>> drivers/net/ethernet/broadcom/bnxt/bnxt.c | 3 ++-
>>> drivers/net/ethernet/broadcom/bnxt/bnxt.h | 2 ++
>>> drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 9 +++++++++
>>> 3 files changed, 13 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>>> index f046478dfd2a..872b15842b11 100644
>>> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>>> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>>> @@ -4455,6 +4455,7 @@ static void bnxt_init_ring_params(struct bnxt *bp)
>>> {
>>> bp->rx_copybreak = BNXT_DEFAULT_RX_COPYBREAK;
>>> bp->flags |= BNXT_FLAG_HDS;
>>> + bp->hds_threshold = BNXT_DEFAULT_RX_COPYBREAK;
>>> }
>>>
>>> /* bp->rx_ring_size, bp->tx_ring_size, dev->mtu, BNXT_FLAG_{G|L}RO flags must
>>> @@ -6429,7 +6430,7 @@ static int bnxt_hwrm_vnic_set_hds(struct bnxt *bp, struct bnxt_vnic_info *vnic)
>>> VNIC_PLCMODES_CFG_REQ_FLAGS_HDS_IPV6);
>>> req->enables |=
>>> cpu_to_le32(VNIC_PLCMODES_CFG_REQ_ENABLES_HDS_THRESHOLD_VALID);
>>> - req->hds_threshold = cpu_to_le16(bp->rx_copybreak);
>>> + req->hds_threshold = cpu_to_le16(bp->hds_threshold);
>>> }
>>> req->vnic_id = cpu_to_le32(vnic->fw_vnic_id);
>>> return hwrm_req_send(bp, req);
>>> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
>>> index 35601c71dfe9..48f390519c35 100644
>>> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
>>> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
>>> @@ -2311,6 +2311,8 @@ struct bnxt {
>>> int rx_agg_nr_pages;
>>> int rx_nr_rings;
>>> int rsscos_nr_ctxs;
>>> +#define BNXT_HDS_THRESHOLD_MAX 256
>>> + u16 hds_threshold;
>>>
>>> u32 tx_ring_size;
>>> u32 tx_ring_mask;
>>> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
>>> index ab64d7f94796..5b1f3047bf84 100644
>>> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
>>> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
>>> @@ -839,6 +839,8 @@ static void bnxt_get_ringparam(struct net_device *dev,
>>> else
>>> kernel_ering->tcp_data_split = ETHTOOL_TCP_DATA_SPLIT_DISABLED;
>>>
>>> + kernel_ering->tcp_data_split_thresh = bp->hds_threshold;
>>> +
>>> ering->tx_max_pending = BNXT_MAX_TX_DESC_CNT;
>>>
>>> ering->rx_pending = bp->rx_ring_size;
>>> @@ -864,6 +866,12 @@ static int bnxt_set_ringparam(struct net_device *dev,
>>> return -EINVAL;
>>> }
>>>
>>> + if (kernel_ering->tcp_data_split_thresh > BNXT_HDS_THRESHOLD_MAX) {
>>> + NL_SET_ERR_MSG_MOD(extack,
>>> + "tcp-data-split-thresh size too big");
>>
>> Should you print the BNXT_HDS_THRESHOLD_MAX value here so the user knows
>> the max size?
>
> Okay, I will print BNXT_HDS_THRESHOLD_MAX value with extack message.
>
>>
>> Actually, does it make more sense for ethtool get_ringparam to query the
>> max threshold size from the driver and reject this in the core so all
>> drivers don't have to have this same kind of check?
>
> Ah, I didn't consider this.
> You mean that like ETHTOOL_A_RINGS_RX_MAX, right?
> So, we can check precise information in userspace without error.
>
> We can check tcp-data-split-threshold-max information like below.
> ethtool -g enp13s0f0np0
> Ring parameters for enp13s0f0np0:
> Pre-set maximums:
> RX: 2047
> RX Mini: n/a
> RX Jumbo: 8191
> TX: 2047
> TX push buff len: n/a
> TCP data split thresh: 256 <-- here
> Current hardware settings:
> RX: 511
> RX Mini: n/a
> RX Jumbo: 2044
> TX: 511
> RX Buf Len: n/a
> CQE Size: n/a
> TX Push: off
> RX Push: off
> TX push buff len: n/a
> TCP data split: on
> TCP data split thresh: 0
>
> I agree with this suggestion.
> So, I will try to add ETHTOOL_A_RINGS_TCP_DATA_SPLIT_THRESH_MAX
> option in the ethtool core unless there is no objection.
Yeah, I think this makes the most sense so all/any vendor drivers
implementing tcp-data-split-thresh don't have to worry about checking
against their max in their set_ringparam callback, they just have to set
their max in the get_ringparam callback.
You mentioned ETHTOOL_A_RINGS_RX_MAX, which there are already examples
of this in the kernel and other similar ring parameter max values.
Thanks,
Brett
>
>>
>> Thanks,
>>
>> Brett
>>
>>> + return -EINVAL;
>>> + }
>>> +
>>> if (netif_running(dev))
>>> bnxt_close_nic(bp, false, false);
>>>
>>> @@ -871,6 +879,7 @@ static int bnxt_set_ringparam(struct net_device *dev,
>>> case ETHTOOL_TCP_DATA_SPLIT_UNKNOWN:
>>> case ETHTOOL_TCP_DATA_SPLIT_ENABLED:
>>> bp->flags |= BNXT_FLAG_HDS;
>>> + bp->hds_threshold = (u16)kernel_ering->tcp_data_split_thresh;
>>> break;
>>> case ETHTOOL_TCP_DATA_SPLIT_DISABLED:
>>> bp->flags &= ~BNXT_FLAG_HDS;
>>> --
>>> 2.34.1
>>>
>>>
>
> Thanks a lot!
> Taehee Yoo
Powered by blists - more mailing lists