diff mbox

[ovs-dev] tunnel: Fix deletion of datapath tunnel ports in case of reconfiguration

Message ID DB5PR07MB14961F5D880EB3E671A9F4EC9FA00@DB5PR07MB1496.eurprd07.prod.outlook.com
State Not Applicable
Headers show

Commit Message

Balazs Nemeth July 17, 2017, 12:37 p.m. UTC 
There is an issue in OVS with tunnel deletion during the
reconfiguration of OF tunnels. If the dst_port value is changed, the
old tunnel map entry will not be deleted, because the tp_port
argument of tnl_port_map_delete() has the new dst_port setting, hence
the tunnel cannot be found in the list of tnl_port structures.

The patch corrects this mechanism by adding a new argument,
'old_odp_port' to tnl_port_reconfigure(). This value is used to
identify the datapath tunnel port which is being reconfigured. In
connection with this fix, to unify the tunnel port map handling,
odp_port value is used to search the proper port to insert and delete
tunnel map entries as well. This variable can be used instead of
tp_port, as it is unique for all datapath tunnel ports, and there is
no need to reach dst_port from netdev_tunnel_config structure.

This patch also adds a printout to check the reference counter of
a tnl_port structure in tnl-port.c. Extending OVS unit test cases to
have ref_cnt values in the expected dump. Adding new test cases to
check if packet receiving is still working in the case of OF tunnel
port deletion. Adding new test cases to check the reference counter
in case of OF tunnel deletion or reconfiguration.

Signed-off-by: Balazs Nemeth <balazs.nemeth@ericsson.com>
Signed-off-by: Jan Scheurich <jan.scheurich@ericsson.com>
Co-authored-by: Jan Scheurich <jan.scheurich@ericsson.com>
---
lib/tnl-ports.c               |  9 +++++----
lib/tnl-ports.h               |  4 ++--
ofproto/ofproto-dpif.c        |  8 +++++---
ofproto/tunnel.c              | 37 ++++++++++++++++++++-----------------
ofproto/tunnel.h              |  7 ++++---
tests/tunnel-push-pop-ipv6.at | 33 ++++++++++++++++++++++++++++++---
tests/tunnel-push-pop.at      | 35 ++++++++++++++++++++++++++++++++---
7 files changed, 98 insertions(+), 35 deletions(-)

--
1.9.1

Comments

Ben Pfaff Aug. 2, 2017, 8:48 p.m. UTC | #1 
On Mon, Jul 17, 2017 at 12:37:07PM +0000, Balazs Nemeth wrote:
> There is an issue in OVS with tunnel deletion during the
> reconfiguration of OF tunnels. If the dst_port value is changed, the
> old tunnel map entry will not be deleted, because the tp_port
> argument of tnl_port_map_delete() has the new dst_port setting, hence
> the tunnel cannot be found in the list of tnl_port structures.
> 
> The patch corrects this mechanism by adding a new argument,
> 'old_odp_port' to tnl_port_reconfigure(). This value is used to
> identify the datapath tunnel port which is being reconfigured. In
> connection with this fix, to unify the tunnel port map handling,
> odp_port value is used to search the proper port to insert and delete
> tunnel map entries as well. This variable can be used instead of
> tp_port, as it is unique for all datapath tunnel ports, and there is
> no need to reach dst_port from netdev_tunnel_config structure.
> 
> This patch also adds a printout to check the reference counter of
> a tnl_port structure in tnl-port.c. Extending OVS unit test cases to
> have ref_cnt values in the expected dump. Adding new test cases to
> check if packet receiving is still working in the case of OF tunnel
> port deletion. Adding new test cases to check the reference counter
> in case of OF tunnel deletion or reconfiguration.
> 
> Signed-off-by: Balazs Nemeth <balazs.nemeth@ericsson.com>
> Signed-off-by: Jan Scheurich <jan.scheurich@ericsson.com>
> Co-authored-by: Jan Scheurich <jan.scheurich@ericsson.com>

Thanks for finding and fixing the problem.  This sounds like it fixes a
serious problem in the tunnel implementation.  I'd like to get it in.

There are a few issues.  First, the patch is whitespace damaged and
cannot be applied.  The first for this is usually to use "git
send-email" to send the patch or, if this is difficult, to post it to
Github as a pull request.

