On Thu, Oct 11, 2012 at 11:14:40AM +0900, Simon Horman wrote:
[snip]
> Below is a revised version of the patch which:
>
> * Addresses the concerns you raise above, other
> than the default Ethertype for push_mpls()
> * Incorporates the user-space datapath code as there
> seem to be mutual dependencies between that and
> the other user-space code.
>
> I have pushed this and much more minor changes to the other
> patches in the series to the devel/mpls branch of
> git://github.com/horms/openvswitch.git
> (HEAD = 8b8c4af22ba5c1bddd1dc455fd8a5820cc3e2812)
>
> I would repost the entire series, but as you haven't completed
> your review of this patch I thought it would be best to just
> post an update inline.
In the course of adding code to track the MPLS stack depth,
as suggested by Yamahata-san, I found some problems which
I have noted inline.
>
> ----------------------------------------------------------------
>
> From: Simon Horman <ho...@verge.net.au>
>
> User-Space MPLS actions and matches
>
> This patch implements use-space datapath and non-datapath code
> to match and use the datapath API set out in Leo Alterman's patch
> "user-space datapath: Add basic MPLS support to kernel".
>
> The resulting MPLS implementation supports:
> * Pushing a single MPLS label
> * Poping a single MPLS label
> * Modifying an MPLS lable using set-field or load actions
> that act on the label value, tc and bos bit.
> * There is no support for manipulating the TTL
> this is considered future work.
>
> The single-level push pop limitation is implemented by processing
> push, pop and set-field/load actions in order and discarding information
> that would require multiple levels of push/pop to be supported.
>
> e.g.
> push,push -> the first push is discarded
> pop,pop -> the first pop is discarded
>
> This patch is based heavily on work by Ravi K.
>
> Cc: Isaku Yamahata <yamah...@valinux.co.jp>
> Cc: Ravi K <rke...@gmail.com>
> Signed-off-by: Simon Horman <ho...@verge.net.au>
[snip]
> diff --git a/lib/flow.c b/lib/flow.c
> index 76d2340..5291e19 100644
> --- a/lib/flow.c
> +++ b/lib/flow.c
> @@ -93,6 +93,28 @@ pull_icmpv6(struct ofpbuf *packet)
> }
>
> static void
> +parse_remaining_mpls(struct ofpbuf *b)
> +{
> + /* Proceed with parsing remaining MPLS headers. */
> + struct mpls_hdr *mh;
> + while ((mh = ofpbuf_try_pull(b, sizeof *mh)) &&
> + (mh->mpls_lse & htonl(MPLS_BOS_MASK))) {
> + ;
> + }
> +}
I believe the logic above is (still!) wrong. I think that the
BoS check should be inverted yielding.
static void
parse_remaining_mpls(struct ofpbuf *b, struct flow *flow)
{
/* Proceed with parsing remaining MPLS headers. */
struct mpls_hdr *mh;
while ((mh = ofpbuf_try_pull(b, sizeof *mh)) &&
!(mh->mpls_lse & htonl(MPLS_BOS_MASK))) {
flow->mpls_depth++;
}
}
[snip]
> diff --git a/lib/packets.c b/lib/packets.c
> index 16f4fe6..0b85cf9 100644
> --- a/lib/packets.c
> +++ b/lib/packets.c
[snip]
> @@ -211,6 +212,361 @@ eth_pop_vlan(struct ofpbuf *packet)
[snip]
The dec_mpls_ttl(), copy_mpls_ttl_in() and copy_mpls_ttl_out() functions
below are not used by this patchset and I will remove them from the next
revision of this patch.
> +
> +/* Decrement MPLS TTL from the packet.
> + * 'packet->l2_5' must initially point to 'packet''s MPLS Label stack. */
> +void
> +dec_mpls_ttl(struct ofpbuf *packet, uint8_t new_ttl)
> +{
> + ovs_be16 eth_type = htons(0);
> + struct eth_header *eh = packet->data;
> + struct mpls_hdr *mh = packet->l2_5;
> +
> + if (packet->size < sizeof *eh) {
> + return;
> + }
> +
> + /* Packet type should be mpls to decrement ttl. */
> + eth_type = get_ethertype(packet);
> +
> + if (eth_type == htons(ETH_TYPE_MPLS) ||
> + eth_type == htons(ETH_TYPE_MPLS_MCAST)) {
> +
> + /* Update decremented ttl into mpls header. */
> + set_mpls_lse_ttl(&mh->mpls_lse, htonl(new_ttl << MPLS_TTL_SHIFT));
> + }
> +}
> +
> +/* Copy MPLS TTL from the packet either ipv4/ipv6.
> + * 'packet->l2_5' must initially point to 'packet''s MPLS Label stack. */
> +void
> +copy_mpls_ttl_in(struct ofpbuf *packet, uint8_t new_ttl)
> +{
> + struct eth_header *eh = packet->data;
> + struct mpls_hdr *mh = packet->l2_5;
> + struct ip_header *ih = packet->l3;
> + struct ip6_hdr *ih6 = packet->l3;
> + ovs_be16 eth_type = htons(0);
> + size_t hdr_size = sizeof *eh + sizeof *mh + sizeof *ih;
> +
> + if (packet->size < hdr_size) {
> + return;
> + }
> +
> + /* Packet type should be mpls to copy ttl to l3. */
> + eth_type = get_ethertype(packet);
> + if (eth_type == htons(ETH_TYPE_MPLS) ||
> + eth_type == htons(ETH_TYPE_MPLS_MCAST)) {
> +
> + /* If bottom of the stack handle IP checksum. */
> + if (mh->mpls_lse & htonl(MPLS_BOS_MASK)) {
> + if (IP_VER(ih->ip_ihl_ver) == IP_VERSION) {
> + /* Change the ip checksum. */
> + uint8_t *field = &ih->ip_ttl;
> + ih->ip_csum = recalc_csum16(ih->ip_csum,
> + htons(*field << 8), htons(new_ttl << 8));
> + ih->ip_ttl = new_ttl;
> + } else if (IP6_VER(ih6->ip6_vfc) == IP6_VERSION) {
> + ih6->ip6_hlim = new_ttl;
> + }
> + } else {
> + struct mpls_hdr *mh2;
> + mh2 = (struct mpls_hdr *)((char *) packet->l2_5 + sizeof *mh);
> + set_mpls_lse_ttl(&mh2->mpls_lse, htonl(new_ttl <<
> MPLS_TTL_SHIFT));
> + }
> + }
> +}
> +
> +/* Copy MPLS TTL to the packet layer3 only ipv4/ipv6.
> + * 'packet->l2_5' must initially point to 'packet''s MPLS Label stack. */
> +void
> +copy_mpls_ttl_out(struct ofpbuf *packet, uint8_t new_ttl)
> +{
> + struct eth_header *eh = packet->data;
> + struct mpls_hdr *mh = packet->l2_5;
> + struct ip_header *ih = packet->l3;
> + struct ip6_hdr *ih6 = packet->l3;
> + ovs_be16 eth_type = htons(0);
> + size_t hdr_size = sizeof *eh + sizeof *mh + sizeof *ih;
> +
> + /* TTL sent from ofproto-dpif.c is not the correct one,
> + * hence ignore it. */
> + if (packet->size < hdr_size) {
> + return;
> + }
> +
> + /* Packet type should me mpls to copy ttl from l3. */
> + eth_type = get_ethertype(packet);
> + if (eth_type == htons(ETH_TYPE_MPLS) ||
> + eth_type == htons(ETH_TYPE_MPLS_MCAST)) {
> +
> + /* If bottom of the stack copy from l3. */
> + if (mh->mpls_lse & htonl(MPLS_BOS_MASK)) {
> + uint8_t nh_ttl;
> + /* Get ipv4 or ipv6 or default ttl. */
> + if (IP_VER(ih->ip_ihl_ver) == IP_VERSION) {
> + nh_ttl = ih->ip_ttl;
> + } else if (IP6_VER(ih6->ip6_vfc) == IP6_VERSION) {
> + nh_ttl = ih6->ip6_hlim;
> + } else {
> + nh_ttl = 64; /* Default ttl for non-IP. */
> + }
> + set_mpls_lse_ttl(&mh->mpls_lse, htonl(nh_ttl << MPLS_TTL_SHIFT));
> + } else {
> + struct mpls_hdr *mh2;
> + mh2 = (struct mpls_hdr *)((char *) packet->l2_5 + sizeof *mh);
> + new_ttl = mpls_lse_to_ttl(mh2->mpls_lse);
> + set_mpls_lse_ttl(&mh->mpls_lse, htonl(new_ttl <<
> MPLS_TTL_SHIFT));
> + }
> + }
> +}
[snip]
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
