Re: [PATCH iwl-next 6/9] igc: Add support for frame preemption verification

[Abdul Rahim, Faizal]

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))