Re: [ath9k-devel] [PATCH v4] ath9k: Switch to using mac80211 intermediate software queues.

[Toke Høiland-Jørgensen]

Kalle Valo  writes:

> Toke Høiland-Jørgensen  writes:
>
>> Kalle Valo  writes:
>>
>>> Toke Høiland-Jørgensen  writes:
>>>
 Kalle Valo  writes:

> Toke Høiland-Jørgensen  writes:
>
> I understand your point, but I don't want to rush this to 4.9 and then
> start getting lots of bug reports and eventually forced to revert it. If
> we just found a new serious regression the chances are that there are
> more lurking somewhere and this patch is just not ready yet.

 So, the changes to mac80211 that fixes the known regressions of this
 patch have gone in.
>>>
>>> I guess you mean this commit:
>>>
>>> bb42f2d13ffc mac80211: Move reorder-sensitive TX handlers to after TXQ 
>>> dequeue
>>>
>>> (Just making sure that I have the same commit in my tree when I apply
>>> this)
>>
>> Yup, that's the one :)
>>
 Any chance of seeing this merged during the current merge window? :)
>>>
>>> I sent last new feature ("-next") patches for 4.9 last week, sorry. So
>>> this has to wait for 4.10.
>>
>> Ah, right, I think I got my merge windows confused. You already said you
>> wouldn't take it for 4.9. So I guess what I'm asking is for you to put
>> it into the appropriate -next tree so it can get some wider exposure
>> ahead of the *next* merge window...
>
> Yeah, we have plenty of time for 4.10 :) So my plan is to apply this
> after I open wireless-drivers-next in 2-3 weeks or so. That would mean
> that the patch would hit Linus' tree when 4.10-rc1 is released
> (estimated to happen on 2017-01-01). The timing is actually perfect as
> now we get maximal testing time on -next.

So the -next trees are those that are open outside the merge window.
Right, got it; thanks :)

>>> And I assume I need to take v5:
>>>
>>> https://patchwork.kernel.org/patch/9311037/
>>
>> Yes. Haven't noticed anything that changed since that might conflict
>> with it, but let me know if I missed something and you want a refreshed
>> version.
>
> Thanks, I'll let you know if there are any problems.

Cool.

-Toke
___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel

Re: [ath9k-devel] [PATCH v4] ath9k: Switch to using mac80211 intermediate software queues.

[Toke Høiland-Jørgensen]

Kalle Valo  writes:

> Toke Høiland-Jørgensen  writes:
>
>> Kalle Valo  writes:
>>
>>> Toke Høiland-Jørgensen  writes:
>>>
>>> I understand your point, but I don't want to rush this to 4.9 and then
>>> start getting lots of bug reports and eventually forced to revert it. If
>>> we just found a new serious regression the chances are that there are
>>> more lurking somewhere and this patch is just not ready yet.
>>
>> So, the changes to mac80211 that fixes the known regressions of this
>> patch have gone in.
>
> I guess you mean this commit:
>
> bb42f2d13ffc mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue
>
> (Just making sure that I have the same commit in my tree when I apply
> this)

Yup, that's the one :)

>> Any chance of seeing this merged during the current merge window? :)
>
> I sent last new feature ("-next") patches for 4.9 last week, sorry. So
> this has to wait for 4.10.

Ah, right, I think I got my merge windows confused. You already said you
wouldn't take it for 4.9. So I guess what I'm asking is for you to put
it into the appropriate -next tree so it can get some wider exposure
ahead of the *next* merge window...

> And I assume I need to take v5:
>
> https://patchwork.kernel.org/patch/9311037/

Yes. Haven't noticed anything that changed since that might conflict
with it, but let me know if I missed something and you want a refreshed
version.

-Toke
___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel

Re: [ath9k-devel] [PATCH v4] ath9k: Switch to using mac80211 intermediate software queues.

[Toke Høiland-Jørgensen]

Kalle Valo  writes:

> Toke Høiland-Jørgensen  writes:
>
> This is great work but due to the regressions I'm not sure if this
> will be ready for 4.9. To get more testing time I wonder if we should
> wait for 4.10? IMHO applying this in the end of the cycle is too risky
> and we should try to maximise the time linux-next by applying this
> just after -rc1 is released.
>
> Thoughts?

 Well, now that we understand what is causing the throughput regressions,
 fixing them should be fairly straight forward (yeah, famous last words,
 but still...). I already have a patch for the fast path and will go poke
 at the slow path next. It'll probably require another workaround or two,
 so I guess it won't be the architecturally clean ideal solution; but it
 would make it possible to have something that works for 4.9 and then
 iterate for a cleaner design for 4.10.
