| 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: <fe42e84d-3b78-4bba-b1a3-27fe89625809@linux.intel.com>
Date: Mon, 23 Dec 2024 17:32:03 +0800
From: "Abdul Rahim, Faizal" <faizal.abdul.rahim@...ux.intel.com>
To: Vladimir Oltean <olteanv@...il.com>, Furong Xu <0x1207@...il.com>
Cc: Tony Nguyen <anthony.l.nguyen@...el.com>,
Przemek Kitszel <przemyslaw.kitszel@...el.com>,
Andrew Lunn <andrew+netdev@...n.ch>, "David S . Miller"
<davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>,
Jesper Dangaard Brouer <hawk@...nel.org>,
John Fastabend <john.fastabend@...il.com>,
Vinicius Costa Gomes <vinicius.gomes@...el.com>,
intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, bpf@...r.kernel.org
Subject: Re: [PATCH iwl-next 6/9] igc: Add support for frame preemption
verification
On 17/12/2024 8:22 am, Vladimir Oltean wrote:
>> diff --git a/drivers/net/ethernet/intel/igc/igc_defines.h b/drivers/net/ethernet/intel/igc/igc_defines.h
>> index 3088cdd08f35..ba96776d5854 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_defines.h
>> +++ b/drivers/net/ethernet/intel/igc/igc_defines.h
>> @@ -308,6 +308,8 @@
>> #define IGC_TXD_DTYP_C 0x00000000 /* Context Descriptor */
>> #define IGC_TXD_POPTS_IXSM 0x01 /* Insert IP checksum */
>> #define IGC_TXD_POPTS_TXSM 0x02 /* Insert TCP/UDP checksum */
>> +#define IGC_TXD_POPTS_SMD_V 0x10 /* Transmitted packet is a SMD-Verify */
>> +#define IGC_TXD_POPTS_SMD_R 0x20 /* Transmitted packet is a SMD-Response */
>> #define IGC_TXD_CMD_EOP 0x01000000 /* End of Packet */
>> #define IGC_TXD_CMD_IC 0x04000000 /* Insert Checksum */
>> #define IGC_TXD_CMD_DEXT 0x20000000 /* Desc extension (0 = legacy) */
>> @@ -370,9 +372,13 @@
>> #define IGC_RXD_STAT_VP 0x08 /* IEEE VLAN Packet */
>>
>> #define IGC_RXDEXT_STATERR_LB 0x00040000
>> +#define IGC_RXD_STAT_SMD_V 0x2000 /* SMD-Verify packet */
>> +#define IGC_RXD_STAT_SMD_R 0x4000 /* SMD-Response packet */
>>
>> /* Advanced Receive Descriptor bit definitions */
>> #define IGC_RXDADV_STAT_TSIP 0x08000 /* timestamp in packet */
>> +#define IGC_RXDADV_STAT_SMD_TYPE_MASK 0x06000
>> +#define IGC_RXDADV_STAT_SMD_TYPE_SHIFT 13
>>
>> #define IGC_RXDEXT_STATERR_L4E 0x20000000
>> #define IGC_RXDEXT_STATERR_IPE 0x40000000
>> diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
>> index 1954561ec4aa..7cde0e5a7320 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
>> +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
>> @@ -1788,6 +1788,7 @@ static int igc_ethtool_set_mm(struct net_device *netdev,
>> {
>> struct igc_adapter *adapter = netdev_priv(netdev);
>> struct fpe_t *fpe = &adapter->fpe;
>> + bool verify_enabled_changed;
>>
>> if (cmd->tx_min_frag_size < IGC_TX_MIN_FRAG_SIZE ||
>> cmd->tx_min_frag_size > IGC_TX_MAX_FRAG_SIZE)
>> @@ -1805,7 +1806,12 @@ static int igc_ethtool_set_mm(struct net_device *netdev,
>>
>> fpe->tx_enabled = cmd->tx_enabled;
>> fpe->pmac_enabled = cmd->pmac_enabled;
>> - fpe->verify_enabled = cmd->verify_enabled;
>> + verify_enabled_changed = (cmd->verify_enabled != fpe->verify_enabled);
>
> I wonder if it's worth using an intermediary variable when the result is
> only evaluated once. The intention is clear enough already, you call a
> function named igc_fpe_verify_enabled_changed().
>
yea when I read this part again, I don't this it's needed.
Will remove the new local var and change the code to:
if (cmd->verify_enabled != fpe->verify_enabled) {
fpe->verify_enabled = cmd->verify_enabled;
igc_fpe_verify_enabled_changed(fpe);
>> +
>> + if (verify_enabled_changed) {
>> + fpe->verify_enabled = cmd->verify_enabled;
>> + igc_fpe_verify_enabled_changed(fpe);
>> + }
>>
>> return igc_tsn_offload_apply(adapter);
>> }
>> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
>> index b85eaf34d07b..e184959ef218 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_main.c
>> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
>> @@ -2534,7 +2534,7 @@ static struct sk_buff *igc_xdp_run_prog(struct igc_adapter *adapter,
>> }
>>
>> /* This function assumes __netif_tx_lock is held by the caller. */
>> -static void igc_flush_tx_descriptors(struct igc_ring *ring)
>> +void igc_flush_tx_descriptors(struct igc_ring *ring)
>> {
>> /* Once tail pointer is updated, hardware can fetch the descriptors
>> * any time so we issue a write membar here to ensure all memory
>> @@ -2585,6 +2585,7 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
>> struct sk_buff *skb = rx_ring->skb;
>> u16 cleaned_count = igc_desc_unused(rx_ring);
>> int xdp_status = 0, rx_buffer_pgcnt;
>> + int smd_type;
>>
>> while (likely(total_packets < budget)) {
>> struct igc_xdp_buff ctx = { .rx_ts = NULL };
>> @@ -2622,6 +2623,18 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
>> size -= IGC_TS_HDR_LEN;
>> }
>>
>> + smd_type = igc_fpe_get_smd_type(rx_desc->wb.upper.status_error);
>> +
>> + if (igc_fpe_is_verify_or_response(smd_type, size)) {
>> + igc_fpe_preprocess_verify_response(&adapter->fpe,
>> + smd_type);
>> +
>> + /* Advance the ring next-to-clean */
>> + igc_is_non_eop(rx_ring, rx_desc);
>> + cleaned_count++;
>> + continue;
>> + }
>> +
>
> Premature optimization is the root of all evil, I know, but in the
> future it might be interesting to add a static key here that gets
> incremented (enabled) based on pmac_enabled, such that the fast path
> does not get to suffer a performance penalty when frame preemption is
> supported in the kernel, regardless of whether it is enabled or not.
>
You mean something like this ?
igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
{
static bool pmac_enabled = adapter->fpe.pmac_enabled;
..
..
if (pmac_enabled && (igc_fpe_is_verify_or_response(smd_type, size))
Powered by blists - more mailing lists