Second, is this related to the following patch?  Does either one mean
that the other is not necessary or should change?

    commit e7c9ff0ed108d73f4aca2d5e54fa1e671e935e20
    Author: zhaozhanxu <zhaozhanxu@163.com>
    Date:   Mon Jun 26 18:29:22 2017 +0800

        tnl-ports: Fix loss of tunneling upon removal of a single tunnel port.

        When OVS had multiple tunnel ports of a single kind, and any one of them
        was removed, the remaining ports could no longer receive traffic.  This
        fixes the problem.

        Signed-off-by: zhaozhanxu <zhaozhanxu@163.com>
        Signed-off-by: Ben Pfaff <blp@ovn.org>

Finally, we are branching for the 2.8 release soon, but that does not
affect the status of this patch because bug fixes are welcomed at any
stage of development.

Thanks,

Ben.
Balazs Nemeth Aug. 3, 2017, 1:27 p.m. UTC | #2 
Thanks for the notice, I will upload v2 in a few minutes without whitespace errors.

Yes, this patch is related to commit e7c9ff0. Both are fixing an issue with tunnel port handling, but I think the two issue is logically independent. One is with deletion, the other one is with changing dst_port value during reconfiguration.
I tried to upload a patch earlier, where I was fixing both issue, then I noticed that the first one is fixed in the meantime by zhaozhanxu. I sent a note to the list in:
https://mail.openvswitch.org/pipermail/ovs-dev/2017-July/335565.html 
I think both patch is necessary, first one should not be changed. I would be happy if it could fit to 2.8, also I think backporting to branch-2.7, 2.6, .. would be useful.

Balazs

-----Original Message-----
From: Ben Pfaff [mailto:blp@ovn.org] 
Sent: 02 August 2017 22:48
To: Balazs Nemeth <balazs.nemeth@ericsson.com>
Cc: ovs-dev@openvswitch.org
Subject: Re: [ovs-dev] [PATCH] tunnel: Fix deletion of datapath tunnel ports in case of reconfiguration

On Mon, Jul 17, 2017 at 12:37:07PM +0000, Balazs Nemeth wrote:
> There is an issue in OVS with tunnel deletion during the
> reconfiguration of OF tunnels. If the dst_port value is changed, the
> old tunnel map entry will not be deleted, because the tp_port
> argument of tnl_port_map_delete() has the new dst_port setting, hence
> the tunnel cannot be found in the list of tnl_port structures.
> 
> The patch corrects this mechanism by adding a new argument,
> 'old_odp_port' to tnl_port_reconfigure(). This value is used to
> identify the datapath tunnel port which is being reconfigured. In
> connection with this fix, to unify the tunnel port map handling,
> odp_port value is used to search the proper port to insert and delete
> tunnel map entries as well. This variable can be used instead of
> tp_port, as it is unique for all datapath tunnel ports, and there is
> no need to reach dst_port from netdev_tunnel_config structure.
> 
> This patch also adds a printout to check the reference counter of
> a tnl_port structure in tnl-port.c. Extending OVS unit test cases to
> have ref_cnt values in the expected dump. Adding new test cases to
> check if packet receiving is still working in the case of OF tunnel
> port deletion. Adding new test cases to check the reference counter
> in case of OF tunnel deletion or reconfiguration.
> 
> Signed-off-by: Balazs Nemeth <balazs.nemeth@ericsson.com>
> Signed-off-by: Jan Scheurich <jan.scheurich@ericsson.com>
> Co-authored-by: Jan Scheurich <jan.scheurich@ericsson.com>

Thanks for finding and fixing the problem.  This sounds like it fixes a
serious problem in the tunnel implementation.  I'd like to get it in.

There are a few issues.  First, the patch is whitespace damaged and
cannot be applied.  The first for this is usually to use "git
send-email" to send the patch or, if this is difficult, to post it to
Github as a pull request.