>>>
>>> But if we try to rush this to 4.9 it won't be in linux-next for long. We
>>> are now in -rc3 and let's say that the patches are ready to apply in two
>>> weeks. That would leave us only two weeks of -next time before the merge
>>> window, which I think is not enough for a controversial patch like this
>>> one. There might be other bugs lurking which haven't been found yet.
>>
>> What, other hidden bugs? Unpossible! :)
>
> Yeah, right ;)
>
>> Would it be possible to merge the partial solution (which is ready now,
>> basically) and fix the slow path in a separate patch later?
>
> What do you mean with partial solution? You mean ath9k users would
> suffer from regressions until they are fixed? We can't do that.
>
>> (Just spit-balling here; I'm still fairly new to this process. But I am
>> concerned that we'll hit a catch-22 where we can't get wider testing
>> before it's "ready" and we can't prove that it's "ready" until we've had
>> wider testing...)
>
> I understand your point, but I don't want to rush this to 4.9 and then
> start getting lots of bug reports and eventually forced to revert it. If
> we just found a new serious regression the chances are that there are
> more lurking somewhere and this patch is just not ready yet.

So, the changes to mac80211 that fixes the known regressions of this
patch have gone in. Any chance of seeing this merged during the current
merge window? :)

-Toke
___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel

Re: [ath9k-devel] [PATCH v4] ath9k: Switch to using mac80211 intermediate software queues.

[Toke Høiland-Jørgensen]

Kalle Valo  writes:

> Toke Høiland-Jørgensen  writes:
>
>> Kalle Valo  writes:
>>
>>> Toke Høiland-Jørgensen  writes:
>>>
 This switches ath9k over to using the mac80211 intermediate software
 queueing mechanism for data packets. It removes the queueing inside the
 driver, except for the retry queue, and instead pulls from mac80211 when
 a packet is needed. The retry queue is used to store a packet that was
 pulled but can't be sent immediately.

 The old code path in ath_tx_start that would queue packets has been
 removed completely, as has the qlen limit tunables (since there's no
 longer a queue in the driver to limit).

 Based on Tim's original patch set, but reworked quite thoroughly.

 Cc: Tim Shepard 
 Cc: Felix Fietkau 
 Signed-off-by: Toke Høiland-Jørgensen 
 ---
 Changes since v3 (most due to Felix; thanks!):
   - Correctly notify mac80211 when there are packets in the retry queue
 on powersave start/stop.
   - Get rid of ath_tx_aggr_resume().
   - Some readability changes and additional WARN_ON/BUG_ON in
 appropriate places.
>>>
>>> This is great work but due to the regressions I'm not sure if this
>>> will be ready for 4.9. To get more testing time I wonder if we should
>>> wait for 4.10? IMHO applying this in the end of the cycle is too risky
>>> and we should try to maximise the time linux-next by applying this
>>> just after -rc1 is released.
>>>
>>> Thoughts?
>>
>> Well, now that we understand what is causing the throughput regressions,
>> fixing them should be fairly straight forward (yeah, famous last words,
>> but still...). I already have a patch for the fast path and will go poke
>> at the slow path next. It'll probably require another workaround or two,
>> so I guess it won't be the architecturally clean ideal solution; but it
>> would make it possible to have something that works for 4.9 and then
>> iterate for a cleaner design for 4.10.
>
> But if we try to rush this to 4.9 it won't be in linux-next for long. We
> are now in -rc3 and let's say that the patches are ready to apply in two
> weeks. That would leave us only two weeks of -next time before the merge
> window, which I think is not enough for a controversial patch like this
> one. There might be other bugs lurking which haven't been found yet.

What, other hidden bugs? Unpossible! :)

Would it be possible to merge the partial solution (which is ready now,
basically) and fix the slow path in a separate patch later?

(Just spit-balling here; I'm still fairly new to this process. But I am
concerned that we'll hit a catch-22 where we can't get wider testing
before it's "ready" and we can't prove that it's "ready" until we've had
wider testing...)

-Toke
___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel

Re: [ath9k-devel] [PATCH v4] ath9k: Switch to using mac80211 intermediate software queues.

