| Message ID | 20210604074419.53956-1-dong.menglong@zte.com.cn |
|---|---|
| State | New |
| Headers | show |
| Series | [net-next] net: tipc: fix FB_MTU eat two pages | expand |
Hello Maloy, On Sat, Jun 5, 2021 at 3:20 AM Jon Maloy <jmaloy@redhat.com> wrote: > > [...] > Please don't add any extra file just for this little fix. We have enough > files. > Keep the macros in msg.h/c where they used to be. You can still add > your copyright line to those files. > Regarding the macros kept inside msg.c, they are there because we design > by the principle of minimal exposure, even among our module internal files. > Otherwise it is ok. > I don't want to add a new file too, but I found it's hard to define FB_MTU. I tried to define it in msg.h, and 'crypto.h' should be included, which 'BUF_HEADROOM' is defined in. However, 'msg.h' is already included in 'crypto.h', so it doesn't work. I tried to define FB_MTU in 'crypto.h', but it feels weird to define it here. And FB_MTU is also used in 'bcast.c', so it can't be defined in 'msg.c'. I will see if there is a better solution. Thanks! Menglong Dong > > @@ -0,0 +1,55 @@ > > +/* SPDX-License-Identifier: GPL-2.0-only */ > > +/* > > + * Copyright 2021 ZTE Corporation. > > + * All rights reserved. > > + * > > + * Redistribution and use in source and binary forms, with or without > > + * modification, are permitted provided that the following conditions are met: > > + * > > + * 1. Redistributions of source code must retain the above copyright > > + * notice, this list of conditions and the following disclaimer. > > + * 2. Redistributions in binary form must reproduce the above copyright > > + * notice, this list of conditions and the following disclaimer in the > > + * documentation and/or other materials provided with the distribution. > > + * 3. Neither the names of the copyright holders nor the names of its > > + * contributors may be used to endorse or promote products derived from > > + * this software without specific prior written permission. > > + * > > + * Alternatively, this software may be distributed under the terms of the > > + * GNU General Public License ("GPL") version 2 as published by the Free > > + * Software Foundation. > > + * > > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" > > + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE > > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE > > + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE > > + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR > > + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF > > + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS > > + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN > > + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) > > + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE > > + * POSSIBILITY OF SUCH DAMAGE. > > + */ > > + > > +#ifndef _TIPC_MTU_H > > +#define _TIPC_MTU_H > > + > > +#include <linux/tipc.h> > > +#include "crypto.h" > > + > > +#ifdef CONFIG_TIPC_CRYPTO > > +#define BUF_HEADROOM ALIGN(((LL_MAX_HEADER + 48) + EHDR_MAX_SIZE), 16) > > +#define BUF_TAILROOM (TIPC_AES_GCM_TAG_SIZE) > > +#define FB_MTU (PAGE_SIZE - \ > > + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) - \ > > + SKB_DATA_ALIGN(BUF_HEADROOM + BUF_TAILROOM + 3)) > > +#else > > +#define BUF_HEADROOM (LL_MAX_HEADER + 48) > > +#define BUF_TAILROOM 16 > > +#define FB_MTU (PAGE_SIZE - \ > > + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) - \ > > + SKB_DATA_ALIGN(BUF_HEADROOM + 3)) > > +#endif > > + > > +#endif >
On 6/4/21 9:28 PM, Menglong Dong wrote: > Hello Maloy, > > On Sat, Jun 5, 2021 at 3:20 AM Jon Maloy <jmaloy@redhat.com> wrote: >> > [...] >> Please don't add any extra file just for this little fix. We have enough >> files. >> Keep the macros in msg.h/c where they used to be. You can still add >> your copyright line to those files. >> Regarding the macros kept inside msg.c, they are there because we design >> by the principle of minimal exposure, even among our module internal files. >> Otherwise it is ok. >> > I don't want to add a new file too, but I found it's hard to define FB_MTU. I > tried to define it in msg.h, and 'crypto.h' should be included, which > 'BUF_HEADROOM' is defined in. However, 'msg.h' is already included in > 'crypto.h', so it doesn't work. > > I tried to define FB_MTU in 'crypto.h', but it feels weird to define > it here. And > FB_MTU is also used in 'bcast.c', so it can't be defined in 'msg.c'. > > I will see if there is a better solution. I think we can leverage the fact that this by definition is a node local message, and those are never encrypted. So, if you base FB_MTU on the non-crypto versions of BUF_HEADROOM and BUF_TAILROOM we should be safe. That will even give us better utilization of the space available. ///jon > > Thanks! > Menglong Dong > >>> @@ -0,0 +1,55 @@ >>> +/* SPDX-License-Identifier: GPL-2.0-only */ >>> +/* >>> + * Copyright 2021 ZTE Corporation. >>> + * All rights reserved. >>> + * >>> + * Redistribution and use in source and binary forms, with or without >>> + * modification, are permitted provided that the following conditions are met: >>> + * >>> + * 1. Redistributions of source code must retain the above copyright >>> + * notice, this list of conditions and the following disclaimer. >>> + * 2. Redistributions in binary form must reproduce the above copyright >>> + * notice, this list of conditions and the following disclaimer in the >>> + * documentation and/or other materials provided with the distribution. >>> + * 3. Neither the names of the copyright holders nor the names of its >>> + * contributors may be used to endorse or promote products derived from >>> + * this software without specific prior written permission. >>> + * >>> + * Alternatively, this software may be distributed under the terms of the >>> + * GNU General Public License ("GPL") version 2 as published by the Free >>> + * Software Foundation. >>> + * >>> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" >>> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE >>> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE >>> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE >>> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR >>> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF >>> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS >>> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN >>> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) >>> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE >>> + * POSSIBILITY OF SUCH DAMAGE. >>> + */ >>> + >>> +#ifndef _TIPC_MTU_H >>> +#define _TIPC_MTU_H >>> + >>> +#include <linux/tipc.h> >>> +#include "crypto.h" >>> + >>> +#ifdef CONFIG_TIPC_CRYPTO >>> +#define BUF_HEADROOM ALIGN(((LL_MAX_HEADER + 48) + EHDR_MAX_SIZE), 16) >>> +#define BUF_TAILROOM (TIPC_AES_GCM_TAG_SIZE) >>> +#define FB_MTU (PAGE_SIZE - \ >>> + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) - \ >>> + SKB_DATA_ALIGN(BUF_HEADROOM + BUF_TAILROOM + 3)) >>> +#else >>> +#define BUF_HEADROOM (LL_MAX_HEADER + 48) >>> +#define BUF_TAILROOM 16 >>> +#define FB_MTU (PAGE_SIZE - \ >>> + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) - \ >>> + SKB_DATA_ALIGN(BUF_HEADROOM + 3)) >>> +#endif >>> + >>> +#endif
On Sat, Jun 5, 2021 at 10:25 PM Jon Maloy <jmaloy@redhat.com> wrote: > > > > On 6/4/21 9:28 PM, Menglong Dong wrote: > > Hello Maloy, > > > > On Sat, Jun 5, 2021 at 3:20 AM Jon Maloy <jmaloy@redhat.com> wrote: > >> > > [...] > >> Please don't add any extra file just for this little fix. We have enough > >> files. > >> Keep the macros in msg.h/c where they used to be. You can still add > >> your copyright line to those files. > >> Regarding the macros kept inside msg.c, they are there because we design > >> by the principle of minimal exposure, even among our module internal files. > >> Otherwise it is ok. > >> > > I don't want to add a new file too, but I found it's hard to define FB_MTU. I > > tried to define it in msg.h, and 'crypto.h' should be included, which > > 'BUF_HEADROOM' is defined in. However, 'msg.h' is already included in > > 'crypto.h', so it doesn't work. > > > > I tried to define FB_MTU in 'crypto.h', but it feels weird to define > > it here. And > > FB_MTU is also used in 'bcast.c', so it can't be defined in 'msg.c'. > > > > I will see if there is a better solution. > I think we can leverage the fact that this by definition is a node local > message, and those are never encrypted. > So, if you base FB_MTU on the non-crypto versions of BUF_HEADROOM and > BUF_TAILROOM we should be safe. > That will even give us better utilization of the space available. > Ok, that sounds nice. I'll make a V2 based on that. Thanks! Menglong Dong
On Sat, Jun 05, 2021 at 10:25:53AM -0400, Jon Maloy wrote: > > > On 6/4/21 9:28 PM, Menglong Dong wrote: > > Hello Maloy, > > > > On Sat, Jun 5, 2021 at 3:20 AM Jon Maloy <jmaloy@redhat.com> wrote: > > > > > [...] > > > Please don't add any extra file just for this little fix. We have enough > > > files. > > > Keep the macros in msg.h/c where they used to be. You can still add > > > your copyright line to those files. > > > Regarding the macros kept inside msg.c, they are there because we design > > > by the principle of minimal exposure, even among our module internal files. > > > Otherwise it is ok. > > > > > I don't want to add a new file too, but I found it's hard to define FB_MTU. I > > tried to define it in msg.h, and 'crypto.h' should be included, which > > 'BUF_HEADROOM' is defined in. However, 'msg.h' is already included in > > 'crypto.h', so it doesn't work. > > > > I tried to define FB_MTU in 'crypto.h', but it feels weird to define > > it here. And > > FB_MTU is also used in 'bcast.c', so it can't be defined in 'msg.c'. > > > > I will see if there is a better solution. > I think we can leverage the fact that this by definition is a node local > message, and those are never encrypted. > So, if you base FB_MTU on the non-crypto versions of BUF_HEADROOM and > BUF_TAILROOM we should be safe. > That will even give us better utilization of the space available. > I think we misunderstanded something. If I base FB_MTU on the non-crypto version of BUF_TAILROOM, it will eat 2 pages too. Because the BUF_TAILROOM used in tipc_buf_acquire() is the crypto version, which is larger than the non-crypto version: struct sk_buff *tipc_buf_acquire(u32 size, gfp_t gfp) { struct sk_buff *skb; #ifdef CONFIG_TIPC_CRYPTO unsigned int buf_size = (BUF_HEADROOM + size + BUF_TAILROOM + 3) & ~3u; #else unsigned int buf_size = (BUF_HEADROOM + size + 3) & ~3u; #endif [..] } So if I use the non-crypto version, the size allocated will be: PAGE_SIZE - BUF_HEADROOM_non-crypto - BUF_TAILROOM_non-crypt + BUF_HEADROOM_crypto + BUF_TAILROOM_crypto which is larger than PAGE_SIZE. So, I think the simple way is to define FB_MTU in 'crypto.h'. Is this acceptable? Thanks! Menglong Dong
On 6/7/21 8:51 AM, Menglong Dong wrote: > On Sat, Jun 05, 2021 at 10:25:53AM -0400, Jon Maloy wrote: >> >> On 6/4/21 9:28 PM, Menglong Dong wrote: >>> Hello Maloy, >>> >>> On Sat, Jun 5, 2021 at 3:20 AM Jon Maloy <jmaloy@redhat.com> wrote: >>>> [...] >>> >>> So if I use the non-crypto version, the size allocated will be: >>> >>> PAGE_SIZE - BUF_HEADROOM_non-crypto - BUF_TAILROOM_non-crypt + >>> BUF_HEADROOM_crypto + BUF_TAILROOM_crypto >>> >>> which is larger than PAGE_SIZE. >>> >>> So, I think the simple way is to define FB_MTU in 'crypto.h'. Is this >>> acceptable? >>> >>> Thanks! >>> Menglong Dong >>> I spent a little more time looking into this. I think the best we can do is to keep FB_MTU internal to msg.c, and then add an outline function to msg.c that can be used by bcast.c. The way it is used is never time critical. I also see that we could need a little cleanup around this. There is a redundant align() function that should be removed and replaced with the global ALIGN() macro. Even tipc_buf_acquire() should use this macro instead of the explicit method that is used now. In general, I stongly dislike conditional code, and it is not necessary in this function. If we redefine the non-crypto BUF_TAILROOM to 0 instead of 16 (it is not used anywhere else) we could get rid of this too. But I leave that to you. If you only fix the FB_MTU macro I am content. ///jon
On Tue, Jun 08, 2021 at 06:37:38PM -0400, Jon Maloy wrote: > > [...] > I spent a little more time looking into this. I think the best we can do is > to keep FB_MTU internal to msg.c, and then add an outline function to msg.c > that can be used by bcast.c. The way it is used is never time critical. > > I also see that we could need a little cleanup around this. There is a > redundant align() function that should be removed and replaced with the > global ALIGN() macro. > Even tipc_buf_acquire() should use this macro instead of the explicit method > that is used now. > In general, I stongly dislike conditional code, and it is not necessary in > this function. If we redefine the non-crypto BUF_TAILROOM to 0 instead of 16 > (it is not used anywhere else) we could get rid of this too. > > But I leave that to you. If you only fix the FB_MTU macro I am content. > Yeah, I think I can handle it, just leave it to me. (finger heart :/) Menglong Dong
On 6/8/21 10:54 PM, Menglong Dong wrote: > On Tue, Jun 08, 2021 at 06:37:38PM -0400, Jon Maloy wrote: >> > [...] >> I spent a little more time looking into this. I think the best we can do is >> to keep FB_MTU internal to msg.c, and then add an outline function to msg.c >> that can be used by bcast.c. The way it is used is never time critical. >> >> I also see that we could need a little cleanup around this. There is a >> redundant align() function that should be removed and replaced with the >> global ALIGN() macro. >> Even tipc_buf_acquire() should use this macro instead of the explicit method >> that is used now. >> In general, I stongly dislike conditional code, and it is not necessary in >> this function. If we redefine the non-crypto BUF_TAILROOM to 0 instead of 16 >> (it is not used anywhere else) we could get rid of this too. >> >> But I leave that to you. If you only fix the FB_MTU macro I am content. >> > Yeah, I think I can handle it, just leave it to me. > > (finger heart :/) > Menglong DongI It seems like I have been misleading you. It turns out that these messages *will* be sent out over the nework in some cases, i.e. at multicast/broadcast over an UDP bearer. So, what we need is two macros, one with the conditional crypto head/tailroom defined as you first suggested, and one that only use the non-crypto head/tailroom as we have been discussing now. The first one can be defined inside bcast.c, the latter inside msg.c. It might also be a good idea to give the macros more descriptive names, such as ONEPAGE_MTU in the broadcast version, and ONEPAGE_SKB in the node local version. Does that make sense? ///jon >
On Wed, Jun 09, 2021 at 03:34:33AM -0400, Jon Maloy wrote: > > [...] > It seems like I have been misleading you. It turns out that these messages > *will* be sent out over the nework in some cases, i.e. at > multicast/broadcast over an UDP bearer. > So, what we need is two macros, one with the conditional crypto > head/tailroom defined as you first suggested, and one that only use the > non-crypto head/tailroom as we have been discussing now. > The first one can be defined inside bcast.c, the latter inside msg.c. > It might also be a good idea to give the macros more descriptive names, such > as ONEPAGE_MTU in the broadcast version, and ONEPAGE_SKB in the node local > version. > > Does that make sense? I think it's another point which can be optimized. However, with CONFIG_TIPC_CRYPTO=y, the BUF_HEADROOM used in tipc_buf_acquire() is always the crypto version, so it donen't work to define FB_MTU with non-crypto BUF_HEADROOM. So the point is to make a non-crypto version tipc_buf_acquire(), which is used to alloc data for non-crypto message with non-crypto BUF_HEADROOM. And that require us to distinguish the message that don't crypto. Is there simple way to achieve this goal? (Btw, I resended the patches for the 'two pages' problems.) Thanks! Menglong Dong > > >
diff --git a/net/tipc/bcast.c b/net/tipc/bcast.c index d4beca895992..c641b68e0812 100644 --- a/net/tipc/bcast.c +++ b/net/tipc/bcast.c @@ -41,6 +41,7 @@ #include "bcast.h" #include "link.h" #include "name_table.h" +#include "mtu.h" #define BCLINK_WIN_DEFAULT 50 /* bcast link window size (default) */ #define BCLINK_WIN_MIN 32 /* bcast minimum link window size */ diff --git a/net/tipc/msg.c b/net/tipc/msg.c index ce6ab54822d8..ec70d271c2da 100644 --- a/net/tipc/msg.c +++ b/net/tipc/msg.c @@ -40,15 +40,9 @@ #include "addr.h" #include "name_table.h" #include "crypto.h" +#include "mtu.h" #define MAX_FORWARD_SIZE 1024 -#ifdef CONFIG_TIPC_CRYPTO -#define BUF_HEADROOM ALIGN(((LL_MAX_HEADER + 48) + EHDR_MAX_SIZE), 16) -#define BUF_TAILROOM (TIPC_AES_GCM_TAG_SIZE) -#else -#define BUF_HEADROOM (LL_MAX_HEADER + 48) -#define BUF_TAILROOM 16 -#endif static unsigned int align(unsigned int i) { diff --git a/net/tipc/msg.h b/net/tipc/msg.h index 5d64596ba987..e83689d0f0f6 100644 --- a/net/tipc/msg.h +++ b/net/tipc/msg.h @@ -99,7 +99,6 @@ struct plist; #define MAX_H_SIZE 60 /* Largest possible TIPC header size */ #define MAX_MSG_SIZE (MAX_H_SIZE + TIPC_MAX_USER_MSG_SIZE) -#define FB_MTU 3744 #define TIPC_MEDIA_INFO_OFFSET 5 struct tipc_skb_cb { diff --git a/net/tipc/mtu.h b/net/tipc/mtu.h new file mode 100644 index 000000000000..033f0b178f9d --- /dev/null +++ b/net/tipc/mtu.h @@ -0,0 +1,55 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Copyright 2021 ZTE Corporation. + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * + * 1. Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * 2. Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in the + * documentation and/or other materials provided with the distribution. + * 3. Neither the names of the copyright holders nor the names of its + * contributors may be used to endorse or promote products derived from + * this software without specific prior written permission. + * + * Alternatively, this software may be distributed under the terms of the + * GNU General Public License ("GPL") version 2 as published by the Free + * Software Foundation. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE + * POSSIBILITY OF SUCH DAMAGE. + */ + +#ifndef _TIPC_MTU_H +#define _TIPC_MTU_H + +#include <linux/tipc.h> +#include "crypto.h" + +#ifdef CONFIG_TIPC_CRYPTO +#define BUF_HEADROOM ALIGN(((LL_MAX_HEADER + 48) + EHDR_MAX_SIZE), 16) +#define BUF_TAILROOM (TIPC_AES_GCM_TAG_SIZE) +#define FB_MTU (PAGE_SIZE - \ + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) - \ + SKB_DATA_ALIGN(BUF_HEADROOM + BUF_TAILROOM + 3)) +#else +#define BUF_HEADROOM (LL_MAX_HEADER + 48) +#define BUF_TAILROOM 16 +#define FB_MTU (PAGE_SIZE - \ + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) - \ + SKB_DATA_ALIGN(BUF_HEADROOM + 3)) +#endif + +#endif