On Sun, 2021-12-05 at 06:47 +0530, Sriram R wrote:
>
> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index 775dbb9..089fbb7 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -868,6 +868,8 @@ enum mac80211_tx_info_flags {
> * @IEEE80211_TX_CTRL_DONT_REORDER: This frame should not be reordered
> * relative to other frames that have this flag set, independent
> * of their QoS TID or other priority field values.
> + * @IEEE80211_TX_CTRL_CHECK_FAST_MESH: During Mesh xmit, the header of this
> + * frame can be cached for faster lookup later.
> *
> * These flags are used in tx_info->control.flags.
> */
> @@ -881,6 +883,7 @@ enum mac80211_tx_control_flags {
> IEEE80211_TX_INTCFL_NEED_TXPROCESSING = BIT(6),
> IEEE80211_TX_CTRL_NO_SEQNO = BIT(7),
> IEEE80211_TX_CTRL_DONT_REORDER = BIT(8),
> + IEEE80211_TX_CTRL_CHECK_FAST_MESH = BIT(9),
It would be nice if we could get away without this, and shouldn't it
anyway be an internal flag or so, not sure why the driver needs to know?
> +/**
> + * struct mesh_hdr_cache
should have a description there, if it's kernel-doc.
> + * @rhead: the rhashtable containing header cache entries
> + * @walk_head: linked list containing all cached header entries
> + * @walk_lock: lock protecting walk_head
> + * @size: number of entries in the header cache
> + */
> +struct mesh_hdr_cache {
> + struct rhashtable rhead;
> + struct hlist_head walk_head;
> + /* protects header hlist */
> + spinlock_t walk_lock;
> + atomic_t size;
> +};
However, is it even worth keeping the few variables here in a separate
allocation?
Mesh might not even be the largest user of space in the interface union,
so perhaps inlining the struct makes sense?
> +static void mesh_hdr_cache_init(struct ieee80211_sub_if_data *sdata)
> +{
> + struct ieee80211_local *local = sdata->local;
> + struct mesh_hdr_cache *cache;
> +
> + sdata->u.mesh.hdr_cache = NULL;
> +
> + if (!ieee80211_hw_check(&local->hw, SUPPORT_FAST_XMIT))
> + return;
> +
> + cache = kmalloc(sizeof(*cache), GFP_ATOMIC);
And if we keep this outside, surely that need not be GFP_ATOMIC?
>
> + spin_lock_bh(&cache->walk_lock);
> + hlist_add_head(&mhdr->walk_list, &cache->walk_head);
> + spin_unlock_bh(&cache->walk_lock);
> +
> + atomic_inc(&cache->size);
There's no point in keeping cache->size as an atomic_t, you always
access it very near the spinlock. Better just move it under the
spinlock.
Also are you sure you don't have to put the rhashtable change under the
spinlock??
> @@ -425,6 +808,7 @@ struct mesh_path *mesh_path_add(struct ieee80211_sub_if_data *sdata,
> mesh_rht_params);
> if (!mpath)
> hlist_add_head(&new_mpath->walk_list, &tbl->walk_head);
> +
> spin_unlock_bh(&tbl->walk_lock);
Unrelated change
> + /* TODO reduce/combine multiple checks which aren't per packet */
> + if (ifmsh->mshcfg.dot11MeshNolearn)
> + return false;
> +
> + if (!ieee80211_hw_check(&local->hw, SUPPORT_FAST_XMIT))
> + return false;
> +
> + if (sdata->noack_map)
> + return false;
Yeah, just don't create cache entries in those cases?
Saves memory (for the more interesting cases) too.
johannes