[Toke Høiland-Jørgensen]

Kalle Valo  writes:

> Toke Høiland-Jørgensen  writes:
>
>> This switches ath9k over to using the mac80211 intermediate software
>> queueing mechanism for data packets. It removes the queueing inside the
>> driver, except for the retry queue, and instead pulls from mac80211 when
>> a packet is needed. The retry queue is used to store a packet that was
>> pulled but can't be sent immediately.
>>
>> The old code path in ath_tx_start that would queue packets has been
>> removed completely, as has the qlen limit tunables (since there's no
>> longer a queue in the driver to limit).
>>
>> Based on Tim's original patch set, but reworked quite thoroughly.
>>
>> Cc: Tim Shepard 
>> Cc: Felix Fietkau 
>> Signed-off-by: Toke Høiland-Jørgensen 
>> ---
>> Changes since v3 (most due to Felix; thanks!):
>>   - Correctly notify mac80211 when there are packets in the retry queue
>> on powersave start/stop.
>>   - Get rid of ath_tx_aggr_resume().
>>   - Some readability changes and additional WARN_ON/BUG_ON in
>> appropriate places.
>
> This is great work but due to the regressions I'm not sure if this
> will be ready for 4.9. To get more testing time I wonder if we should
> wait for 4.10? IMHO applying this in the end of the cycle is too risky
> and we should try to maximise the time linux-next by applying this
> just after -rc1 is released.
>
> Thoughts?

Well, now that we understand what is causing the throughput regressions,
fixing them should be fairly straight forward (yeah, famous last words,
but still...). I already have a patch for the fast path and will go poke
at the slow path next. It'll probably require another workaround or two,
so I guess it won't be the architecturally clean ideal solution; but it
would make it possible to have something that works for 4.9 and then
iterate for a cleaner design for 4.10.

-Toke
___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel

Re: [ath9k-devel] [PATCH v4] ath9k: Switch to using mac80211 intermediate software queues.

[Arend van Spriel]

On 23-08-16 08:59, Kalle Valo wrote:
> Toke Høiland-Jørgensen  writes:
> 
> This is great work but due to the regressions I'm not sure if this
> will be ready for 4.9. To get more testing time I wonder if we should
> wait for 4.10? IMHO applying this in the end of the cycle is too risky
> and we should try to maximise the time linux-next by applying this
> just after -rc1 is released.
>
> Thoughts?

 Well, now that we understand what is causing the throughput regressions,
 fixing them should be fairly straight forward (yeah, famous last words,
 but still...). I already have a patch for the fast path and will go poke
 at the slow path next. It'll probably require another workaround or two,
 so I guess it won't be the architecturally clean ideal solution; but it
 would make it possible to have something that works for 4.9 and then
 iterate for a cleaner design for 4.10.
>>>
>>> But if we try to rush this to 4.9 it won't be in linux-next for long. We
>>> are now in -rc3 and let's say that the patches are ready to apply in two
>>> weeks. That would leave us only two weeks of -next time before the merge
>>> window, which I think is not enough for a controversial patch like this
>>> one. There might be other bugs lurking which haven't been found yet.
>>
>> What, other hidden bugs? Unpossible! :)
> 
> Yeah, right ;)
> 
>> Would it be possible to merge the partial solution (which is ready now,
>> basically) and fix the slow path in a separate patch later?
> 
> What do you mean with partial solution? You mean ath9k users would
> suffer from regressions until they are fixed? We can't do that.
> 
>> (Just spit-balling here; I'm still fairly new to this process. But I am
>> concerned that we'll hit a catch-22 where we can't get wider testing
>> before it's "ready" and we can't prove that it's "ready" until we've had
>> wider testing...)

So could the wider testing be accomplished by working on a branch in the
wireless-testing repo and make its availability known on wireless-list,
ath?k-list, LWN or whatever.

Regards,
Arend

> I understand your point, but I don't want to rush this to 4.9 and then
> start getting lots of bug reports and eventually forced to revert it. If
> we just found a new serious regression the chances are that there are
> more lurking somewhere and this patch is just not ready yet.

___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel

Re: [ath9k-devel] [PATCH v4] ath9k: Switch to using mac80211 intermediate software queues.

[Kalle Valo]

Toke Høiland-Jørgensen  writes:

