Hi William,
I think the question here is what is the main target functionality?
3-5/G VNF switching?
Note that in many cases there is RSS issue here because GTP between
two functions will have the exact same outer header, there is no
port entropy. All the packets between two peers will get to the same
core (per direction, according to RSS symmetric cfg).
See inline.
>-----Original Message-----
>From: William Tu <u9012...@gmail.com>
>Sent: Wednesday, December 11, 2019 8:20 PM
>To: Roni Bar Yanai <ron...@mellanox.com>
>Cc: d...@openvswitch.org
>Subject: Re: [ovs-dev] [PATCHv4] userspace: Add GTP-U support.
>
>On Sun, Dec 08, 2019 at 08:11:19AM +0000, Roni Bar Yanai wrote:
>> Hi William,
>>
>> GTP-U header size is not constant, you *must* take into account the
>> flags, mainly the sequence. The use of sequence in GTP-U is optional
>> but some devices do use it. see from 3GPP definition:
>> "For PGW, SGW and eNodeB the usage of sequence numbers in G-PDUs is
>> optional, but if GTP-U protocol entities in these nodes are relaying
>> G-PDUs to other nodes, they shall relay the sequence numbers as well.
>> An RNC, SGSN or GGSN shall reorder out of sequence T-PDUs when in
>sequence delivery is required. This is optional"
>>
>> The assumption that GTP-U is only data is not correct, you have some
>> signaling for example END MARKER (like you wrote). This message is
>> sent when there is a mobility between cells, and the message contains
>> a TEID and indicates that last packet sent from the origin cell, to
>> prevent packet re-order as handover might have packets on the fly. So
>> I would expected that END MARKER will do the exact same forwarding as the
>GTP-U data.
>>
>> see inline
>>
>> >diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c index
>> >a78972888e33..7ae554c87964 100644
>> >--- a/lib/netdev-native-tnl.c
>> >+++ b/lib/netdev-native-tnl.c
>> >@@ -55,6 +55,9 @@ static struct vlog_rate_limit err_rl =
>> >VLOG_RATE_LIMIT_INIT(60, 5);
>> > #define GENEVE_BASE_HLEN (sizeof(struct udp_header) + \
>> > sizeof(struct genevehdr))
>> >
>> >+#define GTPU_HLEN (sizeof(struct udp_header) + \
>> >+ sizeof(struct gtpuhdr))
>> This is the base header length, if S or N-PDU is on you have to add
>> 4-bytes
>> >+
>> > uint16_t tnl_udp_port_min = 32768;
>> > uint16_t tnl_udp_port_max = 61000;
>> >
>> >@@ -708,6 +711,88 @@ netdev_erspan_build_header(const struct netdev
>> >*netdev, }
>> >
>> > struct dp_packet *
>> >+netdev_gtpu_pop_header(struct dp_packet *packet) {
>> >+ struct pkt_metadata *md = &packet->md;
>> >+ struct flow_tnl *tnl = &md->tunnel;
>> >+ struct gtpuhdr *gtph;
>> >+ unsigned int hlen;
>> >+
>> >+ ovs_assert(packet->l3_ofs > 0);
>> >+ ovs_assert(packet->l4_ofs > 0);
>> >+
>> >+ pkt_metadata_init_tnl(md);
>> >+ if (GTPU_HLEN > dp_packet_l4_size(packet)) {
>> >+ goto err;
>> >+ }
>> >+
>> >+ gtph = udp_extract_tnl_md(packet, tnl, &hlen);
>> >+ if (!gtph) {
>> >+ goto err;
>> >+ }
>> >+
>> >+ if (gtph->md.flags != GTPU_FLAGS_DEFAULT) {
>> >+ VLOG_WARN_RL(&err_rl, "GTP-U not supported");
>> >+ goto err;
>> >+ }
>> >+
>> >+ tnl->gtpu_flags = gtph->md.flags;
>> >+ tnl->gtpu_msgtype = gtph->md.msgtype;
>> >+ tnl->tun_id = htonll(ntohl(get_16aligned_be32(>ph->teid)));
>> >+
>> >+ if (tnl->gtpu_msgtype == GTPU_MSGTYPE_GPDU) {
>> >+ struct ip_header *ip;
>> >+
>> >+ ip = (struct ip_header *)(gtph + 1);
>> No always correct, should be additional 4 bytes on sequence, n-pdu
>> >+ if (IP_VER(ip->ip_ihl_ver) == 4) {
>> >+ packet->packet_type = htonl(PT_IPV4);
>> >+ } else if (IP_VER(ip->ip_ihl_ver) == 6) {
>> >+ packet->packet_type = htonl(PT_IPV6);
>> >+ } else {
>> >+ VLOG_WARN_RL(&err_rl, "GTP-U: Receive non-IP packet");
>> >+ }
>> >+ } else {
>> >+ /* non GPDU GTP-U messages, ex: echo request, end marker. */
>> >+ packet->packet_type = htonl(PT_GTPU_MSG);
>> >+ }
>> >+
>> >+ dp_packet_reset_packet(packet, hlen + GTPU_HLEN);
>> Why do you want to reset on control such as end marker?
>
>Right, for non-GPDU, we shouldn't reset.
>I will keep the original packet, no pop.
>
>> >+
>> >+ return packet;
>> >+
>> >+err:
>> >+ dp_packet_delete(packet);
>> >+ return NULL;
>> >+}
>> >+
>> >+int
>> >+netdev_gtpu_build_header(const struct netdev *netdev,
>> >+ struct ovs_action_push_tnl *data,
>> >+ const struct netdev_tnl_build_header_params
>> >+*params) {
>> >+ struct netdev_vport *dev = netdev_vport_cast(netdev);
>> >+ struct netdev_tunnel_config *tnl_cfg;
>> >+ struct gtpuhdr *gtph;
>> >+
>> >+ ovs_mutex_lock(&dev->mutex);
>> >+ tnl_cfg = &dev->tnl_cfg;
>> >+ gtph = udp_build_header(tnl_cfg, data, params);
>> >+ ovs_mutex_unlock(&dev->mutex);
>> >+
>> >+ /* Set to default if not set in flow. */
>> >+ gtph->md.flags = params->flow->tunnel.gtpu_flags ? :
>GTPU_FLAGS_DEFAULT;
>> >+ gtph->md.msgtype = params->flow->tunnel.gtpu_msgtype ? :
>> >+ GTPU_MSGTYPE_GPDU;
>> >+ put_16aligned_be32(>ph->teid,
>> >+ htonl(ntohll(params->flow->tunnel.tun_id)));
>> >+
>> >+ data->header_len += sizeof *gtph;
>> >+ data->tnl_type = OVS_VPORT_TYPE_GTPU;
>> Note that in some cases, application might need to replace TEID , if
>> you have tnl_push you need to preserve the origin sequence.
>What use case are you talking about here?
>Are you saying that OVS receive a GTP-U packet, decap it, preserve its sequence
>number, and send to anther GTPU tunnel?
>
>For replacing TEID, change the tun_id will do the work.
What is the use case you target here?
I guess for switching this part can be skipped it is more VNF functionality.
There are points on data path like p-gw that tunnel TEID changes because the
allocation is done by the two peers of the connection and it depends on their
available tunnels. So packet will arrive on one tunnel and leave for next
function
with a different tunnel. If this is the use case and GTP header has a sequence,
the
sequence should remain the same.
>
>Thanks for your feedback!
>William
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
