[B.A.T.M.A.N.] skb->priority and fragments

[B.A.T.M.A.N.] skb->priority and fragments

[Andrew Lunn]

Hi Folks

Does anybody remember the history for the follow and can explain why
the code is as it is?

The soft interface transmit function, batadv_interface_tx() calls
batadv_skb_set_priority(skb, 0) to set the skb->priority based on the
TOS bits or 801.p.

If the packet needs to be fragmented because of MTU issues,
batadv_frag_create() is used to create the fragments. It overwrites
the skb->priority in the original skb to TC_PRIO_CONTROL, and leaves
the fragment skb with the default priority.

This seems a bit odd to me. I would of expected the priority to of
been copied from the original into the fragment.

Thanks
	Andrew

Re: [B.A.T.M.A.N.] skb->priority and fragments

[Simon Wunderlich]

[-- Attachment #1: Type: text/plain, Size: 1484 bytes --]

Hi Andrew,

On Tuesday 12 April 2016 22:42:59 Andrew Lunn wrote:
> Hi Folks
> 
> Does anybody remember the history for the follow and can explain why
> the code is as it is?
> 
> The soft interface transmit function, batadv_interface_tx() calls
> batadv_skb_set_priority(skb, 0) to set the skb->priority based on the
> TOS bits or 801.p.
> 
> If the packet needs to be fragmented because of MTU issues,
> batadv_frag_create() is used to create the fragments. It overwrites
> the skb->priority in the original skb to TC_PRIO_CONTROL, and leaves
> the fragment skb with the default priority.
> 
> This seems a bit odd to me. I would of expected the priority to of
> been copied from the original into the fragment.

I think this part could be improved. Right now, if I remember correctly, we 
set TC_PRIO_CONTROL by default and set another priority if we can parse the 
header (batadv_skb_set_priority()).

There are two cases:

1.) On the original sender, both fragments could adopt the priority as you 
suggest. The code probably doesn't take care of that yet, so that could be 
fixed.

2.) On routers on the way, the priority could only be set based on the first 
fragment, since the second fragment will not have a valid header to parse. And 
unless we remember the priority from the first fragment, we have no way to 
know to which priority we should set the second fragment.

I believe case 1 can be fixed easily, for case 2 I don't have an idea right 
now. :)

Cheers,
     Simon

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [B.A.T.M.A.N.] skb->priority and fragments

[Andrew Lunn]

On Wed, Apr 13, 2016 at 01:11:12PM +0200, Simon Wunderlich wrote:
> Hi Andrew,
> 
> On Tuesday 12 April 2016 22:42:59 Andrew Lunn wrote:
> > Hi Folks
> > 
> > Does anybody remember the history for the follow and can explain why
> > the code is as it is?
> > 
> > The soft interface transmit function, batadv_interface_tx() calls
> > batadv_skb_set_priority(skb, 0) to set the skb->priority based on the
> > TOS bits or 801.p.
> > 
> > If the packet needs to be fragmented because of MTU issues,
> > batadv_frag_create() is used to create the fragments. It overwrites
> > the skb->priority in the original skb to TC_PRIO_CONTROL, and leaves
> > the fragment skb with the default priority.
> > 
> > This seems a bit odd to me. I would of expected the priority to of
> > been copied from the original into the fragment.

Hi Simon

> I think this part could be improved. Right now, if I remember correctly, we 
> set TC_PRIO_CONTROL by default and set another priority if we can parse the 
> header (batadv_skb_set_priority()).

Yes, that is what i thought the intention was. The implementation is a
bit different.

> There are two cases:
> 
> 1.) On the original sender, both fragments could adopt the priority as you 
> suggest. The code probably doesn't take care of that yet, so that could be 
> fixed.

I will cook up a patch for this.
 
> 2.) On routers on the way, the priority could only be set based on the first 
> fragment, since the second fragment will not have a valid header to parse. And 
> unless we remember the priority from the first fragment, we have no way to 
> know to which priority we should set the second fragment.

I don't think remembering works. It looks like it fragments from the
tail towards the head. So we are not going to receive the IP header
until we get the last fragment.

> I believe case 1 can be fixed easily, for case 2 I don't have an idea right 
> now. :)

There is one reasonable option i can think
of. batadv_skb_set_priority() extracts three bits of priority
information, either from the TOS bits, or the 801.q header.

The fragment header is:

struct batadv_frag_packet {
        u8     packet_type;
        u8     version;  /* batman version field */
        u8     ttl;
#if defined(__BIG_ENDIAN_BITFIELD)
        u8     no:4;
        u8     reserved:4;
#elif defined(__LITTLE_ENDIAN_BITFIELD)
        u8     reserved:4;
	u8     no:4;
#else
#error "unknown bitfield endianness"
#endif
        u8     dest[ETH_ALEN];
        u8     orig[ETH_ALEN];
        __be16 seqno;
	__be16 total_size;
};

Place the priority information into 3 of the 4 reserved bits. The
receiver can then set the skb->priority of the fragment before passing
it to the hard interface.

   Andrew

Re: [B.A.T.M.A.N.] skb->priority and fragments

[Simon Wunderlich]

[-- Attachment #1: Type: text/plain, Size: 2051 bytes --]

Hi Andrew,

On Wednesday 13 April 2016 14:14:16 Andrew Lunn wrote:
> [...]
> > There are two cases:
> > 
> > 1.) On the original sender, both fragments could adopt the priority as you
> > suggest. The code probably doesn't take care of that yet, so that could be
> > fixed.
> 
> I will cook up a patch for this.

Excellent!

> > 2.) On routers on the way, the priority could only be set based on the
> > first fragment, since the second fragment will not have a valid header to
> > parse. And unless we remember the priority from the first fragment, we
> > have no way to know to which priority we should set the second fragment.
> 
> I don't think remembering works. It looks like it fragments from the
> tail towards the head. So we are not going to receive the IP header
> until we get the last fragment.
> 

Ah,  you are deeper into that. But my hunch was also that it would be messy, 
since we can't guarantee the order of the fragments anyway.

> > I believe case 1 can be fixed easily, for case 2 I don't have an idea
> > right
> > now. :)
> 
> There is one reasonable option i can think
> of. batadv_skb_set_priority() extracts three bits of priority
> information, either from the TOS bits, or the 801.q header.
> 
> The fragment header is:
> 
> struct batadv_frag_packet {
>         u8     packet_type;
>         u8     version;  /* batman version field */
>         u8     ttl;
> #if defined(__BIG_ENDIAN_BITFIELD)
>         u8     no:4;
>         u8     reserved:4;
> #elif defined(__LITTLE_ENDIAN_BITFIELD)
>         u8     reserved:4;
> 	u8     no:4;
> #else
> #error "unknown bitfield endianness"
> #endif
>         u8     dest[ETH_ALEN];
>         u8     orig[ETH_ALEN];
>         __be16 seqno;
> 	__be16 total_size;
> };
> 
> Place the priority information into 3 of the 4 reserved bits. The
> receiver can then set the skb->priority of the fragment before passing
> it to the hard interface.

Ah, yes that sounds like an excellent idea! I like that. The default should be 
zero right now, which would also fit. :)

Thanks!
     Simon

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]