Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1280022 > unrolled thread

Re: [PATCH 06/13] net: mvneta: enable mixed egress processing using HR timer

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 06/13] net: mvneta: enable mixed egress processing using  HR timer Marcin Wojtas <mw@semihalf.com> - 2015-11-30 17:00 +0100
    Re: [PATCH 06/13] net: mvneta: enable mixed egress processing using  HR timer Marcin Wojtas <mw@semihalf.com> - 2015-12-02 11:10 +0100

#1280022 — Re: [PATCH 06/13] net: mvneta: enable mixed egress processing using HR timer

Hi Simon,

2015-11-26 17:45 GMT+01:00 Simon Guinot <simon.guinot@sequanux.org>:
> Hi Marcin,
>
> On Sun, Nov 22, 2015 at 08:53:52AM +0100, Marcin Wojtas wrote:
>> Mixed approach allows using higher interrupt threshold (increased back to
>> 15 packets), useful in high throughput. In case of small amount of data
>> or very short TX queues HR timer ensures releasing buffers with small
>> latency.
>>
>> Along with existing tx_done processing by coalescing interrupts this
>> commit enables triggering HR timer each time the packets are sent.
>> Time threshold can also be configured, using ethtool.
>>
>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>> Signed-off-by: Simon Guinot <simon.guinot@sequanux.org>
>> ---
>>  drivers/net/ethernet/marvell/mvneta.c | 89 +++++++++++++++++++++++++++++++++--
>>  1 file changed, 85 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
>> index 9c9e858..f5acaf6 100644
>> --- a/drivers/net/ethernet/marvell/mvneta.c
>> +++ b/drivers/net/ethernet/marvell/mvneta.c
>> @@ -21,6 +21,8 @@
>>  #include <linux/module.h>
>>  #include <linux/interrupt.h>
>>  #include <linux/if_vlan.h>
>> +#include <linux/hrtimer.h>
>> +#include <linux/ktime.h>
>
> ktime.h is already included by hrtimer.h.
>
>>  #include <net/ip.h>
>>  #include <net/ipv6.h>
>>  #include <linux/io.h>
>> @@ -226,7 +228,8 @@
>>  /* Various constants */
>>
>>  /* Coalescing */
>> -#define MVNETA_TXDONE_COAL_PKTS              1
>> +#define MVNETA_TXDONE_COAL_PKTS              15
>> +#define MVNETA_TXDONE_COAL_USEC              100
>
> Maybe we should keep the default configuration and let the user choose
> to enable (or not) this feature ?

I think that this feature should be enabled by default, same as in RX
(which is enabled by HW in ingress). It satisfies all kinds of traffic
or queues sizes. I'd prefer a situation that if someone really wants
to disable it (even if I don't know the possible justification), then
let him use ethtool for this purpose.

>
>>  #define MVNETA_RX_COAL_PKTS          32
>>  #define MVNETA_RX_COAL_USEC          100
>>
>> @@ -356,6 +359,11 @@ struct mvneta_port {
>>       struct net_device *dev;
>>       struct notifier_block cpu_notifier;
>>
>> +     /* Egress finalization */
>> +     struct tasklet_struct tx_done_tasklet;
>> +     struct hrtimer tx_done_timer;
>> +     bool timer_scheduled;
>
> I think we could use hrtimer_is_queued() instead of introducing a new
> variable.
>

Good point, i'll try that.

Best regards,
Marcin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1281582

Hi Simon,

I checked using hrtimer_is_queued instead of a custom flag and it
resulted in ~20kpps drop in my setup. timer_scheduled flag is cleared
in the tasklet, so no timer can be scheduled until the tasklet is
executed. hr_timer flags do not cover this situation, so much more
timers are enqueued. I added a counter and with maximal throughput
during 30s test and 9 times less timers were enqueued with
timer_scheduled flag (~31k vs ~281k), so it's much more efficient and
I'll leave it as is.

Best regards,
Marcin

2015-11-30 16:57 GMT+01:00 Marcin Wojtas <mw@semihalf.com>:
> Hi Simon,
>
> 2015-11-26 17:45 GMT+01:00 Simon Guinot <simon.guinot@sequanux.org>:
>> Hi Marcin,
>>
>> On Sun, Nov 22, 2015 at 08:53:52AM +0100, Marcin Wojtas wrote:
>>> Mixed approach allows using higher interrupt threshold (increased back to
>>> 15 packets), useful in high throughput. In case of small amount of data
>>> or very short TX queues HR timer ensures releasing buffers with small
>>> latency.
>>>
>>> Along with existing tx_done processing by coalescing interrupts this
>>> commit enables triggering HR timer each time the packets are sent.
>>> Time threshold can also be configured, using ethtool.
>>>
>>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>>> Signed-off-by: Simon Guinot <simon.guinot@sequanux.org>
>>> ---
>>>  drivers/net/ethernet/marvell/mvneta.c | 89 +++++++++++++++++++++++++++++++++--
>>>  1 file changed, 85 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
>>> index 9c9e858..f5acaf6 100644
>>> --- a/drivers/net/ethernet/marvell/mvneta.c
>>> +++ b/drivers/net/ethernet/marvell/mvneta.c
>>> @@ -21,6 +21,8 @@
>>>  #include <linux/module.h>
>>>  #include <linux/interrupt.h>
>>>  #include <linux/if_vlan.h>
>>> +#include <linux/hrtimer.h>
>>> +#include <linux/ktime.h>
>>
>> ktime.h is already included by hrtimer.h.
>>
>>>  #include <net/ip.h>
>>>  #include <net/ipv6.h>
>>>  #include <linux/io.h>
>>> @@ -226,7 +228,8 @@
>>>  /* Various constants */
>>>
>>>  /* Coalescing */
>>> -#define MVNETA_TXDONE_COAL_PKTS              1
>>> +#define MVNETA_TXDONE_COAL_PKTS              15
>>> +#define MVNETA_TXDONE_COAL_USEC              100
>>
>> Maybe we should keep the default configuration and let the user choose
>> to enable (or not) this feature ?
>
> I think that this feature should be enabled by default, same as in RX
> (which is enabled by HW in ingress). It satisfies all kinds of traffic
> or queues sizes. I'd prefer a situation that if someone really wants
> to disable it (even if I don't know the possible justification), then
> let him use ethtool for this purpose.
>
>>
>>>  #define MVNETA_RX_COAL_PKTS          32
>>>  #define MVNETA_RX_COAL_USEC          100
>>>
>>> @@ -356,6 +359,11 @@ struct mvneta_port {
>>>       struct net_device *dev;
>>>       struct notifier_block cpu_notifier;
>>>
>>> +     /* Egress finalization */
>>> +     struct tasklet_struct tx_done_tasklet;
>>> +     struct hrtimer tx_done_timer;
>>> +     bool timer_scheduled;
>>
>> I think we could use hrtimer_is_queued() instead of introducing a new
>> variable.
>>
>
> Good point, i'll try that.
>
> Best regards,
> Marcin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web