| Message ID | 586321df-72c6-0dee-4ce6-22ca2a0860fb@cumulusnetworks.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, Aug 24, 2016 at 9:37 AM, David Ahern <dsa@cumulusnetworks.com> wrote: > On 8/24/16 10:28 AM, pravin shelar wrote: >>> How do you feel about implementing the do_output() idea I suggested above? >>> I'm happy to provide testing and review. >> >> I am not sure about changing do_output(). why not just use same scheme >> to track mpls header in OVS datapath as done in mpls device? >> > > was just replying with the same. > > Something like this should be able to handle multiple labels. The inner network header is set once and the outer one pointing to MPLS is adjusted each time a label is pushed: > > diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c > index 1ecbd7715f6d..0f37b17e3a73 100644 > --- a/net/openvswitch/actions.c > +++ b/net/openvswitch/actions.c > @@ -162,10 +162,16 @@ static int push_mpls(struct sk_buff *skb, struct sw_flow_key *key, > if (skb_cow_head(skb, MPLS_HLEN) < 0) > return -ENOMEM; > > + if (!skb->inner_protocol) { > + skb_set_inner_network_header(skb, skb->mac_len); > + skb_set_inner_protocol(skb, skb->protocol); > + } > + > skb_push(skb, MPLS_HLEN); > memmove(skb_mac_header(skb) - MPLS_HLEN, skb_mac_header(skb), > skb->mac_len); > skb_reset_mac_header(skb); > + skb_set_network_header(skb, skb->mac_len); > > new_mpls_lse = (__be32 *)skb_mpls_header(skb); > *new_mpls_lse = mpls->mpls_lse; > @@ -173,8 +179,7 @@ static int push_mpls(struct sk_buff *skb, struct sw_flow_key *key, > skb_postpush_rcsum(skb, new_mpls_lse, MPLS_HLEN); > > update_ethertype(skb, eth_hdr(skb), mpls->mpls_ethertype); > - if (!skb->inner_protocol) > - skb_set_inner_protocol(skb, skb->protocol); > + > skb->protocol = mpls->mpls_ethertype; > > invalidate_flow_key(key); > > > > > If it does, what else needs to be changed in OVS to handle the network layer now pointing to the MPLS labels? > You also need to change pop_mpls(). Anyways I was thinking about the neigh output functions skb pull issue, where it is using network-header offset. Can we use mac_len? this way we would not use any inner offsets for MPLS skb and current scheme used by OVS datapath works.
On Wed, 24 Aug 2016 10:37:51 -0600, David Ahern wrote: > Something like this should be able to handle multiple labels. The > inner network header is set once and the outer one pointing to MPLS > is adjusted each time a label is pushed: > > diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c > index 1ecbd7715f6d..0f37b17e3a73 100644 > --- a/net/openvswitch/actions.c > +++ b/net/openvswitch/actions.c > @@ -162,10 +162,16 @@ static int push_mpls(struct sk_buff *skb, > struct sw_flow_key *key, if (skb_cow_head(skb, MPLS_HLEN) < 0) > return -ENOMEM; > > + if (!skb->inner_protocol) { > + skb_set_inner_network_header(skb, skb->mac_len); > + skb_set_inner_protocol(skb, skb->protocol); > + } > + > skb_push(skb, MPLS_HLEN); > memmove(skb_mac_header(skb) - MPLS_HLEN, skb_mac_header(skb), > skb->mac_len); > skb_reset_mac_header(skb); > + skb_set_network_header(skb, skb->mac_len); Sorry for chiming in after a month. The code above got in (48d2ab609b6bb), I'm currently looking at this and it looks very suspicious to me. After push_mpls, network_header points to the start of MPLS headers. Which I understand was the point of this patch. However, push_mpls also calls invalidate_flow_key. Meaning that, depending on actions, we may end up calling key_extract soon after. And key_extract sets the network header *after* the MPLS headers. That means that on output, for otherwise identical packet, network_header can point before or after MPLS headers based on what actions happened to be executed (recirculation, mainly). If I'm not misreading the code or missing something, this can't be right. mpls_gso_segment does not care, it resets the network_header anyway. What about drivers? What is the correct behavior? Jiri
On Mon, 26 Sep 2016 17:56:22 +0200, Jiri Benc wrote: > After push_mpls, network_header points to the start of MPLS headers. > Which I understand was the point of this patch. However, push_mpls also > calls invalidate_flow_key. Meaning that, depending on actions, we may > end up calling key_extract soon after. And key_extract sets the network > header *after* the MPLS headers. > > That means that on output, for otherwise identical packet, > network_header can point before or after MPLS headers based on what > actions happened to be executed (recirculation, mainly). > > If I'm not misreading the code or missing something, this can't be > right. > > mpls_gso_segment does not care, it resets the network_header anyway. > What about drivers? What is the correct behavior? Answering to myself: it breaks skb_mac_gso_segment. Seems we need to fix key_extract to set network_header to the beginning of MPLS headers. I'll prepare a patch. Jiri
On 9/26/16 11:02 AM, Jiri Benc wrote: > On Mon, 26 Sep 2016 17:56:22 +0200, Jiri Benc wrote: >> After push_mpls, network_header points to the start of MPLS headers. >> Which I understand was the point of this patch. However, push_mpls also >> calls invalidate_flow_key. Meaning that, depending on actions, we may >> end up calling key_extract soon after. And key_extract sets the network >> header *after* the MPLS headers. you know this code better than me, but key_extract pulls the eth header and then sets network header. If MPLS labels are present then it is the labels that the network_header now points to. How did come to the conclusion it is after the labels? >> >> That means that on output, for otherwise identical packet, >> network_header can point before or after MPLS headers based on what >> actions happened to be executed (recirculation, mainly). >> >> If I'm not misreading the code or missing something, this can't be >> right. >> >> mpls_gso_segment does not care, it resets the network_header anyway. >> What about drivers? What is the correct behavior? > > Answering to myself: it breaks skb_mac_gso_segment. Seems we need to > fix key_extract to set network_header to the beginning of MPLS headers. > I'll prepare a patch. > > Jiri >
On Mon, 26 Sep 2016 20:04:06 -0600, David Ahern wrote: > you know this code better than me, but key_extract pulls the eth > header and then sets network header. If MPLS labels are present then > it is the labels that the network_header now points to. How did come > to the conclusion it is after the labels? Look ~100 lines below that, to "if (eth_p_mpls(key->eth.type))". There's a while loop advancing network header. Jiri
On 9/27/16 1:45 AM, Jiri Benc wrote: > On Mon, 26 Sep 2016 20:04:06 -0600, David Ahern wrote: >> you know this code better than me, but key_extract pulls the eth >> header and then sets network header. If MPLS labels are present then >> it is the labels that the network_header now points to. How did come >> to the conclusion it is after the labels? > > Look ~100 lines below that, to "if (eth_p_mpls(key->eth.type))". > There's a while loop advancing network header. got it, thanks. so that block can drop the while loop and just set mpls.top_lse
On Tue, 27 Sep 2016 10:38:41 -0600, David Ahern wrote: > On 9/27/16 1:45 AM, Jiri Benc wrote: > > On Mon, 26 Sep 2016 20:04:06 -0600, David Ahern wrote: > >> you know this code better than me, but key_extract pulls the eth > >> header and then sets network header. If MPLS labels are present then > >> it is the labels that the network_header now points to. How did come > >> to the conclusion it is after the labels? > > > > Look ~100 lines below that, to "if (eth_p_mpls(key->eth.type))". > > There's a while loop advancing network header. > > got it, thanks. so that block can drop the while loop and just set mpls.top_lse I think we still need to traverse the loop to set inner_network_header. Jiri
diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c index 1ecbd7715f6d..0f37b17e3a73 100644 --- a/net/openvswitch/actions.c +++ b/net/openvswitch/actions.c @@ -162,10 +162,16 @@ static int push_mpls(struct sk_buff *skb, struct sw_flow_key *key, if (skb_cow_head(skb, MPLS_HLEN) < 0) return -ENOMEM; + if (!skb->inner_protocol) { + skb_set_inner_network_header(skb, skb->mac_len); + skb_set_inner_protocol(skb, skb->protocol); + } + skb_push(skb, MPLS_HLEN); memmove(skb_mac_header(skb) - MPLS_HLEN, skb_mac_header(skb), skb->mac_len); skb_reset_mac_header(skb); + skb_set_network_header(skb, skb->mac_len); new_mpls_lse = (__be32 *)skb_mpls_header(skb); *new_mpls_lse = mpls->mpls_lse; @@ -173,8 +179,7 @@ static int push_mpls(struct sk_buff *skb, struct sw_flow_key *key, skb_postpush_rcsum(skb, new_mpls_lse, MPLS_HLEN); update_ethertype(skb, eth_hdr(skb), mpls->mpls_ethertype); - if (!skb->inner_protocol) - skb_set_inner_protocol(skb, skb->protocol); + skb->protocol = mpls->mpls_ethertype; invalidate_flow_key(key);