> Kalle Valo  writes:
>
>> Toke Høiland-Jørgensen  writes:
>>
>>> Kalle Valo  writes:
>>>
 Toke Høiland-Jørgensen  writes:

 I understand your point, but I don't want to rush this to 4.9 and then
 start getting lots of bug reports and eventually forced to revert it. If
 we just found a new serious regression the chances are that there are
 more lurking somewhere and this patch is just not ready yet.
>>>
>>> So, the changes to mac80211 that fixes the known regressions of this
>>> patch have gone in.
>>
>> I guess you mean this commit:
>>
>> bb42f2d13ffc mac80211: Move reorder-sensitive TX handlers to after TXQ 
>> dequeue
>>
>> (Just making sure that I have the same commit in my tree when I apply
>> this)
>
> Yup, that's the one :)
>
>>> Any chance of seeing this merged during the current merge window? :)
>>
>> I sent last new feature ("-next") patches for 4.9 last week, sorry. So
>> this has to wait for 4.10.
>
> Ah, right, I think I got my merge windows confused. You already said you
> wouldn't take it for 4.9. So I guess what I'm asking is for you to put
> it into the appropriate -next tree so it can get some wider exposure
> ahead of the *next* merge window...

Yeah, we have plenty of time for 4.10 :) So my plan is to apply this
after I open wireless-drivers-next in 2-3 weeks or so. That would mean
that the patch would hit Linus' tree when 4.10-rc1 is released
(estimated to happen on 2017-01-01). The timing is actually perfect as
now we get maximal testing time on -next.

>> And I assume I need to take v5:
>>
>> https://patchwork.kernel.org/patch/9311037/
>
> Yes. Haven't noticed anything that changed since that might conflict
> with it, but let me know if I missed something and you want a refreshed
> version.

Thanks, I'll let you know if there are any problems.

-- 
Kalle Valo
___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel

Re: [ath9k-devel] [PATCH v4] ath9k: Switch to using mac80211 intermediate software queues.

[Kalle Valo]

Toke Høiland-Jørgensen  writes:

> Kalle Valo  writes:
>
>> Toke Høiland-Jørgensen  writes:
>>
>> I understand your point, but I don't want to rush this to 4.9 and then
>> start getting lots of bug reports and eventually forced to revert it. If
>> we just found a new serious regression the chances are that there are
>> more lurking somewhere and this patch is just not ready yet.
>
> So, the changes to mac80211 that fixes the known regressions of this
> patch have gone in.

I guess you mean this commit:

bb42f2d13ffc mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue

(Just making sure that I have the same commit in my tree when I apply this)

> Any chance of seeing this merged during the current merge window? :)

I sent last new feature ("-next") patches for 4.9 last week, sorry. So
this has to wait for 4.10.

And I assume I need to take v5:

https://patchwork.kernel.org/patch/9311037/

-- 
Kalle Valo
___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel

Re: [ath9k-devel] [PATCH v4] ath9k: Switch to using mac80211 intermediate software queues.

[Kalle Valo]

Toke Høiland-Jørgensen  writes:

 This is great work but due to the regressions I'm not sure if this
 will be ready for 4.9. To get more testing time I wonder if we should
 wait for 4.10? IMHO applying this in the end of the cycle is too risky
 and we should try to maximise the time linux-next by applying this
 just after -rc1 is released.

 Thoughts?
>>>
>>> Well, now that we understand what is causing the throughput regressions,
>>> fixing them should be fairly straight forward (yeah, famous last words,
>>> but still...). I already have a patch for the fast path and will go poke
>>> at the slow path next. It'll probably require another workaround or two,
>>> so I guess it won't be the architecturally clean ideal solution; but it
>>> would make it possible to have something that works for 4.9 and then
>>> iterate for a cleaner design for 4.10.
>>
>> But if we try to rush this to 4.9 it won't be in linux-next for long. We
>> are now in -rc3 and let's say that the patches are ready to apply in two
>> weeks. That would leave us only two weeks of -next time before the merge
>> window, which I think is not enough for a controversial patch like this
>> one. There might be other bugs lurking which haven't been found yet.
>
> What, other hidden bugs? Unpossible! :)

Yeah, right ;)

> Would it be possible to merge the partial solution (which is ready now,
> basically) and fix the slow path in a separate patch later?

What do you mean with partial solution? You mean ath9k users would
suffer from regressions until they are fixed? We can't do that.

