| Message ID | 503659DD.6070708@canonical.com |
|---|---|
| State | New |
| Headers | show |
* patch "net: Allow driver to limit number of GSO segments per skb" I think the backport is wrong. On Lucid there is no netif_skb_features, but the check shouldn't go inside dev_can_checksum. As far as I understand the code changes, the clearing GSO flags in dev->features should happen right before netif_needs_gso, and on Lucid netif_needs_gso is called from two places instead of one (also on xen-netfront), so may be for Lucid the clearing of the feature flags could be done inside netif_needs_gso. * patch "tcp: Apply device TSO segment limit earlier" I didn't paid attention before, but I noticed now that on backports of this patch, on tcp_is_cwnd_limited, you are returning a bool on the backport while the function was still of int type, shouldn't cause problems but looks ugly. It happened on all backports of this patch since Precise, so if fixing this here on Lucid, probably should be fixed on the others as well (Natty->Precise). Also other thing that was not part of the change in the commit upstream, and is harmless anyway, is that on tcp_mss_split_point, you change "struct tcp_sock *tp" declaration to const. I don't think is a problem anyway, but isn't related to the change. It wasn't const in Oneiric and earlier. * patch "sfc: Fix maximum number of TSO segments and minimum TX queue size" On this last one, I saw you define new macros, EFX_MAX_DMAQ_SIZE/EFX_MIN_DMAQ_SIZE, but they are not used on the backport, is that needed? Also EFX_TXQ_SIZE is changed, but doesn't seem needed as well on the backport.
On 08/23/2012 01:24 PM, Herton Ronaldo Krzesinski wrote: > * patch "net: Allow driver to limit number of GSO segments per skb" > > I think the backport is wrong. On Lucid there is no netif_skb_features, > but the check shouldn't go inside dev_can_checksum. As far as I > understand the code changes, the clearing GSO flags in dev->features > should happen right before netif_needs_gso, and on Lucid netif_needs_gso > is called from two places instead of one (also on xen-netfront), so may > be for Lucid the clearing of the feature flags could be done inside > netif_needs_gso. > You are correct. I didn't look at that one close enough. > * patch "tcp: Apply device TSO segment limit earlier" > > I didn't paid attention before, but I noticed now that on backports of > this patch, on tcp_is_cwnd_limited, you are returning a bool on the > backport while the function was still of int type, shouldn't cause > problems but looks ugly. It happened on all backports of this patch > since Precise, so if fixing this here on Lucid, probably should be fixed > on the others as well (Natty->Precise). > The code is functionally equivalent, and is true to the upstream patch so I'm inclined to leave it be. > Also other thing that was not part of the change in the commit upstream, > and is harmless anyway, is that on tcp_mss_split_point, you change > "struct tcp_sock *tp" declaration to const. I don't think is a problem > anyway, but isn't related to the change. It wasn't const in Oneiric and > earlier. > The upstream prototype is 'const struct tcp_sock *tp', but I think its benign. > * patch "sfc: Fix maximum number of TSO segments and minimum TX queue size" > > On this last one, I saw you define new macros, > EFX_MAX_DMAQ_SIZE/EFX_MIN_DMAQ_SIZE, but they are not used on the > backport, is that needed? Also EFX_TXQ_SIZE is changed, but doesn't seem > needed as well on the backport. > EFX_MAX_DMAQ_SIZE/EFX_MIN_DMAQ_SIZE came in with the patch backport and are indeed unused (and benign). I changed both EFX_TXQ_SIZE and EFX_RXQ_SIZE to use EFX_DEFAULT_DMAQ_SIZE so that neither is a magic number. I've repushed with an update to "net: Allow driver to limit number of GSO segments per skb" rtg
On Thu, Aug 23, 2012 at 02:14:23PM -0600, Tim Gardner wrote: [...] > I've repushed with an update to "net: Allow driver to limit number of > GSO segments per skb" This one still doesn't look ok. You clear dev->features, but the behaviour of the upstream patch is different: they copy dev->features to a local variable, and clear that, passing to netif_needs_gso, otherwise after you clear first time, dev->features will have always the flags cleared, and the checks are per skb, shouldn't be cleared globally in dev->features. Fixing this, Ack from me for the patch series, the other things I just wanted to point out but should be benign or style issues. > > rtg > -- > Tim Gardner tim.gardner@canonical.com >
The following changes since commit cf52d980c29eade382d95198b7362787038b9ef5: eCryptfs: Initialize empty lower files when opening them (2012-08-23 12:05:04 -0400) are available in the git repository at: git://kernel.ubuntu.com/rtg/ubuntu-lucid.git CVE-2012-3412 for you to fetch changes up to 67438dd33e0bb1e2f120f4f6672b5c43d30ce5fd: sfc: Fix maximum number of TSO segments and minimum TX queue size (2012-08-23 10:07:17 -0600) ---------------------------------------------------------------- Ben Hutchings (4): net: Allow driver to limit number of GSO segments per skb tcp: Apply device TSO segment limit earlier sfc: Replace some literal constants with EFX_PAGE_SIZE/EFX_BUF_SIZE sfc: Fix maximum number of TSO segments and minimum TX queue size Neal Cardwell (1): tcp: do not scale TSO segment size with reordering degree drivers/net/sfc/efx.c | 8 ++++++++ drivers/net/sfc/efx.h | 19 +++++++++++++++++-- drivers/net/sfc/nic.c | 5 +---- drivers/net/sfc/nic.h | 5 +++++ drivers/net/sfc/tx.c | 21 ++++++++++++++++++++- include/linux/netdevice.h | 2 ++ include/net/sock.h | 2 ++ include/net/tcp.h | 8 ++++++++ net/core/dev.c | 4 ++++ net/core/sock.c | 1 + net/ipv4/tcp.c | 4 +++- net/ipv4/tcp_cong.c | 7 ++++--- net/ipv4/tcp_output.c | 27 +++++++++++++++------------ 13 files changed, 90 insertions(+), 23 deletions(-)