Second, is this related to the following patch?  Does either one mean
that the other is not necessary or should change?

    commit e7c9ff0ed108d73f4aca2d5e54fa1e671e935e20
    Author: zhaozhanxu <zhaozhanxu@163.com>
    Date:   Mon Jun 26 18:29:22 2017 +0800

        tnl-ports: Fix loss of tunneling upon removal of a single tunnel port.

        When OVS had multiple tunnel ports of a single kind, and any one of them
        was removed, the remaining ports could no longer receive traffic.  This
        fixes the problem.

        Signed-off-by: zhaozhanxu <zhaozhanxu@163.com>
        Signed-off-by: Ben Pfaff <blp@ovn.org>

Finally, we are branching for the 2.8 release soon, but that does not
affect the status of this patch because bug fixes are welcomed at any
stage of development.

Thanks,

Ben.
diff mbox

Patch

diff --git a/lib/tnl-ports.c b/lib/tnl-ports.c
index 70f6c3b..a371847 100644
--- a/lib/tnl-ports.c
+++ b/lib/tnl-ports.c
@@ -195,7 +195,7 @@  tnl_port_map_insert(odp_port_t port, ovs_be16 tp_port,

     ovs_mutex_lock(&mutex);
     LIST_FOR_EACH(p, node, &port_list) {
-        if (tp_port == p->tp_port && p->nw_proto == nw_proto) {
+        if (p->port == port && p->nw_proto == nw_proto) {
             ovs_refcount_ref(&p->ref_cnt);
             goto out;
         }
@@ -255,7 +255,7 @@  ipdev_map_delete(struct ip_device *ip_dev, ovs_be16 tp_port, uint8_t nw_proto)
}

void
-tnl_port_map_delete(ovs_be16 tp_port, const char type[])
+tnl_port_map_delete(odp_port_t port, const char type[])
{
     struct tnl_port *p, *next;
     struct ip_device *ip_dev;
@@ -265,7 +265,7 @@  tnl_port_map_delete(ovs_be16 tp_port, const char type[])

     ovs_mutex_lock(&mutex);
     LIST_FOR_EACH_SAFE(p, next, node, &port_list) {
-        if (p->tp_port == tp_port && p->nw_proto == nw_proto &&
+        if (p->port == port && p->nw_proto == nw_proto &&
                     ovs_refcount_unref_relaxed(&p->ref_cnt) == 1) {
             ovs_list_remove(&p->node);
             LIST_FOR_EACH(ip_dev, node, &addr_list) {
@@ -348,7 +348,8 @@  tnl_port_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
     }

     LIST_FOR_EACH(p, node, &port_list) {
-        ds_put_format(&ds, "%s (%"PRIu32")\n", p->dev_name, p->port);
+        ds_put_format(&ds, "%s (%"PRIu32") ref_cnt=%u\n", p->dev_name, p->port,
+                      ovs_refcount_read(&p->ref_cnt));
     }

out:
diff --git a/lib/tnl-ports.h b/lib/tnl-ports.h
index 58b048a..61ca0f8 100644
--- a/lib/tnl-ports.h
+++ b/lib/tnl-ports.h
@@ -26,10 +26,10 @@ 

odp_port_t tnl_port_map_lookup(struct flow *flow, struct flow_wildcards *wc);

-void tnl_port_map_insert(odp_port_t port, ovs_be16 udp_port,
+void tnl_port_map_insert(odp_port_t, ovs_be16 tp_port,
                          const char dev_name[], const char type[]);

-void tnl_port_map_delete(ovs_be16 udp_port, const char type[]);
+void tnl_port_map_delete(odp_port_t, const char type[]);
void tnl_port_map_insert_ipdev(const char dev[]);
void tnl_port_map_delete_ipdev(const char dev[]);
void tnl_port_map_run(void);
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 7cf1a40..2ada049 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -378,6 +378,7 @@  type_run(const char *type)
             HMAP_FOR_EACH (iter, up.hmap_node, &ofproto->up.ports) {
                 char namebuf[NETDEV_VPORT_NAME_BUFSIZE];
                 const char *dp_port;
+                odp_port_t old_odp_port;

                 if (!iter->is_tunnel) {
                     continue;
@@ -385,6 +386,7 @@  type_run(const char *type)

                 dp_port = netdev_vport_get_dpif_port(iter->up.netdev,
                                                      namebuf, sizeof namebuf);
+                old_odp_port = iter->odp_port;
                 node = simap_find(&tmp_backers, dp_port);
                 if (node) {
                     simap_put(&backer->tnl_backers, dp_port, node->data);
@@ -406,7 +408,7 @@  type_run(const char *type)

                 iter->odp_port = node ? u32_to_odp(node->data) : ODPP_NONE;
                 if (tnl_port_reconfigure(iter, iter->up.netdev,
-                                         iter->odp_port,
+                                         iter->odp_port, old_odp_port,
                                          ovs_native_tunneling_is_on(ofproto), dp_port)) {
                     backer->need_revalidate = REV_RECONFIGURE;
                 }
@@ -1937,7 +1939,7 @@  port_destruct(struct ofport *port_, bool del)
        dpif_ipfix_del_tunnel_port(ofproto->ipfix, port->odp_port);
     }

-    tnl_port_del(port);
+    tnl_port_del(port, port->odp_port);
     sset_find_and_delete(&ofproto->ports, devname);
     sset_find_and_delete(&ofproto->ghost_ports, devname);
     bundle_remove(port_);
@@ -1993,7 +1995,7 @@  port_modified(struct ofport *port_)
     if (port->is_tunnel) {
         struct ofproto_dpif *ofproto = ofproto_dpif_cast(port->up.ofproto);

-        if (tnl_port_reconfigure(port, netdev, port->odp_port,
+        if (tnl_port_reconfigure(port, netdev, port->odp_port, port->odp_port,
                                  ovs_native_tunneling_is_on(ofproto),
                                  dp_port_name)) {
             ofproto->backer->need_revalidate = REV_RECONFIGURE;
diff --git a/ofproto/tunnel.c b/ofproto/tunnel.c
index c6856a0..1676f4d 100644
--- a/ofproto/tunnel.c
+++ b/ofproto/tunnel.c
@@ -126,7 +126,8 @@  static void tnl_port_mod_log(const struct tnl_port *, const char *action)
     OVS_REQ_RDLOCK(rwlock);
static const char *tnl_port_get_name(const struct tnl_port *)
     OVS_REQ_RDLOCK(rwlock);
-static void tnl_port_del__(const struct ofport_dpif *) OVS_REQ_WRLOCK(rwlock);
+static void tnl_port_del__(const struct ofport_dpif *, odp_port_t)
+    OVS_REQ_WRLOCK(rwlock);

void
ofproto_tunnel_init(void)
@@ -221,13 +222,15 @@  tnl_port_add(const struct ofport_dpif *ofport, const struct netdev *netdev,
}

/* Checks if the tunnel represented by 'ofport' reconfiguration due to changes
- * in its netdev_tunnel_config.  If it does, returns true. Otherwise, returns
- * false.  'ofport' and 'odp_port' should be the same as would be passed to
- * tnl_port_add(). */
+ * in its netdev_tunnel_config. If it does, returns true. Otherwise, returns
+ * false. 'new_odp_port' should be the port number coming from 'ofport' that
+ * is passed to tnl_port_add__(). 'old_odp_port' should be the port number
+ * that is passed to tnl_port_del__(). */
bool
tnl_port_reconfigure(const struct ofport_dpif *ofport,
-                     const struct netdev *netdev, odp_port_t odp_port,
-                     bool native_tnl, const char name[])
+                     const struct netdev *netdev, odp_port_t new_odp_port,
+                     odp_port_t old_odp_port, bool native_tnl,
+                     const char name[])
     OVS_EXCLUDED(rwlock)
{
     struct tnl_port *tnl_port;
@@ -236,14 +239,14 @@  tnl_port_reconfigure(const struct ofport_dpif *ofport,
     fat_rwlock_wrlock(&rwlock);
     tnl_port = tnl_find_ofport(ofport);
     if (!tnl_port) {
-        changed = tnl_port_add__(ofport, netdev, odp_port, false, native_tnl,
-                                 name);
+        changed = tnl_port_add__(ofport, netdev, new_odp_port, false,
+                                 native_tnl, name);
     } else if (tnl_port->netdev != netdev
-               || tnl_port->match.odp_port != odp_port
+               || tnl_port->match.odp_port != new_odp_port
                || tnl_port->change_seq != netdev_get_change_seq(tnl_port->netdev)) {
         VLOG_DBG("reconfiguring %s", tnl_port_get_name(tnl_port));
-        tnl_port_del__(ofport);
-        tnl_port_add__(ofport, netdev, odp_port, true, native_tnl, name);
+        tnl_port_del__(ofport, old_odp_port);
+        tnl_port_add__(ofport, netdev, new_odp_port, true, native_tnl, name);
         changed = true;
     }
     fat_rwlock_unlock(&rwlock);
@@ -251,7 +254,8 @@  tnl_port_reconfigure(const struct ofport_dpif *ofport,
}

static void
-tnl_port_del__(const struct ofport_dpif *ofport) OVS_REQ_WRLOCK(rwlock)
+tnl_port_del__(const struct ofport_dpif *ofport, odp_port_t odp_port)
+    OVS_REQ_WRLOCK(rwlock)
{
     struct tnl_port *tnl_port;

@@ -261,11 +265,9 @@  tnl_port_del__(const struct ofport_dpif *ofport) OVS_REQ_WRLOCK(rwlock)

     tnl_port = tnl_find_ofport(ofport);
     if (tnl_port) {
-        const struct netdev_tunnel_config *cfg =
-            netdev_get_tunnel_config(tnl_port->netdev);
         struct hmap **map;

-        tnl_port_map_delete(cfg->dst_port, netdev_get_type(tnl_port->netdev));
+        tnl_port_map_delete(odp_port, netdev_get_type(tnl_port->netdev));
         tnl_port_mod_log(tnl_port, "removing");
         map = tnl_match_map(&tnl_port->match);
         hmap_remove(*map, &tnl_port->match_node);
@@ -282,10 +284,11 @@  tnl_port_del__(const struct ofport_dpif *ofport) OVS_REQ_WRLOCK(rwlock)

/* Removes 'ofport' from the module. */
void
-tnl_port_del(const struct ofport_dpif *ofport) OVS_EXCLUDED(rwlock)
+tnl_port_del(const struct ofport_dpif *ofport, odp_port_t odp_port)
+    OVS_EXCLUDED(rwlock)
{
     fat_rwlock_wrlock(&rwlock);
-    tnl_port_del__(ofport);
+    tnl_port_del__(ofport, odp_port);
     fat_rwlock_unlock(&rwlock);
}

diff --git a/ofproto/tunnel.h b/ofproto/tunnel.h
index b0ec67c..47d3dd5 100644
--- a/ofproto/tunnel.h
+++ b/ofproto/tunnel.h
@@ -32,11 +32,12 @@  struct netdev_tnl_build_header_params;

void ofproto_tunnel_init(void);
bool tnl_port_reconfigure(const struct ofport_dpif *, const struct netdev *,
-                          odp_port_t, bool native_tnl, const char name[]);
+                          odp_port_t new_odp_port, odp_port_t old_odp_port,
+                          bool native_tnl, const char name[]);

int tnl_port_add(const struct ofport_dpif *, const struct netdev *,
-                 odp_port_t odp_port, bool native_tnl, const char name[]);
-void tnl_port_del(const struct ofport_dpif *);
+                 odp_port_t, bool native_tnl, const char name[]);
+void tnl_port_del(const struct ofport_dpif *, odp_port_t);

const struct ofport_dpif *tnl_port_receive(const struct flow *);
void tnl_wc_init(struct flow *, struct flow_wildcards *);
diff --git a/tests/tunnel-push-pop-ipv6.at b/tests/tunnel-push-pop-ipv6.at
index 9ff7c89..8f51c06 100644
--- a/tests/tunnel-push-pop-ipv6.at
+++ b/tests/tunnel-push-pop-ipv6.at
@@ -66,9 +66,9 @@  AT_CHECK([ovs-appctl tnl/arp/show | tail -n+3 | sort], [0], [dnl

AT_CHECK([ovs-appctl tnl/ports/show |sort], [0], [dnl
Listening ports:
-genev_sys_6081 (6081)
-gre_sys (3)
-vxlan_sys_4789 (4789)
+genev_sys_6081 (6081) ref_cnt=1
+gre_sys (3) ref_cnt=2
+vxlan_sys_4789 (4789) ref_cnt=2
])

dnl Check VXLAN tunnel pop
@@ -167,5 +167,32 @@  AT_CHECK([ovs-appctl dpif/dump-flows int-br | grep 'in_port(6081)'], [0], [dnl
tunnel(tun_id=0x7b,ipv6_src=2001:cafe::92,ipv6_dst=2001:cafe::88,geneve({class=0xffff,type=0x80,len=4,0xa/0xf}{class=0xffff,type=0,len=4}),flags(-df-csum+key)),recirc_id(0),in_port(6081),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no), packets:0, bytes:0, used:never, actions:userspace(pid=0,slow_path(controller))
])

+ovs-appctl time/warp 10000
+
+AT_CHECK([ovs-vsctl del-port int-br t3 \
+                    -- set Interface t1 type=vxlan \
+                    -- set Interface t2 options:dst_port=4790 \
+                       ], [0])
+
+dnl Check tunnel lookup entries after deleting/reconfiguring some ports
+AT_CHECK([ovs-appctl tnl/ports/show |sort], [0], [dnl
+Listening ports:
+genev_sys_6081 (6081) ref_cnt=1
+gre_sys (3) ref_cnt=1
+vxlan_sys_4789 (4789) ref_cnt=1
+vxlan_sys_4790 (4790) ref_cnt=1
+])
+
+AT_CHECK([ovs-vsctl del-port int-br t1 \
+                    -- del-port int-br t2 \
+                    -- del-port int-br t4 \
+                    -- del-port int-br t5 \
+                       ], [0])
+
+dnl Check tunnel lookup entries after deleting all remaining tunnel ports
+AT_CHECK([ovs-appctl tnl/ports/show |sort], [0], [dnl
+Listening ports:
+])
+
OVS_VSWITCHD_STOP
AT_CLEANUP
diff --git a/tests/tunnel-push-pop.at b/tests/tunnel-push-pop.at
index c376e71..5f2f172 100644
--- a/tests/tunnel-push-pop.at
+++ b/tests/tunnel-push-pop.at
@@ -80,9 +80,9 @@  AT_CHECK([ovs-appctl tnl/neigh/show | tail -n+3 | sort], [0], [dnl

AT_CHECK([ovs-appctl tnl/ports/show |sort], [0], [dnl
Listening ports:
-genev_sys_6081 (6081)
-gre_sys (3)
-vxlan_sys_4789 (4789)
+genev_sys_6081 (6081) ref_cnt=2
+gre_sys (3) ref_cnt=2
+vxlan_sys_4789 (4789) ref_cnt=3
])

dnl Check VXLAN tunnel pop
@@ -218,6 +218,35 @@  AT_CHECK([ovs-appctl dpif/dump-flows int-br | grep 'in_port(6081)'], [0], [dnl
tunnel(tun_id=0x7b,src=1.1.2.92,dst=1.1.2.88,geneve({class=0xffff,type=0x80,len=4,0xa/0xf}{class=0xffff,type=0,len=4}),flags(-df-csum+key)),recirc_id(0),in_port(6081),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no), packets:0, bytes:0, used:never, actions:userspace(pid=0,slow_path(controller))
])

+ovs-appctl time/warp 10000
+
+AT_CHECK([ovs-vsctl del-port int-br t3 \
+                    -- del-port int-br t5 \
+                    -- set Interface t1 type=vxlan \
+                    -- set Interface t2 options:dst_port=4790 \
+                       ], [0])
+
+dnl Check tunnel lookup entries after deleting/reconfiguring some ports
+AT_CHECK([ovs-appctl tnl/ports/show |sort], [0], [dnl
+Listening ports:
+genev_sys_6081 (6081) ref_cnt=1
+gre_sys (3) ref_cnt=1
+vxlan_sys_4789 (4789) ref_cnt=2
+vxlan_sys_4790 (4790) ref_cnt=1
+])
+
+AT_CHECK([ovs-vsctl del-port int-br t1 \
+                    -- del-port int-br t2 \
+                    -- del-port int-br t4 \
+                    -- del-port int-br t6 \
+                    -- del-port int-br t7 \
+                       ], [0])
+
+dnl Check tunnel lookup entries after deleting all remaining tunnel ports
+AT_CHECK([ovs-appctl tnl/ports/show |sort], [0], [dnl
+Listening ports:
+])
+
OVS_VSWITCHD_STOP
AT_CLEANUP