| Message ID | 20191010144226.4115-3-alobakin@dlink.ru |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: core: use listified Rx for GRO_NORMAL in napi_gro_receive() | expand |
On 10/10/2019 15:42, Alexander Lobakin wrote: > Commit 323ebb61e32b ("net: use listified RX for handling GRO_NORMAL > skbs") have introduced a sysctl variable gro_normal_batch for defining > a limit for listified Rx of GRO_NORMAL skbs. The initial value of 8 is > purely arbitrary and has been chosen, I believe, as a minimal safe > default. 8 was chosen by performance tests on my setup with v1 of that patch; see https://www.spinics.net/lists/netdev/msg585001.html . Sorry for not including that info in the final version of the patch. While I didn't re-do tests on varying gro_normal_batch on the final version, I think changing it needs more evidence than just "we tested it; it's better". In particular, increasing the batch size should be accompanied by demonstration that latency isn't increased in e.g. a multi-stream ping-pong test. > However, several tests show that it's rather suboptimal and doesn't > allow to take a full advantage of listified processing. The best and > the most balanced results have been achieved with a batches of 16 skbs > per flush. > So double the default value to give a yet another boost for Rx path. > It remains configurable via sysctl anyway, so may be fine-tuned for > each hardware. I see this as a reason to leave the default as it is; the combination of your tests and mine have established that the optimal size does vary (I found 16 to be 2% slower than 8 with my setup), so any tweaking of the default is likely only worthwhile if we have data over lots of different hardware combinations. > Signed-off-by: Alexander Lobakin <alobakin@dlink.ru> > --- > net/core/dev.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/core/dev.c b/net/core/dev.c > index a33f56b439ce..4f60444bb766 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -4189,7 +4189,7 @@ int dev_weight_tx_bias __read_mostly = 1; /* bias for output_queue quota */ > int dev_rx_weight __read_mostly = 64; > int dev_tx_weight __read_mostly = 64; > /* Maximum number of GRO_NORMAL skbs to batch up for list-RX */ > -int gro_normal_batch __read_mostly = 8; > +int gro_normal_batch __read_mostly = 16; > > /* Called with irq disabled */ > static inline void ____napi_schedule(struct softnet_data *sd,
Hi Edward, Edward Cree wrote 10.10.2019 21:16: > On 10/10/2019 15:42, Alexander Lobakin wrote: >> Commit 323ebb61e32b ("net: use listified RX for handling GRO_NORMAL >> skbs") have introduced a sysctl variable gro_normal_batch for defining >> a limit for listified Rx of GRO_NORMAL skbs. The initial value of 8 is >> purely arbitrary and has been chosen, I believe, as a minimal safe >> default. > 8 was chosen by performance tests on my setup with v1 of that patch; > see https://www.spinics.net/lists/netdev/msg585001.html . > Sorry for not including that info in the final version of the patch. > While I didn't re-do tests on varying gro_normal_batch on the final > version, I think changing it needs more evidence than just "we tested > it; it's better". In particular, increasing the batch size should be > accompanied by demonstration that latency isn't increased in e.g. a > multi-stream ping-pong test. > >> However, several tests show that it's rather suboptimal and doesn't >> allow to take a full advantage of listified processing. The best and >> the most balanced results have been achieved with a batches of 16 skbs >> per flush. >> So double the default value to give a yet another boost for Rx path. > >> It remains configurable via sysctl anyway, so may be fine-tuned for >> each hardware. > I see this as a reason to leave the default as it is; the combination > of your tests and mine have established that the optimal size does > vary (I found 16 to be 2% slower than 8 with my setup), so any > tweaking of the default is likely only worthwhile if we have data > over lots of different hardware combinations. Agree, if you've got slower results on 16, we must leave the default value, as it seems to be VERY hardware- and driver- dependent. So, patch 2/2 is not actual any more (I supposed that it would likely go away before sending this series). >> Signed-off-by: Alexander Lobakin <alobakin@dlink.ru> >> --- >> net/core/dev.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/net/core/dev.c b/net/core/dev.c >> index a33f56b439ce..4f60444bb766 100644 >> --- a/net/core/dev.c >> +++ b/net/core/dev.c >> @@ -4189,7 +4189,7 @@ int dev_weight_tx_bias __read_mostly = 1; /* >> bias for output_queue quota */ >> int dev_rx_weight __read_mostly = 64; >> int dev_tx_weight __read_mostly = 64; >> /* Maximum number of GRO_NORMAL skbs to batch up for list-RX */ >> -int gro_normal_batch __read_mostly = 8; >> +int gro_normal_batch __read_mostly = 16; >> >> /* Called with irq disabled */ >> static inline void ____napi_schedule(struct softnet_data *sd, Regards, ᚷ ᛖ ᚢ ᚦ ᚠ ᚱ
Alexander Lobakin wrote 11.10.2019 10:23: > Hi Edward, > > Edward Cree wrote 10.10.2019 21:16: >> On 10/10/2019 15:42, Alexander Lobakin wrote: >>> Commit 323ebb61e32b ("net: use listified RX for handling GRO_NORMAL >>> skbs") have introduced a sysctl variable gro_normal_batch for >>> defining >>> a limit for listified Rx of GRO_NORMAL skbs. The initial value of 8 >>> is >>> purely arbitrary and has been chosen, I believe, as a minimal safe >>> default. >> 8 was chosen by performance tests on my setup with v1 of that patch; >> see https://www.spinics.net/lists/netdev/msg585001.html . >> Sorry for not including that info in the final version of the patch. >> While I didn't re-do tests on varying gro_normal_batch on the final >> version, I think changing it needs more evidence than just "we tested >> it; it's better". In particular, increasing the batch size should be >> accompanied by demonstration that latency isn't increased in e.g. a >> multi-stream ping-pong test. >> >>> However, several tests show that it's rather suboptimal and doesn't >>> allow to take a full advantage of listified processing. The best and >>> the most balanced results have been achieved with a batches of 16 >>> skbs >>> per flush. >>> So double the default value to give a yet another boost for Rx path. >> >>> It remains configurable via sysctl anyway, so may be fine-tuned for >>> each hardware. >> I see this as a reason to leave the default as it is; the combination >> of your tests and mine have established that the optimal size does >> vary (I found 16 to be 2% slower than 8 with my setup), so any >> tweaking of the default is likely only worthwhile if we have data >> over lots of different hardware combinations. > > Agree, if you've got slower results on 16, we must leave the default > value, as it seems to be VERY hardware- and driver- dependent. > So, patch 2/2 is not actual any more (I supposed that it would likely > go away before sending this series). I've generated an another solution. Considering that gro_normal_batch is very individual for every single case, maybe it would be better to make it per-NAPI (or per-netdevice) variable rather than a global across the kernel? I think most of all network-capable configurations and systems has more than one network device nowadays, and they might need different values for achieving their bests. One possible variant is: #define THIS_DRIVER_GRO_NORMAL_BATCH 16 /* ... */ netif_napi_add(dev, napi, this_driver_rx_poll, NAPI_POLL_WEIGHT); /* napi->gro_normal_batch will be set to the systcl value during NAPI context initialization */ napi_set_gro_normal_batch(napi, THIS_DRIVER_GRO_NORMAL_BATCH); /* new static inline helper, napi->gro_normal_batch will be set to the driver-speficic value of 16 */ The second possible variant is to make gro_normal_batch sysctl per-netdevice to tune it from userspace. Or we can combine them into one to make it available for tweaking from both driver and userspace, just like it's now with XPS CPUs setting. If you'll find any of this reasonable and worth implementing, I'll come with it in v2 after a proper testing. > >>> Signed-off-by: Alexander Lobakin <alobakin@dlink.ru> >>> --- >>> net/core/dev.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/net/core/dev.c b/net/core/dev.c >>> index a33f56b439ce..4f60444bb766 100644 >>> --- a/net/core/dev.c >>> +++ b/net/core/dev.c >>> @@ -4189,7 +4189,7 @@ int dev_weight_tx_bias __read_mostly = 1; /* >>> bias for output_queue quota */ >>> int dev_rx_weight __read_mostly = 64; >>> int dev_tx_weight __read_mostly = 64; >>> /* Maximum number of GRO_NORMAL skbs to batch up for list-RX */ >>> -int gro_normal_batch __read_mostly = 8; >>> +int gro_normal_batch __read_mostly = 16; >>> >>> /* Called with irq disabled */ >>> static inline void ____napi_schedule(struct softnet_data *sd, > > Regards, > ᚷ ᛖ ᚢ ᚦ ᚠ ᚱ Regards, ᚷ ᛖ ᚢ ᚦ ᚠ ᚱ
On Sat, Oct 12, 2019 at 2:22 AM Alexander Lobakin <alobakin@dlink.ru> wrote: > > I've generated an another solution. Considering that gro_normal_batch > is very individual for every single case, maybe it would be better to > make it per-NAPI (or per-netdevice) variable rather than a global > across the kernel? > I think most of all network-capable configurations and systems has more > than one network device nowadays, and they might need different values > for achieving their bests. > > One possible variant is: > > #define THIS_DRIVER_GRO_NORMAL_BATCH 16 > > /* ... */ > > netif_napi_add(dev, napi, this_driver_rx_poll, NAPI_POLL_WEIGHT); /* > napi->gro_normal_batch will be set to the systcl value during NAPI > context initialization */ > napi_set_gro_normal_batch(napi, THIS_DRIVER_GRO_NORMAL_BATCH); /* new > static inline helper, napi->gro_normal_batch will be set to the > driver-speficic value of 16 */ > > The second possible variant is to make gro_normal_batch sysctl > per-netdevice to tune it from userspace. > Or we can combine them into one to make it available for tweaking from > both driver and userspace, just like it's now with XPS CPUs setting. > > If you'll find any of this reasonable and worth implementing, I'll come > with it in v2 after a proper testing. Most likely the optimal tuning is also a function of the host cpu caches. Building a too big list can also lead to premature cache evictions. Tuning the value on your test machines does not mean the value will be good for other systems. Adding yet another per device value should only be done if you demonstrate a significant performance increase compared to the conservative value Edward chose. Also the behavior can be quite different depending on the protocols, make sure you test handling of TCP pure ACK packets. Accumulating 64 (in case the device uses standard NAPI_POLL_WEIGHT) of them before entering upper stacks seems not a good choice, since 64 skbs will need to be kept in the GRO system, compared to only 8 with Edward value.
Hi Eric, Eric Dumazet wrote 12.10.2019 14:18: > On Sat, Oct 12, 2019 at 2:22 AM Alexander Lobakin <alobakin@dlink.ru> > wrote: > >> >> I've generated an another solution. Considering that gro_normal_batch >> is very individual for every single case, maybe it would be better to >> make it per-NAPI (or per-netdevice) variable rather than a global >> across the kernel? >> I think most of all network-capable configurations and systems has >> more >> than one network device nowadays, and they might need different values >> for achieving their bests. >> >> One possible variant is: >> >> #define THIS_DRIVER_GRO_NORMAL_BATCH 16 >> >> /* ... */ >> >> netif_napi_add(dev, napi, this_driver_rx_poll, NAPI_POLL_WEIGHT); /* >> napi->gro_normal_batch will be set to the systcl value during NAPI >> context initialization */ >> napi_set_gro_normal_batch(napi, THIS_DRIVER_GRO_NORMAL_BATCH); /* new >> static inline helper, napi->gro_normal_batch will be set to the >> driver-speficic value of 16 */ >> >> The second possible variant is to make gro_normal_batch sysctl >> per-netdevice to tune it from userspace. >> Or we can combine them into one to make it available for tweaking from >> both driver and userspace, just like it's now with XPS CPUs setting. >> >> If you'll find any of this reasonable and worth implementing, I'll >> come >> with it in v2 after a proper testing. > > Most likely the optimal tuning is also a function of the host cpu > caches. > > Building a too big list can also lead to premature cache evictions. > > Tuning the value on your test machines does not mean the value will be > good > for other systems. Oh, I missed that it might be a lot more machine-dependent than netdevice-dependent. Thank you for explanation. The best I can do in that case is to leave batch control in its current. I'll publish v2 containing only the acked first part of the series on Monday if nothing serious will happen. Addition of listified Rx to napi_gro_receive() was the main goal anyway. > > Adding yet another per device value should only be done if you > demonstrate > a significant performance increase compared to the conservative value > Edward chose. > > Also the behavior can be quite different depending on the protocols, > make sure you test handling of TCP pure ACK packets. > > Accumulating 64 (in case the device uses standard NAPI_POLL_WEIGHT) > of them before entering upper stacks seems not a good choice, since 64 > skbs > will need to be kept in the GRO system, compared to only 8 with Edward > value. Regards, ᚷ ᛖ ᚢ ᚦ ᚠ ᚱ
diff --git a/net/core/dev.c b/net/core/dev.c index a33f56b439ce..4f60444bb766 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -4189,7 +4189,7 @@ int dev_weight_tx_bias __read_mostly = 1; /* bias for output_queue quota */ int dev_rx_weight __read_mostly = 64; int dev_tx_weight __read_mostly = 64; /* Maximum number of GRO_NORMAL skbs to batch up for list-RX */ -int gro_normal_batch __read_mostly = 8; +int gro_normal_batch __read_mostly = 16; /* Called with irq disabled */ static inline void ____napi_schedule(struct softnet_data *sd,
Commit 323ebb61e32b ("net: use listified RX for handling GRO_NORMAL skbs") have introduced a sysctl variable gro_normal_batch for defining a limit for listified Rx of GRO_NORMAL skbs. The initial value of 8 is purely arbitrary and has been chosen, I believe, as a minimal safe default. However, several tests show that it's rather suboptimal and doesn't allow to take a full advantage of listified processing. The best and the most balanced results have been achieved with a batches of 16 skbs per flush. So double the default value to give a yet another boost for Rx path. It remains configurable via sysctl anyway, so may be fine-tuned for each hardware. Signed-off-by: Alexander Lobakin <alobakin@dlink.ru> --- net/core/dev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)