fail io_uring zc with sockets not supporting it
Subsystems (5)
cover fail io_uring zc with sockets not supporting it
Some sockets don't care about msghdr::ubuf_info and would execute the request by copying data. Such fallback behaviour was always a pain in my experience, so we'd rather want to fail such requests and have a more robust api in the future.
Mark struct socket that support it with a new SOCK_SUPPORT_ZC flag. I'm not entirely sure it's the best place for the flag but at least we don't have to do a bunch of extra dereferences in the hot path.
P.S. patches 2 and 3 are not combined for backporting purposes.
Pavel Begunkov (3):
net: flag sockets supporting msghdr originated zerocopy io_uring/net: fail zc send when unsupported by socket io_uring/net: fail zc sendmsg when unsupported by socket
include/linux/net.h | 1 + io_uring/net.c | 4 ++++ net/ipv4/tcp.c | 1 + net/ipv4/udp.c | 1 + 4 files changed, 7 insertions(+)
Signature · 1 line
2.38.0
PATCH 1/3 net: flag sockets supporting msghdr originated zerocopy
We need an efficient way in io_uring to check whether a socket supports zerocopy with msghdr provided ubuf_info. Add a new flag into the struct socket flags fields.
--- include/linux/net.h | 1 + net/ipv4/tcp.c | 1 + net/ipv4/udp.c | 1 + 3 files changed, 3 insertions(+)
include/linux/net.h +1 -0
@@ -41,6 +41,7 @@ struct net;#define SOCK_NOSPACE 2#define SOCK_PASSCRED 3#define SOCK_PASSSEC 4 +#define SOCK_SUPPORT_ZC 5#ifndef ARCH_HAS_SOCKET_TYPES /**
net/ipv4/tcp.c +1 -0
@@ -457,6 +457,7 @@ void tcp_init_sock(struct sock *sk)WRITE_ONCE (sk->sk_sndbuf,READ_ONCE (sock_net (sk)->ipv4.sysctl_tcp_wmem[1 ]));WRITE_ONCE (sk->sk_rcvbuf,READ_ONCE (sock_net (sk)->ipv4.sysctl_tcp_rmem[1 ])); +set_bit (SOCK_SUPPORT_ZC , &sk->sk_socket->flags);sk_sockets_allocated_inc (sk); }EXPORT_SYMBOL (tcp_init_sock);
net/ipv4/udp.c +1 -0
@@ -1620,6 +1620,7 @@ int udp_init_sock(struct sock *sk) {skb_queue_head_init (&udp_sk (sk)->reader_queue); sk->sk_destruct = udp_destruct_sock; +set_bit (SOCK_SUPPORT_ZC , &sk->sk_socket->flags);return 0 ; }EXPORT_SYMBOL_GPL (udp_init_sock);
Signature · 1 line
2.38.0
Re: [PATCH for-6.1 1/3] net: flag sockets supporting msghdr originated zerocopy
Quoted 17 lines · On Fri, 21 Oct 2022 11:16:39 +0100 Pavel Begunkov wrote:
We need an efficient way in
io_uringto check whether a socket supports zerocopy with msghdr providedubuf_info. Add a new flag into the struct socket flags fields.Cc <[email protected]> # 6.0Signed-off-by Pavel Begunkov <[email protected]>--- include/linux/net.h | 1 + net/ipv4/tcp.c | 1 + net/ipv4/udp.c | 1 + 3 files changed, 3 insertions(+)include/linux/net.h +0
-0
include/linux/net.h struct net; · line 44
Quoted 4 lines
include/linux/net.h +1
-0@@ -41,6 +41,7 @@ struct net;#define SOCK_NOSPACE 2#define SOCK_PASSCRED 3#define SOCK_PASSSEC 4 +#define SOCK_SUPPORT_ZC 5
Any idea on when this will make it to Linus? If within a week we can probably delay:
https://lore.kernel.org/all/dc549a4d5c1d2031c64794c8e12bed55afb85c3e.1666287924.git.pabeni@redhat.com/
and avoid the conflict.
Re: [PATCH for-6.1 1/3] net: flag sockets supporting msghdr originated zerocopy
Quoted 25 lines · Jakub Kicinski
Quoted 17 lines · On Fri, 21 Oct 2022 11:16:39 +0100 Pavel Begunkov wrote:
We need an efficient way in
io_uringto check whether a socket supports zerocopy with msghdr providedubuf_info. Add a new flag into the struct socket flags fields.Cc <[email protected]> # 6.0Signed-off-by Pavel Begunkov <[email protected]>--- include/linux/net.h | 1 + net/ipv4/tcp.c | 1 + net/ipv4/udp.c | 1 + 3 files changed, 3 insertions(+)include/linux/net.h +0
-0include/linux/net.h struct net; · line 44
Quoted 4 lines
include/linux/net.h +1
-0@@ -41,6 +41,7 @@ struct net;#define SOCK_NOSPACE 2#define SOCK_PASSCRED 3#define SOCK_PASSSEC 4 +#define SOCK_SUPPORT_ZC 5Acked-by Jakub Kicinski <[email protected]>
Quoted 2 lines
Any idea on when this will make it to Linus? If within a week we can probably delay:
After a chat with Jens, IIUC he can take it and send out to Linus early. So, sounds like a good plan
Quoted 2 lines
https://lore.kernel.org/all/dc549a4d5c1d2031c64794c8e12bed55afb85c3e.1666287924.git.pabeni@redhat.com/
and avoid the conflict.
Signature · 1 line
Pavel Begunkov
Jens Axboe
maintainerRe: [PATCH for-6.1 1/3] net: flag sockets supporting msghdr originated zerocopy
Quoted 26 lines · Pavel Begunkov
Quoted 25 lines · Jakub Kicinski
Quoted 17 lines · On Fri, 21 Oct 2022 11:16:39 +0100 Pavel Begunkov wrote:
We need an efficient way in
io_uringto check whether a socket supports zerocopy with msghdr providedubuf_info. Add a new flag into the struct socket flags fields.Cc <[email protected]> # 6.0Signed-off-by Pavel Begunkov <[email protected]>---
? include/linux/net.h | 1 + ? net/ipv4/tcp.c????? | 1 + ? net/ipv4/udp.c????? | 1 +? 3 files changed, 3 insertions(+)
include/linux/net.h +0
-0@@ -41,6 +41,7 @@ struct net;Quoted 4 lines
? #define
SOCK_NOSPACE??????? 2
? #defineSOCK_PASSCRED??????? 3
? #defineSOCK_PASSSEC??????? 4
+#defineSOCK_SUPPORT_ZC??????? 5Acked-by Jakub Kicinski <[email protected]>
Quoted 5 lines
Quoted 2 lines
Any idea on when this will make it to Linus? If within a week we can probably delay:
After a chat with Jens, IIUC he can take it and send out to Linus early. So, sounds like a good plan
Yes, and let's retain the name for now, can always be changed if we need to make it more granular. I'll ship this off before -rc2.
Signature · 1 line
Jens Axboe
Re: [PATCH for-6.1 1/3] net: flag sockets supporting msghdr originated zerocopy
Am 22.10.22 um 18:07 schrieb Jens Axboe:
Quoted 27 lines · Pavel Begunkov
Quoted 26 lines · Jakub Kicinski
Quoted 25 lines · On Fri, 21 Oct 2022 11:16:39 +0100 Pavel Begunkov wrote:
Quoted 17 lines
We need an efficient way in
io_uringto check whether a socket supports zerocopy with msghdr providedubuf_info. Add a new flag into the struct socket flags fields.Cc <[email protected]> # 6.0Signed-off-by Pavel Begunkov <[email protected]>---
? include/linux/net.h | 1 + ? net/ipv4/tcp.c????? | 1 + ? net/ipv4/udp.c????? | 1 +? 3 files changed, 3 insertions(+)
include/linux/net.h +0
-0@@ -41,6 +41,7 @@ struct net;Quoted 4 lines
? #define
SOCK_NOSPACE??????? 2
? #defineSOCK_PASSCRED??????? 3
? #defineSOCK_PASSSEC??????? 4
+#defineSOCK_SUPPORT_ZC??????? 5Acked-by Jakub Kicinski <[email protected]>
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
Any idea on when this will make it to Linus? If within a week we can probably delay:
After a chat with Jens, IIUC he can take it and send out to Linus early. So, sounds like a good plan
Yes, and let's retain the name for now, can always be changed if we need to make it more granular. I'll ship this off before -rc2.
I'm now always getting -EOPNOTSUPP from SENDMSG_ZC for tcp connections...
metze
Re: [PATCH for-6.1 1/3] net: flag sockets supporting msghdr originated zerocopy
Am 24.10.22 um 14:49 schrieb Stefan Metzmacher:
Quoted 31 lines
Am 22.10.22 um 18:07 schrieb Jens Axboe:
Quoted 30 lines · Pavel Begunkov
Quoted 29 lines · Jakub Kicinski
Quoted 27 lines · On Fri, 21 Oct 2022 11:16:39 +0100 Pavel Begunkov wrote:
Quoted 17 lines
We need an efficient way in
io_uringto check whether a socket supports zerocopy with msghdr providedubuf_info. Add a new flag into the struct socket flags fields.Cc <[email protected]> # 6.0Signed-off-by Pavel Begunkov <[email protected]>---
? include/linux/net.h | 1 + ? net/ipv4/tcp.c????? | 1 + ? net/ipv4/udp.c????? | 1 +? 3 files changed, 3 insertions(+)
include/linux/net.h +0
-0@@ -41,6 +41,7 @@ struct net;Quoted 4 lines
? #define
SOCK_NOSPACE??????? 2
? #defineSOCK_PASSCRED??????? 3
? #defineSOCK_PASSSEC??????? 4
+#defineSOCK_SUPPORT_ZC??????? 5Acked-by Jakub Kicinski <[email protected]>Any idea on when this will make it to Linus? If within a week we can probably delay:
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
After a chat with Jens, IIUC he can take it and send out to Linus early. So, sounds like a good plan
Yes, and let's retain the name for now, can always be changed if we need to make it more granular. I'll ship this off before -rc2.
I'm now always getting
-EOPNOTSUPPfromSENDMSG_ZCfor tcp connections...
The problem is that inet_accept() doesn't set SOCK_SUPPORT_ZC...
This hack below fixes it for me, but I'm sure there's a nicer way...
The natural way would be to overload inet_csk_accept for tcp, but sk1->sk_prot->accept() is called with sk2->sk_socket == NULL, because sock_graft() is called later...
metze
net/ipv4/af_inet.c +3 -0
@@ -736,6 +736,7 @@ EXPORT_SYMBOL(inet_stream_connect);* Accept a pending connection. The TCP layer now gives BSD semantics. */ +#include <net/transp_v6.h> int inet_accept (struct socket *sock,struct socket *newsock,int flags,bool kern) {
@@ -754,6 +755,8 @@ int inet_accept(struct socket *sock, struct socket *newsock, int flags, (TCPF_ESTABLISHED |TCPF_SYN_RECV | TCPF_CLOSE_WAIT | TCPF_CLOSE))); +if (sk2->sk_prot == &tcp_prot || sk2->sk_prot == &tcpv6_prot) +set_bit (SOCK_SUPPORT_ZC , &newsock->flags);sock_graft (sk2, newsock); newsock->state =SS_CONNECTED ;
Re: [PATCH for-6.1 1/3] net: flag sockets supporting msghdr originated zerocopy
Quoted 35 lines · Stefan Metzmacher
Am 24.10.22 um 14:49 schrieb Stefan Metzmacher:
Quoted 34 lines
Am 22.10.22 um 18:07 schrieb Jens Axboe:
Quoted 33 lines · Pavel Begunkov
Quoted 31 lines · Jakub Kicinski
Quoted 25 lines · On Fri, 21 Oct 2022 11:16:39 +0100 Pavel Begunkov wrote:
Quoted 17 lines
We need an efficient way in
io_uringto check whether a socket supports zerocopy with msghdr providedubuf_info. Add a new flag into the struct socket flags fields.Cc <[email protected]> # 6.0Signed-off-by Pavel Begunkov <[email protected]>---
? include/linux/net.h | 1 + ? net/ipv4/tcp.c????? | 1 + ? net/ipv4/udp.c????? | 1 +? 3 files changed, 3 insertions(+)
include/linux/net.h +0
-0@@ -41,6 +41,7 @@ struct net;Quoted 4 lines
? #define
SOCK_NOSPACE??????? 2
? #defineSOCK_PASSCRED??????? 3
? #defineSOCK_PASSSEC??????? 4
+#defineSOCK_SUPPORT_ZC??????? 5Acked-by Jakub Kicinski <[email protected]>Quoted 2 lines
Any idea on when this will make it to Linus? If within a week we can probably delay:
After a chat with Jens, IIUC he can take it and send out to Linus early. So, sounds like a good plan
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Yes, and let's retain the name for now, can always be changed if we need to make it more granular. I'll ship this off before -rc2.
I'm now always getting
-EOPNOTSUPPfromSENDMSG_ZCfor tcp connections...The problem is that
inet_accept()doesn't setSOCK_SUPPORT_ZC...
Yeah, you're right, accept is not handled, not great. Thanks for tracking it down. And I'll add a test for it
Quoted 5 lines
This hack below fixes it for me, but I'm sure there's a nicer way...
The natural way would be to overload
inet_csk_acceptfor tcp, butsk1->sk_prot->accept()is called withsk2->sk_socket== NULL, becausesock_graft()is called later...
I think it's acceptable for a fix. sk_is_tcp() sounds a bit better assuming that sk_type/protocol are set there.
Quoted 21 lines
net/ipv4/af_inet.c +3
-0@@ -736,6 +736,7 @@ EXPORT_SYMBOL(inet_stream_connect);* Accept a pending connection. The TCP layer now gives BSD semantics. */ +#include <net/transp_v6.h> int inet_accept(struct socket *sock, struct socket *newsock, int flags, bool kern) {@@ -754,6 +755,8 @@ int inet_accept(struct socket *sock, struct socket *newsock, int flags, (TCPF_ESTABLISHED | TCPF_SYN_RECV | TCPF_CLOSE_WAIT | TCPF_CLOSE))); + if (sk2->sk_prot == &tcp_prot || sk2->sk_prot == &tcpv6_prot) + set_bit(SOCK_SUPPORT_ZC, &newsock->flags); sock_graft(sk2, newsock); newsock->state = SS_CONNECTED;
Signature · 1 line
Pavel Begunkov
Re: [PATCH for-6.1 1/3] net: flag sockets supporting msghdr originated zerocopy
Am 24.10.22 um 19:00 schrieb Pavel Begunkov:
Quoted 73 lines · Stefan Metzmacher
Quoted 35 lines
Am 24.10.22 um 14:49 schrieb Stefan Metzmacher:
Quoted 34 lines
Am 22.10.22 um 18:07 schrieb Jens Axboe:
Quoted 33 lines · Pavel Begunkov
Quoted 31 lines · Jakub Kicinski
Quoted 25 lines · On Fri, 21 Oct 2022 11:16:39 +0100 Pavel Begunkov wrote:
Quoted 17 lines
We need an efficient way in
io_uringto check whether a socket supports zerocopy with msghdr providedubuf_info. Add a new flag into the struct socket flags fields.Cc <[email protected]> # 6.0Signed-off-by Pavel Begunkov <[email protected]>---
? include/linux/net.h | 1 + ? net/ipv4/tcp.c????? | 1 + ? net/ipv4/udp.c????? | 1 +? 3 files changed, 3 insertions(+)
include/linux/net.h +0
-0@@ -41,6 +41,7 @@ struct net;Quoted 4 lines
? #define
SOCK_NOSPACE??????? 2
? #defineSOCK_PASSCRED??????? 3
? #defineSOCK_PASSSEC??????? 4
+#defineSOCK_SUPPORT_ZC??????? 5Acked-by Jakub Kicinski <[email protected]>Quoted 2 lines
Any idea on when this will make it to Linus? If within a week we can probably delay:
After a chat with Jens, IIUC he can take it and send out to Linus early. So, sounds like a good plan
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Yes, and let's retain the name for now, can always be changed if we need to make it more granular. I'll ship this off before -rc2.
I'm now always getting
-EOPNOTSUPPfromSENDMSG_ZCfor tcp connections...The problem is that
inet_accept()doesn't setSOCK_SUPPORT_ZC...Yeah, you're right, accept is not handled, not great. Thanks for tracking it down. And I'll add a test for it
Quoted 5 lines
This hack below fixes it for me, but I'm sure there's a nicer way...
The natural way would be to overload
inet_csk_acceptfor tcp, butsk1->sk_prot->accept()is called withsk2->sk_socket== NULL, becausesock_graft()is called later...I think it's acceptable for a fix.
sk_is_tcp()sounds a bit better assuming that sk_type/protocol are set there.Quoted 16 lines
net/ipv4/af_inet.c +1
-0@@ -736,6 +736,7 @@ EXPORT_SYMBOL(inet_stream_connect);* Accept a pending connection. The TCP layer now gives BSD semantics. */ +#include <net/transp_v6.h> int inet_accept(struct socket *sock, struct socket *newsock, int flags, bool kern) {@@ -754,6 +755,8 @@ int inet_accept(struct socket *sock, struct socket *newsock, int flags,(
TCPF_ESTABLISHED|TCPF_SYN_RECV|
TCPF_CLOSE_WAIT|TCPF_CLOSE)));
Quoted 3 lines
Quoted 3 lines
+ if (
sk2->sk_prot==&tcp_prot||sk2->sk_prot==&tcpv6_prot) +set_bit(SOCK_SUPPORT_ZC, &newsock->flags);sock_graft(sk2, newsock);
Hmm, I think we just need to check if the flag was set on the listening socket, otherwise we have a hard coded value again...
This also works for me, and seems like a much nicer patch:
net/ipv4/af_inet.c +2 -0
@@ -754,6 +754,8 @@ int inet_accept(struct socket *sock, struct socket *newsock, int flags, (TCPF_ESTABLISHED |TCPF_SYN_RECV | TCPF_CLOSE_WAIT | TCPF_CLOSE))); +if (test_bit (SOCK_SUPPORT_ZC , &sock->flags)) +set_bit (SOCK_SUPPORT_ZC , &newsock->flags);sock_graft (sk2, newsock); newsock->state =SS_CONNECTED ;
PATCH 2/3 io_uring/net: fail zc send when unsupported by socket
In reply to Pavel BegunkovIf a protocol doesn't support zerocopy it will silently fall back to copying. This type of behaviour has always been a source of troubles so it's better to fail such requests instead.
--- io_uring/net.c | 2 ++ 1 file changed, 2 insertions(+)
io_uring/net.c +2 -0
@@ -1056,6 +1056,8 @@ int io_send_zc(struct io_kiocb *req, unsigned int issue_flags) sock =sock_from_file (req->file);if (unlikely (!sock))return -ENOTSOCK ; +if (!test_bit (SOCK_SUPPORT_ZC , &sock->flags)) +return -EOPNOTSUPP ; msg.msg_name =NULL ; msg.msg_control =NULL ;
Signature · 1 line
2.38.0
PATCH 3/3 io_uring/net: fail zc sendmsg when unsupported by socket
In reply to Pavel BegunkovThe previous patch fails zerocopy send requests for protocols that don't support it, do the same for zerocopy sendmsg.
--- io_uring/net.c | 2 ++ 1 file changed, 2 insertions(+)
io_uring/net.c +2 -0
@@ -1153,6 +1153,8 @@ int io_sendmsg_zc(struct io_kiocb *req, unsigned int issue_flags) sock =sock_from_file (req->file);if (unlikely (!sock))return -ENOTSOCK ; +if (!test_bit (SOCK_SUPPORT_ZC , &sock->flags)) +return -EOPNOTSUPP ;if (req_has_async_data (req)) { kmsg = req->async_data;
Signature · 1 line
2.38.0
Hi Pavel,
Quoted 8 lines
Some sockets don't care about
msghdr::ubuf_infoand would execute the request by copying data. Such fallback behaviour was always a pain in my experience, so we'd rather want to fail such requests and have a more robust api in the future.Mark struct socket that support it with a new
SOCK_SUPPORT_ZCflag. I'm not entirely sure it's the best place for the flag but at least we don't have to do a bunch of extra dereferences in the hot path.
I'd give the flag another name that indicates msg_ubuf and
have a 2nd flag that can indicate support for SO_ZEROCOPY in sk_setsockopt()
The SO_ZEROCOPY version is also provided by AF_RDS.
metze
Quoted 7 lines · Stefan Metzmacher
Hi Pavel,
Quoted 5 lines
Some sockets don't care about
msghdr::ubuf_infoand would execute the request by copying data. Such fallback behaviour was always a pain in my experience, so we'd rather want to fail such requests and have a more robust api in the future.
Quoted 5 lines
Quoted 3 lines
Mark struct socket that support it with a new
SOCK_SUPPORT_ZCflag. I'm not entirely sure it's the best place for the flag but at least we don't have to do a bunch of extra dereferences in the hot path.I'd give the flag another name that indicates
msg_ubufand
Could be renamed, e.g. SOCK_SUPPORT_MSGHDR_UBUF or maybe SOCK_SUPPORT_EXTERNAL_UBUF
Quoted 1 line
have a 2nd flag that can indicate support for
SO_ZEROCOPYinsk_setsockopt()
There is absolutely no reason to introduce a second flag here, it has nothing to do with SO_ZEROCOPY.
Quoted 1 line
The
SO_ZEROCOPYversion is also provided byAF_RDS.
Signature · 1 line
Pavel Begunkov
Am 21.10.22 um 12:42 schrieb Pavel Begunkov:
Quoted 8 lines · Stefan Metzmacher
Quoted 7 lines
Hi Pavel,
Quoted 5 lines
Some sockets don't care about
msghdr::ubuf_infoand would execute the request by copying data. Such fallback behaviour was always a pain in my experience, so we'd rather want to fail such requests and have a more robust api in the future.
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
Mark struct socket that support it with a new
SOCK_SUPPORT_ZCflag. I'm not entirely sure it's the best place for the flag but at least we don't have to do a bunch of extra dereferences in the hot path.I'd give the flag another name that indicates
msg_ubufandCould be renamed, e.g.
SOCK_SUPPORT_MSGHDR_UBUF
That's good or SOCK_SUPPORT_ZC_MSGHDR_UBUF.
Quoted 4 lines
Quoted 1 line
have a 2nd flag that can indicate support for
SO_ZEROCOPYinsk_setsockopt()There is absolutely no reason to introduce a second flag here, it has nothing to do with
SO_ZEROCOPY.
I meant as a separate change to replace the hard coded logic in sk_setsockopt()... But I don't care much about it, it's unlikely that I ever want to use SO_ZEROCOPY...
metze
This series was applied to netdev/net.git (master) by Jens Axboe <[email protected]>:
Hello:
This series was applied to netdev/net.git (master) by Jens Axboe <[email protected]>:
Quoted 10 lines · On Fri, 21 Oct 2022 11:16:38 +0100 you wrote:
Some sockets don't care about
msghdr::ubuf_infoand would execute the request by copying data. Such fallback behaviour was always a pain in my experience, so we'd rather want to fail such requests and have a more robust api in the future.Mark struct socket that support it with a new
SOCK_SUPPORT_ZCflag. I'm not entirely sure it's the best place for the flag but at least we don't have to do a bunch of extra dereferences in the hot path.[...]
Here is the summary with links:
- [for-6.1,1/3] net: flag sockets supporting msghdr originated zerocopy
https://git.kernel.org/netdev/net/c/e993ffe3da4b - [for-6.1,2/3] io_uring/net: fail zc send when unsupported by socket https://git.kernel.org/netdev/net/c/edf81438799c - [for-6.1,3/3] io_uring/net: fail zc sendmsg when unsupported by socket https://git.kernel.org/netdev/net/c/cc767e7c6913
You are awesome, thank you!
Signature · 2 lines
Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
Jakub Kicinski
maintainerQuoted 7 lines · On Mon, 24 Oct 2022 20:40:38 +0000 [email protected] wrote:
Here is the summary with links:
- [for-6.1,1/3] net: flag sockets supporting msghdr originated zerocopyhttps://git.kernel.org/netdev/net/c/e993ffe3da4b - [for-6.1,2/3] io_uring/net: fail zc send when unsupported by socket https://git.kernel.org/netdev/net/c/edf81438799c - [for-6.1,3/3] io_uring/net: fail zc sendmsg when unsupported by socket https://git.kernel.org/netdev/net/c/cc767e7c6913
Ugh, ignore, looks like we forgot to drop this series from our PW and bot thought we've applied it when we forwarded the tree.