Hi,
Couple of small things, sorry I didn't see that before.
> +/**
> + * struct mesh_hdr_cache
need a short description, e.g.
struct mesh_hdr_cache - mesh header cache entry
> + *
> + * @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?
> + * @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'?
> + * @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
> +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
> + 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_...
> +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.
> + 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.
> + 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.
> + 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?
> +
> + /* 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?
> @@ -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".
> + /* availability of hdr entry ensures the mpath and sta are valid
> + * hence validation can be avoided.
What about key? :)
johannes
