| Message ID | 1327598603-16398-1-git-send-email-wei.liu2@citrix.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, Jan 26, 2012 at 05:23:23PM +0000, Wei Liu wrote: Can you give some more details please? What is the impact of not having this? Should it be backported to stable? > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > --- > drivers/net/xen-netfront.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c > index 38f9c95..01f589d 100644 > --- a/drivers/net/xen-netfront.c > +++ b/drivers/net/xen-netfront.c > @@ -68,7 +68,7 @@ struct netfront_cb { > > #define NET_TX_RING_SIZE __CONST_RING_SIZE(xen_netif_tx, PAGE_SIZE) > #define NET_RX_RING_SIZE __CONST_RING_SIZE(xen_netif_rx, PAGE_SIZE) > -#define TX_MAX_TARGET min_t(int, NET_RX_RING_SIZE, 256) > +#define TX_MAX_TARGET min_t(int, NET_TX_RING_SIZE, 256) > > struct netfront_stats { > u64 rx_packets; > -- > 1.7.8.3 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Wei Liu <wei.liu2@citrix.com> Date: Thu, 26 Jan 2012 17:23:23 +0000 > Signed-off-by: Wei Liu <wei.liu2@citrix.com> Pretty obvious and straightforward, applied, thanks! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2012-01-26 at 18:19 +0000, Konrad Rzeszutek Wilk wrote: > On Thu, Jan 26, 2012 at 05:23:23PM +0000, Wei Liu wrote: > > Can you give some more details please? What is the impact of > not having this? Should it be backported to stable? > I think it will not cause crash, only the scratch space size is affected, thus impacting tx batching a bit. As the tx structure is bigger than rx structure. I think scratch space size is likely to shrink after correction. Wei. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/27/12 11:36, Wei Liu wrote: > On Thu, 2012-01-26 at 18:19 +0000, Konrad Rzeszutek Wilk wrote: >> On Thu, Jan 26, 2012 at 05:23:23PM +0000, Wei Liu wrote: >> >> Can you give some more details please? What is the impact of >> not having this? Should it be backported to stable? >> > > I think it will not cause crash, only the scratch space size is > affected, thus impacting tx batching a bit. > > As the tx structure is bigger than rx structure. I think scratch space > size is likely to shrink after correction. It also seems to affect the netfront_tx_slot_available() function, making it stricter (likely). Before the patch, the function may have reported available slots when there were none, causing spurious(?) queue wakeups in xennet_maybe_wake_tx(), and not stopping the queue in xennet_start_xmit() when it should have(?). It seems there are no further uses of TX_MAX_TARGET, and for bounds checking NET_TX_RING_SIZE was used (which was always correct). So I guess the typo may have caused some performance degradation. I can't either prove or disprove a DoS-like busy loop in the pre-patch form. Laszlo -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/03/12 13:27, Laszlo Ersek wrote: > On 01/27/12 11:36, Wei Liu wrote: >> As the tx structure is bigger than rx structure. I think scratch space >> size is likely to shrink after correction. > > It also seems to affect the netfront_tx_slot_available() function, > making it stricter (likely). Before the patch, the function may have > reported available slots when there were none, causing spurious(?) queue > wakeups in xennet_maybe_wake_tx(), and not stopping the queue in > xennet_start_xmit() when it should have(?). (Eyeballing the source makes me think NET_TX_RING_SIZE == (4096 - 16 - 48) / (5 * 4) == 201 NET_RX_RING_SIZE == (4096 - 16 - 48) / (4 * 4) == 252 but I didn't try to verify them.) Laszlo -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>>> On 03.02.12 at 13:59, Laszlo Ersek <lersek@redhat.com> wrote: > On 02/03/12 13:27, Laszlo Ersek wrote: >> On 01/27/12 11:36, Wei Liu wrote: > >>> As the tx structure is bigger than rx structure. I think scratch space >>> size is likely to shrink after correction. >> >> It also seems to affect the netfront_tx_slot_available() function, >> making it stricter (likely). Before the patch, the function may have >> reported available slots when there were none, causing spurious(?) queue >> wakeups in xennet_maybe_wake_tx(), and not stopping the queue in >> xennet_start_xmit() when it should have(?). > > (Eyeballing the source makes me think > > NET_TX_RING_SIZE == (4096 - 16 - 48) / (5 * 4) == 201 > NET_RX_RING_SIZE == (4096 - 16 - 48) / (4 * 4) == 252 NET_TX_RING_SIZE == (4096 - 16 - 48) / (6 * 2) == 336 NET_RX_RING_SIZE == (4096 - 16 - 48) / (4 * 2) == 504 and with {R,T}X_MAX_TARGET capped to 256 the change really is benign without multi-page ring support afaict. Jan > but I didn't try to verify them.) > > Laszlo > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/03/12 14:26, Jan Beulich wrote: >>>> On 03.02.12 at 13:59, Laszlo Ersek<lersek@redhat.com> wrote: >> (Eyeballing the source makes me think >> >> NET_TX_RING_SIZE == (4096 - 16 - 48) / (5 * 4) == 201 >> NET_RX_RING_SIZE == (4096 - 16 - 48) / (4 * 4) == 252 > > NET_TX_RING_SIZE == (4096 - 16 - 48) / (6 * 2) == 336 > NET_RX_RING_SIZE == (4096 - 16 - 48) / (4 * 2) == 504 Sorry, I wasn't sure if gcc would pack them without __attribute__((packed)) :) Dumb mistake, admittedly. Thanks, Laszlo -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c index 38f9c95..01f589d 100644 --- a/drivers/net/xen-netfront.c +++ b/drivers/net/xen-netfront.c @@ -68,7 +68,7 @@ struct netfront_cb { #define NET_TX_RING_SIZE __CONST_RING_SIZE(xen_netif_tx, PAGE_SIZE) #define NET_RX_RING_SIZE __CONST_RING_SIZE(xen_netif_rx, PAGE_SIZE) -#define TX_MAX_TARGET min_t(int, NET_RX_RING_SIZE, 256) +#define TX_MAX_TARGET min_t(int, NET_TX_RING_SIZE, 256) struct netfront_stats { u64 rx_packets;
Signed-off-by: Wei Liu <wei.liu2@citrix.com> --- drivers/net/xen-netfront.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)