netdev - Re: [PATCH net-next v2 08/13] bnxt_en: Refactor filter insertion logic in bnxt_rx_flow

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 25 Dec 2023 17:11:25 +0000
From: Simon Horman <horms@...nel.org>
To: Michael Chan <michael.chan@...adcom.com>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, pavan.chebbi@...adcom.com,
	andrew.gospodarek@...adcom.com
Subject: Re: [PATCH net-next v2 08/13] bnxt_en: Refactor filter insertion
 logic in bnxt_rx_flow_steer().

On Fri, Dec 22, 2023 at 08:22:05PM -0800, Michael Chan wrote:
> Add a new function bnxt_insert_ntp_filter() to insert the ntuple filter
> into the hash table and other basic setup.  We'll use this function
> to insert a user defined filter from ethtool.
> 
> Also, export bnxt_lookup_ntp_filter_from_idx() and bnxt_get_ntp_filter_idx()
> for similar purposes.  All ntuple related functions are now no longer
> compiled only for CONFIG_RFS_ACCEL
> 
> Reviewed-by: Vasundhara Volam <vasundhara-v.volam@...adcom.com>
> Reviewed-by: Andy Gospodarek <andrew.gospodarek@...adcom.com>
> Reviewed-by: Pavan Chebbi <pavan.chebbi@...adcom.com>
> Signed-off-by: Michael Chan <michael.chan@...adcom.com>

...

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c

...

> @@ -13991,33 +13995,20 @@ static int bnxt_rx_flow_steer(struct net_device *dev, const struct sk_buff *skb,
>  	}
>  	rcu_read_unlock();
>  
> -	spin_lock_bh(&bp->ntp_fltr_lock);
> -	bit_id = bitmap_find_free_region(bp->ntp_fltr_bmap, BNXT_MAX_FLTR, 0);
> -	if (bit_id < 0) {
> -		spin_unlock_bh(&bp->ntp_fltr_lock);
> -		rc = -ENOMEM;
> -		goto err_free;
> -	}
> -
> -	new_fltr->base.sw_id = (u16)bit_id;
>  	new_fltr->flow_id = flow_id;
>  	new_fltr->base.rxq = rxq_index;
> -	new_fltr->base.type = BNXT_FLTR_TYPE_NTUPLE;
> -	new_fltr->base.flags = BNXT_ACT_RING_DST;
> -	hlist_add_head_rcu(&new_fltr->base.hash, head);
> -	set_bit(BNXT_FLTR_INSERTED, &new_fltr->base.state);
> -	bp->ntp_fltr_count++;
> -	spin_unlock_bh(&bp->ntp_fltr_lock);
> -
> -	bnxt_queue_sp_work(bp, BNXT_RX_NTP_FLTR_SP_EVENT);
> -
> -	return new_fltr->base.sw_id;
> +	rc = bnxt_insert_ntp_filter(bp, new_fltr, idx);
> +	if (!rc) {
> +		bnxt_queue_sp_work(bp, BNXT_RX_NTP_FLTR_SP_EVENT);
> +		return new_fltr->base.sw_id;
> +	}

Hi Michael,

FIWIW, I think the following would be a more idomatic flow.
(Completely untested!)

	rc = bnxt_insert_ntp_filter(bp, new_fltr, idx);
	if (rc)
		goto err_free;

	bnxt_queue_sp_work(bp, BNXT_RX_NTP_FLTR_SP_EVENT);

	return new_fltr->base.sw_id;

>  
>  err_free:
>  	bnxt_del_l2_filter(bp, l2_fltr);
>  	kfree(new_fltr);
>  	return rc;
>  }
> +#endif
>  
>  static void bnxt_cfg_ntp_filters(struct bnxt *bp)
>  {

...