> -----Original Message-----
> From: Johannes Berg <johannes@xxxxxxxxxxxxxxxx>
> Sent: Monday, December 20, 2021 3:59 PM
> To: Sriram R (QUIC) <quic_srirrama@xxxxxxxxxxx>; linux-
> wireless@xxxxxxxxxxxxxxx
> Subject: Re: [RFC v2] mac80211: Mesh Fast xmit support
>
> WARNING: This email originated from outside of Qualcomm. Please be wary
> of any links or attachments, and do not enable macros.
>
> 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?
Yes Sure, I'll remove this flag and replace with necessary checks in the entry addition function itself.
>
>
> > +/**
> > + * struct mesh_hdr_cache
>
> should have a description there, if it's kernel-doc.
I'll update it.
>
> > + * @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?
Sure, agree Johannes, it seems unnecessary to have separate allocation, I'll move to the mesh struct.
>
> >
> > + 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.
Right, I just reviewed and it seems everywhere its either inside or just outside the lock.
>
> Also are you sure you don't have to put the rhashtable change under the
> spinlock??
Thanks, I missed it, should be under the lock.
>
>
> > @@ -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.
Right, I'll have these checks while caching too.
Thanks for the review. Will address these comments along with suggestions provided by Felix, and
if there are any other comments/concerns from others with this RFC version.
Regards,
Sriram.R
>
> johannes
