[Intel-wired-lan] [PATCH iwl-next 6/9] igc: Add support for frame preemption verification
Abdul Rahim, Faizal
faizal.abdul.rahim at linux.intel.com
Mon Dec 23 09:32:03 UTC 2024
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))
More information about the Intel-wired-lan
mailing list