Message-ID: <7cc08b19d3ac643ed21c5c4325a4a8a64c4233f6.camel@sipsolutions.net>
Subject: Re: [RFC 4/5] wifi: mac80211: mesh fast xmit support
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Felix Fietkau <nbd@nbd.name>, linux-wireless@vger.kernel.org
Cc:     Sriram R <quic_srirrama@quicinc.com>
Date:   Mon, 27 Feb 2023 14:14:06 +0100
In-Reply-To: <20230227110738.54241-4-nbd@nbd.name>
References: <20230227110738.54241-1-nbd@nbd.name>
         <20230227110738.54241-4-nbd@nbd.name>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.3 (3.46.3-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

On Mon, 2023-02-27 at 12:07 +0100, Felix Fietkau wrote:
>=20
> +	/* If the skb is shared we need to obtain our own copy */
> +	if (skb_shared(skb)) {
> +		struct sk_buff *oskb =3D skb;
> +
> +		skb =3D skb_clone(skb, GFP_ATOMIC);
> +		if (!skb)
> +			return false;
> +
> +		kfree_skb(oskb);
> +	}

Use skb_share_check()?

>  	next_hop =3D rcu_dereference(mpath->next_hop);
>  	if (next_hop) {
>  		memcpy(hdr->addr1, next_hop->sta.addr, ETH_ALEN);
>  		memcpy(hdr->addr2, sdata->vif.addr, ETH_ALEN);
>  		ieee80211_mps_set_frame_flags(sdata, next_hop, hdr);
> +		if (ieee80211_hw_check(&sdata->local->hw, SUPPORT_FAST_XMIT))
> +			mesh_fast_tx_cache(sdata, skb, mpath);


I wondered briefly if it's worth moving that check into the function,
but not sure.

> +	/* rate limit, in case fast xmit can't be enabled */
> +	if (mppath->fast_tx_check =3D=3D jiffies)
> +		return;
> +
> +	mppath->fast_tx_check =3D jiffies;

once every jiffies seems pretty frequent though?

> +	spin_lock_bh(&cache->walk_lock);

you could just spin_lock() the inner lock, _bh already taken care of by
the outer?

> @@ -3686,7 +3690,7 @@ static void __ieee80211_xmit_fast(struct ieee80211_=
sub_if_data *sdata,
>  #endif
> =20
>  	if (hdr->frame_control & cpu_to_le16(IEEE80211_STYPE_QOS_DATA)) {
> -		tid =3D skb->priority & IEEE80211_QOS_CTL_TAG1D_MASK;
> +		u8 tid =3D skb->priority & IEEE80211_QOS_CTL_TAG1D_MASK;
>  		*ieee80211_get_qos_ctl(hdr) =3D tid;

That's ... interesting, why wss there an argument in the first place?

but maybe add a blank line now :)

johannes