All of lore.kernel.org
 help / color / mirror / Atom feed
From: Johannes Berg <johannes@sipsolutions.net>
To: "Toke Høiland-Jørgensen" <toke@redhat.com>, "Kan Yan" <kyan@google.com>
Cc: linux-wireless@vger.kernel.org,
	make-wifi-fast@lists.bufferbloat.net, ath10k@lists.infradead.org,
	John Crispin <john@phrozen.org>,
	Lorenzo Bianconi <lorenzo@kernel.org>,
	Felix Fietkau <nbd@nbd.name>,
	Rajkumar Manoharan <rmanohar@codeaurora.org>,
	Kevin Hayes <kevinhayes@google.com>
Subject: Re: [PATCH v2 1/4] mac80211: Rearrange ieee80211_tx_info to make room for tx_time_est
Date: Fri, 18 Oct 2019 14:35:40 +0200	[thread overview]
Message-ID: <4bc17c32337888a57a78a0e2c874abd462faef21.camel@sipsolutions.net> (raw)
In-Reply-To: <87sgnqe4wg.fsf@toke.dk>

On Fri, 2019-10-18 at 12:15 +0200, Toke Høiland-Jørgensen wrote:
> Kan Yan <kyan@google.com> writes:
> 
> > The "tx_time_est" field, shared by control and status, is not able to
> > survive until the skb returns to the mac80211 layer in some
> > architectures. The same space is defined as driver_data and some
> > wireless drivers use it for other purposes, as the cb in the sk_buff
> > is free to be used by any layer.
> > 
> > In the case of ath10k, the tx_time_est get clobbered by
> > struct ath10k_skb_cb {
> >         dma_addr_t paddr;
> >         u8 flags;
> >         u8 eid;
> >         u16 msdu_id;
> >         u16 airtime_est;
> >         struct ieee80211_vif *vif;
> >         struct ieee80211_txq *txq;
> > } __packed;
> 
> Ah, bugger, of course the driver that actually needs this is using the
> full driver_data space :P

Looks like you could shrink *this* fairly easily though.

E.g. most likely vif == txq->vif unless txq==NULL, so it's down to 22
bytes plus a bit/flag for knowing whether the pointer is a vif directly
(if no TXQ) or a TXQ.

> > Do you think shrink driver_data by 2 bytes and use that space for
> > tx_time_est to make it persistent across mac80211 and wireless driver
> > layer an acceptable solution?
> 
> Hmm, the driver_data field is defined as an array of pointers, so we can
> only shrink it in increments of sizeof(void *). I think it may be
> feasible to shrink it (as in, I don't think any drivers are actually
> using the full 40 bytes),

It doesn't have to be defined like that, just was most convenient as
driers were using pointers there.

> but doing this in a way that will gain us a
> 2-byte space that is also usable in the case driver_data is *not* used
> (i.e., it needs be able to align with a field in .control and .status as
> well) would require some serious surgery of the whole ieee80211_tx_info...

Yeah, good point, this doesn't help at all ...

johannes


WARNING: multiple messages have this Message-ID (diff)
From: Johannes Berg <johannes@sipsolutions.net>
To: "Toke Høiland-Jørgensen" <toke@redhat.com>, "Kan Yan" <kyan@google.com>
Cc: Rajkumar Manoharan <rmanohar@codeaurora.org>,
	Kevin Hayes <kevinhayes@google.com>,
	make-wifi-fast@lists.bufferbloat.net,
	linux-wireless@vger.kernel.org, ath10k@lists.infradead.org,
	John Crispin <john@phrozen.org>,
	Lorenzo Bianconi <lorenzo@kernel.org>,
	Felix Fietkau <nbd@nbd.name>
Subject: Re: [PATCH v2 1/4] mac80211: Rearrange ieee80211_tx_info to make room for tx_time_est
Date: Fri, 18 Oct 2019 14:35:40 +0200	[thread overview]
Message-ID: <4bc17c32337888a57a78a0e2c874abd462faef21.camel@sipsolutions.net> (raw)
In-Reply-To: <87sgnqe4wg.fsf@toke.dk>

On Fri, 2019-10-18 at 12:15 +0200, Toke Høiland-Jørgensen wrote:
> Kan Yan <kyan@google.com> writes:
> 
> > The "tx_time_est" field, shared by control and status, is not able to
> > survive until the skb returns to the mac80211 layer in some
> > architectures. The same space is defined as driver_data and some
> > wireless drivers use it for other purposes, as the cb in the sk_buff
> > is free to be used by any layer.
> > 
> > In the case of ath10k, the tx_time_est get clobbered by
> > struct ath10k_skb_cb {
> >         dma_addr_t paddr;
> >         u8 flags;
> >         u8 eid;
> >         u16 msdu_id;
> >         u16 airtime_est;
> >         struct ieee80211_vif *vif;
> >         struct ieee80211_txq *txq;
> > } __packed;
> 
> Ah, bugger, of course the driver that actually needs this is using the
> full driver_data space :P

Looks like you could shrink *this* fairly easily though.

