Hi Johannes,
>> +/**
>> + * struct mesh_hdr_cache
>
>need a short description, e.g.
>
>struct mesh_hdr_cache - mesh header cache entry
Sure, Will update.
>
>> + *
>> + * @enabled: Flag to denote if Header caching is enabled.
>> + * @rhead: the rhashtable containing struct mesh_cache_entry, keyed by
>addr_key which
>> + * For a 6addr format which is currently supported in the cache, the key
>> + * is the external destination address or a5
>
>I don't understand this - should the "which" (on the too long line) be removed?
>Can you please rewrite this?
Sure
>
>> + * @walk_head: linked list containing all mesh_cache_entry objects
>
>Also both here refer to mesh_cache_entry which doesn't exist anywhere?
>Did you mean 'struct mhdr_cache_entry'?
Yes I meant the entries, ill update the doc properly.
>
>> + * @walk_lock: lock protecting walk_head
>> + * @size: number of entries in the cache */ struct mesh_hdr_cache {
>> + bool enabled;
>> + struct rhashtable rhead;
>> + struct hlist_head walk_head;
>> + spinlock_t walk_lock; /* protects cache entries */
>> + u16 size;
>
>might be nicer to put 'bool enabled' last since otherwise you have a lot of
>padding
Sure
>
>> +struct mhdr_cache_entry {
>> + u8 addr_key[ETH_ALEN];
>
>probably better to __aligned(2) even if of course this would matter only if
>someone were to add some entries before it
Okay. I'll update.
>
>> + u8 hdr[MESH_HEADER_MAX_LEN];
>> + u16 machdr_len;
>> + u16 hdrlen;
>> + u8 pn_offs;
>> + u8 band;
>> + struct ieee80211_key __rcu *key;
>> + struct hlist_node walk_list;
>> + struct rhash_head rhash;
>> + struct mesh_path __rcu *mpath;
>> + struct mesh_path __rcu *mppath;
>> + unsigned long timestamp;
>> + struct rcu_head rcu;
>> + u32 path_change_count;
>> +};
>> +
>> /* Recent multicast cache */
>> /* RMC_BUCKETS must be a power of 2, maximum 256 */
>> #define RMC_BUCKETS 256
>> @@ -299,6 +338,13 @@ void mesh_path_tx_root_frame(struct
>> ieee80211_sub_if_data *sdata);
>>
>> bool mesh_action_is_path_sel(struct ieee80211_mgmt *mgmt);
>>
>> +struct mhdr_cache_entry *mesh_fill_cached_hdr(struct
>ieee80211_sub_if_data *sdata,
>> + struct sk_buff *skb);
>
>that's a fairly long line, you can break it better maybe:
>
>struct mhdr_cache_entry *
>mesh_fill_...
Sure.
>
>> +struct mhdr_cache_entry *mesh_fill_cached_hdr(struct
>ieee80211_sub_if_data *sdata,
>> + struct sk_buff *skb) {
>> + struct mesh_hdr_cache *cache;
>> + struct mhdr_cache_entry *entry;
>> + struct mesh_path *mpath, *mppath;
>> + struct ieee80211s_hdr *meshhdr;
>> + struct ieee80211_hdr *hdr;
>> + struct sta_info *new_nhop;
>> + struct ieee80211_key *key;
>> + struct ethhdr *eth;
>> + u8 sa[ETH_ALEN];
>> +
>> + u8 tid;
>> +
>> + cache = &sdata->u.mesh.hdr_cache;
>> +
>> + if (!cache->enabled)
>> + return NULL;
>
>If you check fast-xmit capability here, then I think you don't need the 'enabled'
>at all? The other use of it is to avoid double-release, but I don't see how that
>would happen or be legal in the first place.
>
>> + if (!cache->enabled)
>> + return;
>>
>>
>
>OK same here, but still?
>
>Dunno. Maybe the extra byte is OK and cache locality is better that way ...
>
>Though I've actually thought for a while now that we have so many capability
>checks, we might benefit from making those static keys for systems that only
>have a single wifi NIC (or multiple similar ones), which is quite many I guess.
Sure, we could check for fast-xmit cap. But, I added 'enabled' so as to avoid
accessing local->hw in all these functions and also in case if we need a
selective enable/disable of this cache in future.
>
>> + if (key) {
>> + bool gen_iv, iv_spc;
>> +
>> + gen_iv = key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_IV;
>> + iv_spc = key->conf.flags &
>> + IEEE80211_KEY_FLAG_PUT_IV_SPACE;
>> +
>> + if (!(key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE))
>> + return;
>> +
>> + if (key->flags & KEY_FLAG_TAINTED)
>> + return;
>> +
>> + switch (key->conf.cipher) {
>> + case WLAN_CIPHER_SUITE_CCMP:
>> + case WLAN_CIPHER_SUITE_CCMP_256:
>> + if (gen_iv)
>> + pn_offs = hdrlen;
>> + if (gen_iv || iv_spc)
>> + crypto_len = IEEE80211_CCMP_HDR_LEN;
>> + break;
>> + default:
>> + /* Limiting supported ciphers for testing */
>
>Nit: "limit"
>
>But GCMP should be trivial, since it's the same? Feels strange to limit to just
>CCMP.
Sure, I'll update. No limitations to stick to only CCMP.
>
>> + return;
>> + }
>> + hdr->frame_control |= cpu_to_le16(IEEE80211_FCTL_PROTECTED);
>> + }
>> +
>> + if ((hdrlen + crypto_len + mshhdr_len + sizeof(rfc1042_header)) >
>> + MESH_HEADER_MAX_LEN) {
>
>strange line-break/indentation - maybe just indent the second line by another
>tab or so to make it clear it's not part of the code.
Sure
>
>> + if (pn_offs) {
>> + /* ignore the invalid data getting copied to pn location since it will
>> + * be overwritten during tx
>> + */
>> + memcpy(mhdr->hdr, skb->data, mhdr->machdr_len);
>
>Not sure I understand that comment?
I wanted to make a note that the PN will be updated properly on every tx.
I'll remove it to keep it simple since its obvious similar to fast tx cases for
other modes.
>
>
>> +
>> + /* copy remaining hdr */
>> + memcpy(mhdr->hdr + mhdr->machdr_len,
>> + skb->data + mhdr->machdr_len - crypto_len,
>> + mhdr->hdrlen - mhdr->machdr_len);
>> + } else {
>> + memcpy(mhdr->hdr, skb->data, mhdr->hdrlen);
>> + }
>> +
>> + if (key) {
>> + hdr = (struct ieee80211_hdr *)mhdr->hdr;
>> + hdr->frame_control |= cpu_to_le16(IEEE80211_FCTL_PROTECTED);
>> + }
>
>Isn't "if (key)" equivalent to "if (pn_offs)" at this point, so you can move it into
>the same if?
The pn_offs will be set only on IEEE80211_KEY_FLAG_GENERATE_IV is set in key->conf.flags
So had separate checks.
>
>> @@ -2921,12 +2922,18 @@ ieee80211_rx_h_mesh_fwding(struct
>ieee80211_rx_data *rx)
>> mpp_path_add(sdata, proxied_addr, mpp_addr);
>> } else {
>> spin_lock_bh(&mppath->state_lock);
>> - if (!ether_addr_equal(mppath->mpp, mpp_addr))
>> + if (!ether_addr_equal(mppath->mpp, mpp_addr)) {
>> + update = true;
>> memcpy(mppath->mpp, mpp_addr, ETH_ALEN);
>> + }
>> mppath->exp_time = jiffies;
>> spin_unlock_bh(&mppath->state_lock);
>> }
>> rcu_read_unlock();
>> +
>> + /* Flush any hdr, if external device moved to a new gate */
>> + if (update)
>> + mesh_hdr_cache_flush(mppath, true);
>
>This feels pretty expensive during RX, could/should it be done asynchronously?
>Maybe it's not too bad since "is_mpp==true".
Yes right, this will be called only when there is an mpp update itself and not on
every Rx, so didn’t make is asynchronous.
>
>> + /* availability of hdr entry ensures the mpath and sta are valid
>> + * hence validation can be avoided.
>
>What about key? :)
Hmm.. I was relying on the availability of cache entry to ensure sta and key are valid.
But I see your point and realize the entry gets removed later in __sta_info_destroy_part2()
whereas the key can get cleared before the entry is flushed.
May be I'll see if I could flush the cache entry in __sta_info_destroy_part1() so that
the entry itself is not visible. Hope that solves if such race occurs.
Thanks for the review again.
I'll take care of all the above ones in next update.
Regards,
Sriram.R
>
>johannes
