| Message ID | 20201223144533.4145-2-rasmus.villemoes@prevas.dk (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | MRP without hardware offload? | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | fail | Series targets non-next tree, but doesn't contain any Fixes tags |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 1 maintainers not CCed: nikolay@nvidia.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 16 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
The 12/23/2020 15:45, Rasmus Villemoes wrote: Hi Rasmus, > > Wireshark says that the MRP test packets cannot be decoded - and the > reason for that is that there's a two-byte hole filled with garbage > between the "transitions" and "timestamp" members. > > So Wireshark decodes the two garbage bytes and the top two bytes of > the timestamp written by the kernel as the timestamp value (which thus > fluctuates wildly), and interprets the lower two bytes of the > timestamp as a new (type, length) pair, which is of course broken. > > While my copy of the MRP standard is still under way [*], I cannot > imagine the standard specifying a two-byte hole here, and whoever > wrote the Wireshark decoding code seems to agree with that. > > The struct definitions live under include/uapi/, but they are not > really part of any kernel<->userspace API/ABI, so fixing the > definitions by adding the packed attribute should not cause any > compatibility issues. > > The remaining on-the-wire packet formats likely also don't contain > holes, but pahole and manual inspection says the current definitions > suffice. So adding the packed attribute to those is not strictly > needed, but might be done for good measure. > > [*] I will never understand how something hidden behind a +1000$ > paywall can be called a standard. > > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> > --- > include/uapi/linux/mrp_bridge.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/include/uapi/linux/mrp_bridge.h b/include/uapi/linux/mrp_bridge.h > index 6aeb13ef0b1e..d1d0cf65916d 100644 > --- a/include/uapi/linux/mrp_bridge.h > +++ b/include/uapi/linux/mrp_bridge.h > @@ -96,7 +96,7 @@ struct br_mrp_ring_test_hdr { > __be16 state; > __be16 transitions; > __be32 timestamp; > -}; > +} __attribute__((__packed__)); Yes, I agree that this should be packed but it also needs to be 32 bit alligned, so extra 2 bytes are needed. The same will apply also for 'br_mrp_ring_topo_hdr' > > struct br_mrp_ring_topo_hdr { > __be16 prio; > @@ -141,7 +141,7 @@ struct br_mrp_in_test_hdr { > __be16 state; > __be16 transitions; > __be32 timestamp; > -}; > +} __attribute__((__packed__)); > > struct br_mrp_in_topo_hdr { > __u8 sa[ETH_ALEN]; > -- > 2.23.0 >
> > @@ -96,7 +96,7 @@ struct br_mrp_ring_test_hdr { > > __be16 state; > > __be16 transitions; > > __be32 timestamp; > > -}; > > +} __attribute__((__packed__)); > > Yes, I agree that this should be packed but it also needs to be 32 bit > alligned, so extra 2 bytes are needed. The full structure is: struct br_mrp_ring_test_hdr { __be16 prio; __u8 sa[ETH_ALEN]; __be16 port_role; __be16 state; __be16 transitions; __be32 timestamp; }; If i'm looking at this correctly, the byte offsets are: 0-1 prio 2-7 sa 8-9 port_role 10-11 state 12-13 transition With packed you get 14-17 timestamp which is not 32 bit aligned. Do you mean the whole structure must be 32 bit aligned? We need to add two reserved bytes to the end of the structure? Andrew
Hi Andrew, The 12/23/2020 19:41, Andrew Lunn wrote: > > > > @@ -96,7 +96,7 @@ struct br_mrp_ring_test_hdr { > > > __be16 state; > > > __be16 transitions; > > > __be32 timestamp; > > > -}; > > > +} __attribute__((__packed__)); > > > > Yes, I agree that this should be packed but it also needs to be 32 bit > > alligned, so extra 2 bytes are needed. > > The full structure is: > > struct br_mrp_ring_test_hdr { > __be16 prio; > __u8 sa[ETH_ALEN]; > __be16 port_role; > __be16 state; > __be16 transitions; > __be32 timestamp; > }; > > If i'm looking at this correctly, the byte offsets are: > > 0-1 prio > 2-7 sa > 8-9 port_role > 10-11 state > 12-13 transition > > With packed you get > > 14-17 timestamp > > which is not 32 bit aligned. > > Do you mean the whole structure must be 32 bit aligned? We need to add > two reserved bytes to the end of the structure? Sorry, I looked too fast over this. First some info, in front of the 'br_mrp_ring_test_hdr', there is 'br_mrp_tlv_hdr' which is 2 bytes. So the frame will look something like this: ... |---------|----------------|----------------------|------------| .... ... | MRP Ver | br_mrp_tlv_hdr | br_mrp_ring_test_hdr | Common TLV | .... ... |---------|----------------|----------------------|------------| .... The standard says that for br_mrp_tlv_hdr + br_mrp_ring_test, 32 bit alignment shall be ensured. So my understanding was that it needs to be at word boundary(4 bytes). So based on this, if we use packed then we get the following offsets 0 type 1 length 2-3 prio 4-9 sa 10-11 port_role 12-13 state 14-15 transition 16-19 timestamp. Which is 20 bytes, that fits correctly. So my understanding is we need to use packed, to remove the hole between transition and timestamp as Rasmus suggested and should NOT use these 2 extra bytes that I mentioned because it would not be aligned anymore. Here you can find few more details about MRP[1] > > Andrew [1] https://www.youtube.com/watch?v=R3vQYfwiJ2M
On Wed, 23 Dec 2020 15:45:32 +0100 Rasmus Villemoes wrote: > Wireshark says that the MRP test packets cannot be decoded - and the > reason for that is that there's a two-byte hole filled with garbage > between the "transitions" and "timestamp" members. > > So Wireshark decodes the two garbage bytes and the top two bytes of > the timestamp written by the kernel as the timestamp value (which thus > fluctuates wildly), and interprets the lower two bytes of the > timestamp as a new (type, length) pair, which is of course broken. > > While my copy of the MRP standard is still under way [*], I cannot > imagine the standard specifying a two-byte hole here, and whoever > wrote the Wireshark decoding code seems to agree with that. > > The struct definitions live under include/uapi/, but they are not > really part of any kernel<->userspace API/ABI, so fixing the > definitions by adding the packed attribute should not cause any > compatibility issues. > > The remaining on-the-wire packet formats likely also don't contain > holes, but pahole and manual inspection says the current definitions > suffice. So adding the packed attribute to those is not strictly > needed, but might be done for good measure. > > [*] I will never understand how something hidden behind a +1000$ > paywall can be called a standard. > > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> > --- > include/uapi/linux/mrp_bridge.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/include/uapi/linux/mrp_bridge.h b/include/uapi/linux/mrp_bridge.h > index 6aeb13ef0b1e..d1d0cf65916d 100644 > --- a/include/uapi/linux/mrp_bridge.h > +++ b/include/uapi/linux/mrp_bridge.h > @@ -96,7 +96,7 @@ struct br_mrp_ring_test_hdr { > __be16 state; > __be16 transitions; > __be32 timestamp; > -}; > +} __attribute__((__packed__)); > > struct br_mrp_ring_topo_hdr { > __be16 prio; > @@ -141,7 +141,7 @@ struct br_mrp_in_test_hdr { > __be16 state; > __be16 transitions; > __be32 timestamp; > -}; > +} __attribute__((__packed__)); > > struct br_mrp_in_topo_hdr { > __u8 sa[ETH_ALEN]; Can we use this opportunity to move the definitions of these structures out of the uAPI to a normal kernel header?
The 12/28/2020 14:24, Jakub Kicinski wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Wed, 23 Dec 2020 15:45:32 +0100 Rasmus Villemoes wrote: > > Wireshark says that the MRP test packets cannot be decoded - and the > > reason for that is that there's a two-byte hole filled with garbage > > between the "transitions" and "timestamp" members. > > > > So Wireshark decodes the two garbage bytes and the top two bytes of > > the timestamp written by the kernel as the timestamp value (which thus > > fluctuates wildly), and interprets the lower two bytes of the > > timestamp as a new (type, length) pair, which is of course broken. > > > > While my copy of the MRP standard is still under way [*], I cannot > > imagine the standard specifying a two-byte hole here, and whoever > > wrote the Wireshark decoding code seems to agree with that. > > > > The struct definitions live under include/uapi/, but they are not > > really part of any kernel<->userspace API/ABI, so fixing the > > definitions by adding the packed attribute should not cause any > > compatibility issues. > > > > The remaining on-the-wire packet formats likely also don't contain > > holes, but pahole and manual inspection says the current definitions > > suffice. So adding the packed attribute to those is not strictly > > needed, but might be done for good measure. > > > > [*] I will never understand how something hidden behind a +1000$ > > paywall can be called a standard. > > > > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> > > --- > > include/uapi/linux/mrp_bridge.h | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/include/uapi/linux/mrp_bridge.h b/include/uapi/linux/mrp_bridge.h > > index 6aeb13ef0b1e..d1d0cf65916d 100644 > > --- a/include/uapi/linux/mrp_bridge.h > > +++ b/include/uapi/linux/mrp_bridge.h > > @@ -96,7 +96,7 @@ struct br_mrp_ring_test_hdr { > > __be16 state; > > __be16 transitions; > > __be32 timestamp; > > -}; > > +} __attribute__((__packed__)); > > > > struct br_mrp_ring_topo_hdr { > > __be16 prio; > > @@ -141,7 +141,7 @@ struct br_mrp_in_test_hdr { > > __be16 state; > > __be16 transitions; > > __be32 timestamp; > > -}; > > +} __attribute__((__packed__)); > > > > struct br_mrp_in_topo_hdr { > > __u8 sa[ETH_ALEN]; > > Can we use this opportunity to move the definitions of these structures > out of the uAPI to a normal kernel header? Or maybe we can just remove them, especially if they are not used by the kernel.
On 28/12/2020 23.24, Jakub Kicinski wrote: > On Wed, 23 Dec 2020 15:45:32 +0100 Rasmus Villemoes wrote: >> Wireshark says that the MRP test packets cannot be decoded - and the >> reason for that is that there's a two-byte hole filled with garbage >> between the "transitions" and "timestamp" members. >> >> So Wireshark decodes the two garbage bytes and the top two bytes of >> the timestamp written by the kernel as the timestamp value (which thus >> fluctuates wildly), and interprets the lower two bytes of the >> timestamp as a new (type, length) pair, which is of course broken. >> >> While my copy of the MRP standard is still under way [*], I cannot >> imagine the standard specifying a two-byte hole here, and whoever >> wrote the Wireshark decoding code seems to agree with that. >> >> The struct definitions live under include/uapi/, but they are not >> really part of any kernel<->userspace API/ABI, so fixing the >> definitions by adding the packed attribute should not cause any >> compatibility issues. >> >> The remaining on-the-wire packet formats likely also don't contain >> holes, but pahole and manual inspection says the current definitions >> suffice. So adding the packed attribute to those is not strictly >> needed, but might be done for good measure. >> >> [*] I will never understand how something hidden behind a +1000$ >> paywall can be called a standard. >> >> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> >> --- >> include/uapi/linux/mrp_bridge.h | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/include/uapi/linux/mrp_bridge.h b/include/uapi/linux/mrp_bridge.h >> index 6aeb13ef0b1e..d1d0cf65916d 100644 >> --- a/include/uapi/linux/mrp_bridge.h >> +++ b/include/uapi/linux/mrp_bridge.h >> @@ -96,7 +96,7 @@ struct br_mrp_ring_test_hdr { >> __be16 state; >> __be16 transitions; >> __be32 timestamp; >> -}; >> +} __attribute__((__packed__)); >> >> struct br_mrp_ring_topo_hdr { >> __be16 prio; >> @@ -141,7 +141,7 @@ struct br_mrp_in_test_hdr { >> __be16 state; >> __be16 transitions; >> __be32 timestamp; >> -}; >> +} __attribute__((__packed__)); >> >> struct br_mrp_in_topo_hdr { >> __u8 sa[ETH_ALEN]; > > Can we use this opportunity to move the definitions of these structures > out of the uAPI to a normal kernel header? > Jakub, can we apply this patch to net, then later move the definitions out of uapi (and deleting the unused ones in the process)? Thanks, Rasmus
On Tue, 19 Jan 2021 16:42:14 +0100 Rasmus Villemoes wrote: > On 28/12/2020 23.24, Jakub Kicinski wrote: > > On Wed, 23 Dec 2020 15:45:32 +0100 Rasmus Villemoes wrote: > >> Wireshark says that the MRP test packets cannot be decoded - and the > >> reason for that is that there's a two-byte hole filled with garbage > >> between the "transitions" and "timestamp" members. > >> > >> So Wireshark decodes the two garbage bytes and the top two bytes of > >> the timestamp written by the kernel as the timestamp value (which thus > >> fluctuates wildly), and interprets the lower two bytes of the > >> timestamp as a new (type, length) pair, which is of course broken. > >> > >> While my copy of the MRP standard is still under way [*], I cannot > >> imagine the standard specifying a two-byte hole here, and whoever > >> wrote the Wireshark decoding code seems to agree with that. > >> > >> The struct definitions live under include/uapi/, but they are not > >> really part of any kernel<->userspace API/ABI, so fixing the > >> definitions by adding the packed attribute should not cause any > >> compatibility issues. > >> > >> The remaining on-the-wire packet formats likely also don't contain > >> holes, but pahole and manual inspection says the current definitions > >> suffice. So adding the packed attribute to those is not strictly > >> needed, but might be done for good measure. > >> > >> [*] I will never understand how something hidden behind a +1000$ > >> paywall can be called a standard. > >> > >> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> > >> --- > >> include/uapi/linux/mrp_bridge.h | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/include/uapi/linux/mrp_bridge.h b/include/uapi/linux/mrp_bridge.h > >> index 6aeb13ef0b1e..d1d0cf65916d 100644 > >> --- a/include/uapi/linux/mrp_bridge.h > >> +++ b/include/uapi/linux/mrp_bridge.h > >> @@ -96,7 +96,7 @@ struct br_mrp_ring_test_hdr { > >> __be16 state; > >> __be16 transitions; > >> __be32 timestamp; > >> -}; > >> +} __attribute__((__packed__)); > >> > >> struct br_mrp_ring_topo_hdr { > >> __be16 prio; > >> @@ -141,7 +141,7 @@ struct br_mrp_in_test_hdr { > >> __be16 state; > >> __be16 transitions; > >> __be32 timestamp; > >> -}; > >> +} __attribute__((__packed__)); > >> > >> struct br_mrp_in_topo_hdr { > >> __u8 sa[ETH_ALEN]; > > > > Can we use this opportunity to move the definitions of these structures > > out of the uAPI to a normal kernel header? > > Jakub, can we apply this patch to net, then later move the definitions > out of uapi (and deleting the unused ones in the process)? This has been lost in the patchwork archives already, we'll need a fresh posting anyway. Why not do the move as a part of the same series? Doesn't seems like much work, unless I'm missing something..
diff --git a/include/uapi/linux/mrp_bridge.h b/include/uapi/linux/mrp_bridge.h index 6aeb13ef0b1e..d1d0cf65916d 100644 --- a/include/uapi/linux/mrp_bridge.h +++ b/include/uapi/linux/mrp_bridge.h @@ -96,7 +96,7 @@ struct br_mrp_ring_test_hdr { __be16 state; __be16 transitions; __be32 timestamp; -}; +} __attribute__((__packed__)); struct br_mrp_ring_topo_hdr { __be16 prio; @@ -141,7 +141,7 @@ struct br_mrp_in_test_hdr { __be16 state; __be16 transitions; __be32 timestamp; -}; +} __attribute__((__packed__)); struct br_mrp_in_topo_hdr { __u8 sa[ETH_ALEN];
Wireshark says that the MRP test packets cannot be decoded - and the reason for that is that there's a two-byte hole filled with garbage between the "transitions" and "timestamp" members. So Wireshark decodes the two garbage bytes and the top two bytes of the timestamp written by the kernel as the timestamp value (which thus fluctuates wildly), and interprets the lower two bytes of the timestamp as a new (type, length) pair, which is of course broken. While my copy of the MRP standard is still under way [*], I cannot imagine the standard specifying a two-byte hole here, and whoever wrote the Wireshark decoding code seems to agree with that. The struct definitions live under include/uapi/, but they are not really part of any kernel<->userspace API/ABI, so fixing the definitions by adding the packed attribute should not cause any compatibility issues. The remaining on-the-wire packet formats likely also don't contain holes, but pahole and manual inspection says the current definitions suffice. So adding the packed attribute to those is not strictly needed, but might be done for good measure. [*] I will never understand how something hidden behind a +1000$ paywall can be called a standard. Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> --- include/uapi/linux/mrp_bridge.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)