E.g. most likely vif == txq->vif unless txq==NULL, so it's down to 22
bytes plus a bit/flag for knowing whether the pointer is a vif directly
(if no TXQ) or a TXQ.

> > Do you think shrink driver_data by 2 bytes and use that space for
> > tx_time_est to make it persistent across mac80211 and wireless driver
> > layer an acceptable solution?
> 
> Hmm, the driver_data field is defined as an array of pointers, so we can
> only shrink it in increments of sizeof(void *). I think it may be
> feasible to shrink it (as in, I don't think any drivers are actually
> using the full 40 bytes),

It doesn't have to be defined like that, just was most convenient as
driers were using pointers there.

> but doing this in a way that will gain us a
> 2-byte space that is also usable in the case driver_data is *not* used
> (i.e., it needs be able to align with a field in .control and .status as
> well) would require some serious surgery of the whole ieee80211_tx_info...

Yeah, good point, this doesn't help at all ...

johannes


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

  parent reply	other threads:[~2019-10-18 12:35 UTC|newest]

Thread overview: 48+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-15 17:18 [PATCH v2 0/4] Add Airtime Queue Limits (AQL) to mac80211 Toke Høiland-Jørgensen
2019-10-15 17:18 ` Toke Høiland-Jørgensen
2019-10-15 17:18 ` [PATCH v2 1/4] mac80211: Rearrange ieee80211_tx_info to make room for tx_time_est Toke Høiland-Jørgensen
2019-10-15 17:18   ` Toke Høiland-Jørgensen
2019-10-18  0:50   ` Kan Yan
2019-10-18  0:50     ` Kan Yan
2019-10-18 10:15     ` Toke Høiland-Jørgensen
2019-10-18 10:15       ` Toke Høiland-Jørgensen
2019-10-18 12:21       ` Johannes Berg
2019-10-18 12:21         ` Johannes Berg
2019-10-18 13:31         ` Toke Høiland-Jørgensen
2019-10-18 13:31           ` Toke Høiland-Jørgensen
2019-10-18 13:48           ` Johannes Berg
2019-10-18 13:48             ` Johannes Berg
2019-10-18 14:01             ` Toke Høiland-Jørgensen
2019-10-18 14:01               ` Toke Høiland-Jørgensen
2019-10-18 14:07               ` Johannes Berg
2019-10-18 14:07                 ` Johannes Berg
2019-10-18 14:22                 ` Toke Høiland-Jørgensen
2019-10-18 14:22                   ` Toke Høiland-Jørgensen
2019-10-18 14:14               ` Johannes Berg
2019-10-18 14:14                 ` Johannes Berg
2019-10-18 14:30                 ` Toke Høiland-Jørgensen
2019-10-18 14:30                   ` Toke Høiland-Jørgensen
2019-10-18 12:35       ` Johannes Berg [this message]
2019-10-18 12:35         ` Johannes Berg
2019-10-18 13:01         ` Ben Greear
2019-10-18 13:01           ` Ben Greear
2019-10-15 17:18 ` [PATCH v2 2/4] mac80211: Import airtime calculation code from mt76 Toke Høiland-Jørgensen
2019-10-15 17:18   ` Toke Høiland-Jørgensen
2019-10-15 17:19 ` [PATCH v2 3/4] mac80211: Implement Airtime-based Queue Limit (AQL) Toke Høiland-Jørgensen
2019-10-15 17:19   ` Toke Høiland-Jørgensen
2019-10-15 17:19 ` [PATCH v2 4/4] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue Toke Høiland-Jørgensen
2019-10-15 17:19   ` Toke Høiland-Jørgensen
2019-10-17  0:33   ` Kan Yan
2019-10-17  0:33     ` Kan Yan
2019-10-17  9:44     ` Toke Høiland-Jørgensen
2019-10-17  9:44       ` Toke Høiland-Jørgensen
2019-10-17  9:57       ` [Make-wifi-fast] " Sebastian Moeller
2019-10-17  9:57         ` Sebastian Moeller
2019-10-17 10:24         ` Toke Høiland-Jørgensen
2019-10-17 10:24           ` Toke Høiland-Jørgensen
2019-10-17 10:25           ` Sebastian Moeller
2019-10-17 10:25             ` Sebastian Moeller
2019-10-18  1:11             ` Kan Yan
2019-10-18  1:11               ` Kan Yan
2019-10-18 14:15               ` Johannes Berg
2019-10-18 14:15                 ` Johannes Berg

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4bc17c32337888a57a78a0e2c874abd462faef21.camel@sipsolutions.net \
    --to=johannes@sipsolutions.net \
    --cc=ath10k@lists.infradead.org \
    --cc=john@phrozen.org \
    --cc=kevinhayes@google.com \
    --cc=kyan@google.com \
    --cc=linux-wireless@vger.kernel.org \
    --cc=lorenzo@kernel.org \
    --cc=make-wifi-fast@lists.bufferbloat.net \
    --cc=nbd@nbd.name \
    --cc=rmanohar@codeaurora.org \
    --cc=toke@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.