> (Just spit-balling here; I'm still fairly new to this process. But I am
> concerned that we'll hit a catch-22 where we can't get wider testing
> before it's "ready" and we can't prove that it's "ready" until we've had
> wider testing...)

I understand your point, but I don't want to rush this to 4.9 and then
start getting lots of bug reports and eventually forced to revert it. If
we just found a new serious regression the chances are that there are
more lurking somewhere and this patch is just not ready yet.

-- 
Kalle Valo
___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel

Re: [ath9k-devel] [PATCH v4] ath9k: Switch to using mac80211 intermediate software queues.

[Kalle Valo]

Toke Høiland-Jørgensen  writes:

> Kalle Valo  writes:
>
>> Toke Høiland-Jørgensen  writes:
>>
>>> This switches ath9k over to using the mac80211 intermediate software
>>> queueing mechanism for data packets. It removes the queueing inside the
>>> driver, except for the retry queue, and instead pulls from mac80211 when
>>> a packet is needed. The retry queue is used to store a packet that was
>>> pulled but can't be sent immediately.
>>>
>>> The old code path in ath_tx_start that would queue packets has been
>>> removed completely, as has the qlen limit tunables (since there's no
>>> longer a queue in the driver to limit).
>>>
>>> Based on Tim's original patch set, but reworked quite thoroughly.
>>>
>>> Cc: Tim Shepard 
>>> Cc: Felix Fietkau 
>>> Signed-off-by: Toke Høiland-Jørgensen 
>>> ---
>>> Changes since v3 (most due to Felix; thanks!):
>>>   - Correctly notify mac80211 when there are packets in the retry queue
>>> on powersave start/stop.
>>>   - Get rid of ath_tx_aggr_resume().
>>>   - Some readability changes and additional WARN_ON/BUG_ON in
>>> appropriate places.
>>
>> This is great work but due to the regressions I'm not sure if this
>> will be ready for 4.9. To get more testing time I wonder if we should
>> wait for 4.10? IMHO applying this in the end of the cycle is too risky
>> and we should try to maximise the time linux-next by applying this
>> just after -rc1 is released.
>>
>> Thoughts?
>
> Well, now that we understand what is causing the throughput regressions,
> fixing them should be fairly straight forward (yeah, famous last words,
> but still...). I already have a patch for the fast path and will go poke
> at the slow path next. It'll probably require another workaround or two,
> so I guess it won't be the architecturally clean ideal solution; but it
> would make it possible to have something that works for 4.9 and then
> iterate for a cleaner design for 4.10.

But if we try to rush this to 4.9 it won't be in linux-next for long. We
are now in -rc3 and let's say that the patches are ready to apply in two
weeks. That would leave us only two weeks of -next time before the merge
window, which I think is not enough for a controversial patch like this
one. There might be other bugs lurking which haven't been found yet.

-- 
Kalle Valo
___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel

Re: [ath9k-devel] [PATCH v4] ath9k: Switch to using mac80211 intermediate software queues.

[Kalle Valo]

Toke Høiland-Jørgensen  writes:

> This switches ath9k over to using the mac80211 intermediate software
> queueing mechanism for data packets. It removes the queueing inside the
> driver, except for the retry queue, and instead pulls from mac80211 when
> a packet is needed. The retry queue is used to store a packet that was
> pulled but can't be sent immediately.
>
> The old code path in ath_tx_start that would queue packets has been
> removed completely, as has the qlen limit tunables (since there's no
> longer a queue in the driver to limit).
>
> Based on Tim's original patch set, but reworked quite thoroughly.
>
> Cc: Tim Shepard 
> Cc: Felix Fietkau 
> Signed-off-by: Toke Høiland-Jørgensen 
> ---
> Changes since v3 (most due to Felix; thanks!):
>   - Correctly notify mac80211 when there are packets in the retry queue
> on powersave start/stop.
>   - Get rid of ath_tx_aggr_resume().
>   - Some readability changes and additional WARN_ON/BUG_ON in
> appropriate places.

This is great work but due to the regressions I'm not sure if this will
be ready for 4.9. To get more testing time I wonder if we should wait
for 4.10? IMHO applying this in the end of the cycle is too risky and we
should try to maximise the time linux-next by applying this just after
-rc1 is released.

Thoughts?

-- 
Kalle Valo
___
ath9k-devel mailing list
ath9k-devel@lists.ath9k.org
https://lists.ath9k.org/mailman/listinfo/ath9k-devel