ACK/cmnt: [SRU][Trusty][PATCH v2 0/4] bridge: Provide IPv6 refragmentation for bridge conntrack
diff mbox

Message ID 1412.1455913048@famine
State New
Headers show

Commit Message

Jay Vosburgh Feb. 19, 2016, 8:17 p.m. UTC
Stefan Bader <stefan.bader@canonical.com> wrote:

>On 17.02.2016 01:41, Jay Vosburgh wrote:
>> BugLink: https://bugs.launchpad.net/nova/+bug/1463911
>> 
>> 	This is a respin of
>> 
>> https://lists.ubuntu.com/archives/kernel-team/2015-November/065772.html
>> https://lists.ubuntu.com/archives/kernel-team/2015-November/065773.html
>> https://lists.ubuntu.com/archives/kernel-team/2015-November/065774.html
>> 
>> 	to break out the consolidated patches into their individual parts.
>> 
>> 	This patch series provides the ability to re-fragment IPv6
>> datagrams being forwarded by the bridge when conntrack is enabled.
>> Conntrack will defragment any datagrams it processes, and presently the
>> bridge cannot refragment them on egress (resulting in a drop of affected
>> datagrams).
>> 
>> 	Patches tested according to the recipe provided in patch 3 of the
>> series.
>
>ACK more based on testing results. Especially #3 is hard to verify without being
>accustomed to the network code.
>
>Minor thought on #1: The commit messages talks about renaming a data structure
>which was not actually done. Not sure there is already an established method
>here but maybe quick note under the backported ...

	That comment in the commit log refers to the following change in
the upstream patch:

	I left that particular change out because, on the kernels at
issue here (3.13, 3.16 and 3.19), the assertion in the commit log that
the "only user left is neigh resolution" is not true.  Including this
change would require a cascade of other changes (because the extant
usage exceeds the new neigh_header[8] capacity).

	That said, I should have explained it better somewhere; I can
respin the patches with higher verbosity in the commit log if that would
be preferred.

	-J

---
	-Jay Vosburgh, jay.vosburgh@canonical.com

Comments

Stefan Bader Feb. 22, 2016, 11:38 a.m. UTC | #1
On 19.02.2016 21:17, Jay Vosburgh wrote:
> Stefan Bader <stefan.bader@canonical.com> wrote:
> 
>> On 17.02.2016 01:41, Jay Vosburgh wrote:
>>> BugLink: https://bugs.launchpad.net/nova/+bug/1463911
>>>
>>> 	This is a respin of
>>>
>>> https://lists.ubuntu.com/archives/kernel-team/2015-November/065772.html
>>> https://lists.ubuntu.com/archives/kernel-team/2015-November/065773.html
>>> https://lists.ubuntu.com/archives/kernel-team/2015-November/065774.html
>>>
>>> 	to break out the consolidated patches into their individual parts.
>>>
>>> 	This patch series provides the ability to re-fragment IPv6
>>> datagrams being forwarded by the bridge when conntrack is enabled.
>>> Conntrack will defragment any datagrams it processes, and presently the
>>> bridge cannot refragment them on egress (resulting in a drop of affected
>>> datagrams).
>>>
>>> 	Patches tested according to the recipe provided in patch 3 of the
>>> series.
>>
>> ACK more based on testing results. Especially #3 is hard to verify without being
>> accustomed to the network code.
>>
>> Minor thought on #1: The commit messages talks about renaming a data structure
>> which was not actually done. Not sure there is already an established method
>> here but maybe quick note under the backported ...
> 
> 	That comment in the commit log refers to the following change in
> the upstream patch:
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 36f3f43c0117..f66a089afc41 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -169,7 +169,7 @@ struct nf_bridge_info {
>         unsigned int            mask;
>         struct net_device       *physindev;
>         struct net_device       *physoutdev;
> -       unsigned long           data[32 / sizeof(unsigned long)];
> +       char                    neigh_header[8];
>  };
>  #endif
>  
> 	I left that particular change out because, on the kernels at
> issue here (3.13, 3.16 and 3.19), the assertion in the commit log that
> the "only user left is neigh resolution" is not true.  Including this
> change would require a cascade of other changes (because the extant
> usage exceeds the new neigh_header[8] capacity).
> 
> 	That said, I should have explained it better somewhere; I can
> respin the patches with higher verbosity in the commit log if that would
> be preferred.

Was more a remark than a requirement. As I wrote I am not even sure there is
anything preferred way established. When reviewing I find it helpful to ignore
obvious differences and when applied it may (or may not) help to quickly figure
out how a patch may be different from the upstream version.
But even if this is thought of as improvement it can be added when applying the
set. So no need for a re-spin. And in fact its moot as that has already happened. :)

-Stefan
> 
> 	-J
> 
> ---
> 	-Jay Vosburgh, jay.vosburgh@canonical.com
>

Patch
diff mbox

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 36f3f43c0117..f66a089afc41 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -169,7 +169,7 @@  struct nf_bridge_info {
        unsigned int            mask;
        struct net_device       *physindev;
        struct net_device       *physoutdev;
-       unsigned long           data[32 / sizeof(unsigned long)];
+       char                    neigh_header[8];
 };
 #endif