Re: [PATCH net-next 1/5] net: mvpp2: use the same buffer pool for all ports
Hi Thomas, On Mon, Mar 05, 2018 at 01:41:48PM +0100, Thomas Petazzoni wrote: > On Mon, 5 Mar 2018 11:48:13 +0100, Antoine Tenart wrote: > > > > > +static void mvpp2_setup_bm_pool(void) > > > > +{ > > > > + /* Short pool */ > > > > + mvpp2_pools[MVPP2_BM_SHORT].buf_num = MVPP2_BM_SHORT_BUF_NUM; > > > > + mvpp2_pools[MVPP2_BM_SHORT].pkt_size = MVPP2_BM_SHORT_PKT_SIZE; > > > > + > > > > + /* Long pool */ > > > > + mvpp2_pools[MVPP2_BM_LONG].buf_num = MVPP2_BM_LONG_BUF_NUM; > > > > + mvpp2_pools[MVPP2_BM_LONG].pkt_size = MVPP2_BM_LONG_PKT_SIZE; > > > > +} > > > > > > ? > > > > I wanted to do this, but it's no possible as MVPP2_BM_SHORT_PKT_SIZE and > > MVPP2_BM_LONG_PKT_SIZE use a core definition which expands at some point > > to __max(...) which has to be called from within a function. > > Hum, weird: > > #define MVPP2_BM_LONG_PKT_SIZE > MVPP2_RX_MAX_PKT_SIZE(MVPP2_BM_LONG_FRAME_SIZE) > #define MVPP2_BM_LONG_FRAME_SIZE 2048 > > #define MVPP2_RX_MAX_PKT_SIZE(total_size) \ > ((total_size) - NET_SKB_PAD - MVPP2_SKB_SHINFO_SIZE) > > #define MVPP2_SKB_SHINFO_SIZE \ > SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) > > #define SKB_DATA_ALIGN(X)ALIGN(X, SMP_CACHE_BYTES) > > I don't really see a __max(...) call. NET_SKB_PAD expends to __max(...). > And if this value really expands depending on other values, then it > isn't really a constant, and should be considered as a constant, no? Well, the padding isn't truly a constant for some reasons, you can see an explanation here: https://elixir.bootlin.com/linux/latest/source/include/linux/skbuff.h#L2446 But this value can only change at compile time, so it really depends on what is the definition of a constant is :) Antoine -- Antoine Ténart, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com
Re: [PATCH net-next 1/5] net: mvpp2: use the same buffer pool for all ports
Hi Thomas, On Mon, Mar 05, 2018 at 01:41:48PM +0100, Thomas Petazzoni wrote: > On Mon, 5 Mar 2018 11:48:13 +0100, Antoine Tenart wrote: > > > > > +static void mvpp2_setup_bm_pool(void) > > > > +{ > > > > + /* Short pool */ > > > > + mvpp2_pools[MVPP2_BM_SHORT].buf_num = MVPP2_BM_SHORT_BUF_NUM; > > > > + mvpp2_pools[MVPP2_BM_SHORT].pkt_size = MVPP2_BM_SHORT_PKT_SIZE; > > > > + > > > > + /* Long pool */ > > > > + mvpp2_pools[MVPP2_BM_LONG].buf_num = MVPP2_BM_LONG_BUF_NUM; > > > > + mvpp2_pools[MVPP2_BM_LONG].pkt_size = MVPP2_BM_LONG_PKT_SIZE; > > > > +} > > > > > > ? > > > > I wanted to do this, but it's no possible as MVPP2_BM_SHORT_PKT_SIZE and > > MVPP2_BM_LONG_PKT_SIZE use a core definition which expands at some point > > to __max(...) which has to be called from within a function. > > Hum, weird: > > #define MVPP2_BM_LONG_PKT_SIZE > MVPP2_RX_MAX_PKT_SIZE(MVPP2_BM_LONG_FRAME_SIZE) > #define MVPP2_BM_LONG_FRAME_SIZE 2048 > > #define MVPP2_RX_MAX_PKT_SIZE(total_size) \ > ((total_size) - NET_SKB_PAD - MVPP2_SKB_SHINFO_SIZE) > > #define MVPP2_SKB_SHINFO_SIZE \ > SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) > > #define SKB_DATA_ALIGN(X)ALIGN(X, SMP_CACHE_BYTES) > > I don't really see a __max(...) call. NET_SKB_PAD expends to __max(...). > And if this value really expands depending on other values, then it > isn't really a constant, and should be considered as a constant, no? Well, the padding isn't truly a constant for some reasons, you can see an explanation here: https://elixir.bootlin.com/linux/latest/source/include/linux/skbuff.h#L2446 But this value can only change at compile time, so it really depends on what is the definition of a constant is :) Antoine -- Antoine Ténart, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com
Re: [PATCH net-next 1/5] net: mvpp2: use the same buffer pool for all ports
Hello, On Mon, 5 Mar 2018 11:48:13 +0100, Antoine Tenart wrote: > > > +static void mvpp2_setup_bm_pool(void) > > > +{ > > > + /* Short pool */ > > > + mvpp2_pools[MVPP2_BM_SHORT].buf_num = MVPP2_BM_SHORT_BUF_NUM; > > > + mvpp2_pools[MVPP2_BM_SHORT].pkt_size = MVPP2_BM_SHORT_PKT_SIZE; > > > + > > > + /* Long pool */ > > > + mvpp2_pools[MVPP2_BM_LONG].buf_num = MVPP2_BM_LONG_BUF_NUM; > > > + mvpp2_pools[MVPP2_BM_LONG].pkt_size = MVPP2_BM_LONG_PKT_SIZE; > > > +} > > > > ? > > I wanted to do this, but it's no possible as MVPP2_BM_SHORT_PKT_SIZE and > MVPP2_BM_LONG_PKT_SIZE use a core definition which expands at some point > to __max(...) which has to be called from within a function. Hum, weird: #define MVPP2_BM_LONG_PKT_SIZE MVPP2_RX_MAX_PKT_SIZE(MVPP2_BM_LONG_FRAME_SIZE) #define MVPP2_BM_LONG_FRAME_SIZE2048 #define MVPP2_RX_MAX_PKT_SIZE(total_size) \ ((total_size) - NET_SKB_PAD - MVPP2_SKB_SHINFO_SIZE) #define MVPP2_SKB_SHINFO_SIZE \ SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) #define SKB_DATA_ALIGN(X)ALIGN(X, SMP_CACHE_BYTES) I don't really see a __max(...) call. And if this value really expands depending on other values, then it isn't really a constant, and should be considered as a constant, no? Thomas -- Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering http://bootlin.com
Re: [PATCH net-next 1/5] net: mvpp2: use the same buffer pool for all ports
Hello, On Mon, 5 Mar 2018 11:48:13 +0100, Antoine Tenart wrote: > > > +static void mvpp2_setup_bm_pool(void) > > > +{ > > > + /* Short pool */ > > > + mvpp2_pools[MVPP2_BM_SHORT].buf_num = MVPP2_BM_SHORT_BUF_NUM; > > > + mvpp2_pools[MVPP2_BM_SHORT].pkt_size = MVPP2_BM_SHORT_PKT_SIZE; > > > + > > > + /* Long pool */ > > > + mvpp2_pools[MVPP2_BM_LONG].buf_num = MVPP2_BM_LONG_BUF_NUM; > > > + mvpp2_pools[MVPP2_BM_LONG].pkt_size = MVPP2_BM_LONG_PKT_SIZE; > > > +} > > > > ? > > I wanted to do this, but it's no possible as MVPP2_BM_SHORT_PKT_SIZE and > MVPP2_BM_LONG_PKT_SIZE use a core definition which expands at some point > to __max(...) which has to be called from within a function. Hum, weird: #define MVPP2_BM_LONG_PKT_SIZE MVPP2_RX_MAX_PKT_SIZE(MVPP2_BM_LONG_FRAME_SIZE) #define MVPP2_BM_LONG_FRAME_SIZE2048 #define MVPP2_RX_MAX_PKT_SIZE(total_size) \ ((total_size) - NET_SKB_PAD - MVPP2_SKB_SHINFO_SIZE) #define MVPP2_SKB_SHINFO_SIZE \ SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) #define SKB_DATA_ALIGN(X)ALIGN(X, SMP_CACHE_BYTES) I don't really see a __max(...) call. And if this value really expands depending on other values, then it isn't really a constant, and should be considered as a constant, no? Thomas -- Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering http://bootlin.com
Re: [PATCH net-next 1/5] net: mvpp2: use the same buffer pool for all ports
Hi Thomas, On Fri, Mar 02, 2018 at 05:01:59PM +0100, Thomas Petazzoni wrote: > On Fri, 2 Mar 2018 16:40:40 +0100, Antoine Tenart wrote: > > +static struct { > > + int pkt_size; > > + int buf_num; > > +} mvpp2_pools[MVPP2_BM_POOLS_NUM]; > > Any reason for not doing: > > } mvpp2_pools[MVPP2_BM_POOLS_NUM] = { > [MVPP2_BM_SHORT] = { > .pkt_size = MVPP2_BM_SHORT_PKT_SIZE, > .buf_num = MVPP2_BM_SHORT_BUF_NUM > }, > [MVPP2_BM_LONG] = { > .pkt_size = MVPP2_BM_LONG_PKT_SIZE, > .buf_num = MVPP2_BM_LONG_BUF_NUM, > }, > }; > > And get rid of: > > > +static void mvpp2_setup_bm_pool(void) > > +{ > > + /* Short pool */ > > + mvpp2_pools[MVPP2_BM_SHORT].buf_num = MVPP2_BM_SHORT_BUF_NUM; > > + mvpp2_pools[MVPP2_BM_SHORT].pkt_size = MVPP2_BM_SHORT_PKT_SIZE; > > + > > + /* Long pool */ > > + mvpp2_pools[MVPP2_BM_LONG].buf_num = MVPP2_BM_LONG_BUF_NUM; > > + mvpp2_pools[MVPP2_BM_LONG].pkt_size = MVPP2_BM_LONG_PKT_SIZE; > > +} > > ? I wanted to do this, but it's no possible as MVPP2_BM_SHORT_PKT_SIZE and MVPP2_BM_LONG_PKT_SIZE use a core definition which expands at some point to __max(...) which has to be called from within a function. That's why I kept mvpp2_setup_bm_pool(). Thanks! Antoine -- Antoine Ténart, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com
Re: [PATCH net-next 1/5] net: mvpp2: use the same buffer pool for all ports
Hi Thomas, On Fri, Mar 02, 2018 at 05:01:59PM +0100, Thomas Petazzoni wrote: > On Fri, 2 Mar 2018 16:40:40 +0100, Antoine Tenart wrote: > > +static struct { > > + int pkt_size; > > + int buf_num; > > +} mvpp2_pools[MVPP2_BM_POOLS_NUM]; > > Any reason for not doing: > > } mvpp2_pools[MVPP2_BM_POOLS_NUM] = { > [MVPP2_BM_SHORT] = { > .pkt_size = MVPP2_BM_SHORT_PKT_SIZE, > .buf_num = MVPP2_BM_SHORT_BUF_NUM > }, > [MVPP2_BM_LONG] = { > .pkt_size = MVPP2_BM_LONG_PKT_SIZE, > .buf_num = MVPP2_BM_LONG_BUF_NUM, > }, > }; > > And get rid of: > > > +static void mvpp2_setup_bm_pool(void) > > +{ > > + /* Short pool */ > > + mvpp2_pools[MVPP2_BM_SHORT].buf_num = MVPP2_BM_SHORT_BUF_NUM; > > + mvpp2_pools[MVPP2_BM_SHORT].pkt_size = MVPP2_BM_SHORT_PKT_SIZE; > > + > > + /* Long pool */ > > + mvpp2_pools[MVPP2_BM_LONG].buf_num = MVPP2_BM_LONG_BUF_NUM; > > + mvpp2_pools[MVPP2_BM_LONG].pkt_size = MVPP2_BM_LONG_PKT_SIZE; > > +} > > ? I wanted to do this, but it's no possible as MVPP2_BM_SHORT_PKT_SIZE and MVPP2_BM_LONG_PKT_SIZE use a core definition which expands at some point to __max(...) which has to be called from within a function. That's why I kept mvpp2_setup_bm_pool(). Thanks! Antoine -- Antoine Ténart, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com
RE: [PATCH net-next 1/5] net: mvpp2: use the same buffer pool for all ports
> Hello, > > On Fri, 2 Mar 2018 16:40:40 +0100, Antoine Tenart wrote: > > +static struct { > > + int pkt_size; > > + int buf_num; > > +} mvpp2_pools[MVPP2_BM_POOLS_NUM]; > > Any reason for not doing: > > } mvpp2_pools[MVPP2_BM_POOLS_NUM] = { > [MVPP2_BM_SHORT] = { > .pkt_size = MVPP2_BM_SHORT_PKT_SIZE, > .buf_num = MVPP2_BM_SHORT_BUF_NUM > }, > [MVPP2_BM_LONG] = { > .pkt_size = MVPP2_BM_LONG_PKT_SIZE, > .buf_num = MVPP2_BM_LONG_BUF_NUM, > }, > }; > > And get rid of: > > > +static void mvpp2_setup_bm_pool(void) { > > + /* Short pool */ > > + mvpp2_pools[MVPP2_BM_SHORT].buf_num = > MVPP2_BM_SHORT_BUF_NUM; > > + mvpp2_pools[MVPP2_BM_SHORT].pkt_size = > MVPP2_BM_SHORT_PKT_SIZE; > > + > > + /* Long pool */ > > + mvpp2_pools[MVPP2_BM_LONG].buf_num = > MVPP2_BM_LONG_BUF_NUM; > > + mvpp2_pools[MVPP2_BM_LONG].pkt_size = > MVPP2_BM_LONG_PKT_SIZE; } > > ? > No, we can change it. Stefan.
RE: [PATCH net-next 1/5] net: mvpp2: use the same buffer pool for all ports
> Hello, > > On Fri, 2 Mar 2018 16:40:40 +0100, Antoine Tenart wrote: > > +static struct { > > + int pkt_size; > > + int buf_num; > > +} mvpp2_pools[MVPP2_BM_POOLS_NUM]; > > Any reason for not doing: > > } mvpp2_pools[MVPP2_BM_POOLS_NUM] = { > [MVPP2_BM_SHORT] = { > .pkt_size = MVPP2_BM_SHORT_PKT_SIZE, > .buf_num = MVPP2_BM_SHORT_BUF_NUM > }, > [MVPP2_BM_LONG] = { > .pkt_size = MVPP2_BM_LONG_PKT_SIZE, > .buf_num = MVPP2_BM_LONG_BUF_NUM, > }, > }; > > And get rid of: > > > +static void mvpp2_setup_bm_pool(void) { > > + /* Short pool */ > > + mvpp2_pools[MVPP2_BM_SHORT].buf_num = > MVPP2_BM_SHORT_BUF_NUM; > > + mvpp2_pools[MVPP2_BM_SHORT].pkt_size = > MVPP2_BM_SHORT_PKT_SIZE; > > + > > + /* Long pool */ > > + mvpp2_pools[MVPP2_BM_LONG].buf_num = > MVPP2_BM_LONG_BUF_NUM; > > + mvpp2_pools[MVPP2_BM_LONG].pkt_size = > MVPP2_BM_LONG_PKT_SIZE; } > > ? > No, we can change it. Stefan.
Re: [PATCH net-next 1/5] net: mvpp2: use the same buffer pool for all ports
Hello, On Fri, 2 Mar 2018 16:40:40 +0100, Antoine Tenart wrote: > +static struct { > + int pkt_size; > + int buf_num; > +} mvpp2_pools[MVPP2_BM_POOLS_NUM]; Any reason for not doing: } mvpp2_pools[MVPP2_BM_POOLS_NUM] = { [MVPP2_BM_SHORT] = { .pkt_size = MVPP2_BM_SHORT_PKT_SIZE, .buf_num = MVPP2_BM_SHORT_BUF_NUM }, [MVPP2_BM_LONG] = { .pkt_size = MVPP2_BM_LONG_PKT_SIZE, .buf_num = MVPP2_BM_LONG_BUF_NUM, }, }; And get rid of: > +static void mvpp2_setup_bm_pool(void) > +{ > + /* Short pool */ > + mvpp2_pools[MVPP2_BM_SHORT].buf_num = MVPP2_BM_SHORT_BUF_NUM; > + mvpp2_pools[MVPP2_BM_SHORT].pkt_size = MVPP2_BM_SHORT_PKT_SIZE; > + > + /* Long pool */ > + mvpp2_pools[MVPP2_BM_LONG].buf_num = MVPP2_BM_LONG_BUF_NUM; > + mvpp2_pools[MVPP2_BM_LONG].pkt_size = MVPP2_BM_LONG_PKT_SIZE; > +} ? Best regards, Thomas -- Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering http://bootlin.com
Re: [PATCH net-next 1/5] net: mvpp2: use the same buffer pool for all ports
Hello, On Fri, 2 Mar 2018 16:40:40 +0100, Antoine Tenart wrote: > +static struct { > + int pkt_size; > + int buf_num; > +} mvpp2_pools[MVPP2_BM_POOLS_NUM]; Any reason for not doing: } mvpp2_pools[MVPP2_BM_POOLS_NUM] = { [MVPP2_BM_SHORT] = { .pkt_size = MVPP2_BM_SHORT_PKT_SIZE, .buf_num = MVPP2_BM_SHORT_BUF_NUM }, [MVPP2_BM_LONG] = { .pkt_size = MVPP2_BM_LONG_PKT_SIZE, .buf_num = MVPP2_BM_LONG_BUF_NUM, }, }; And get rid of: > +static void mvpp2_setup_bm_pool(void) > +{ > + /* Short pool */ > + mvpp2_pools[MVPP2_BM_SHORT].buf_num = MVPP2_BM_SHORT_BUF_NUM; > + mvpp2_pools[MVPP2_BM_SHORT].pkt_size = MVPP2_BM_SHORT_PKT_SIZE; > + > + /* Long pool */ > + mvpp2_pools[MVPP2_BM_LONG].buf_num = MVPP2_BM_LONG_BUF_NUM; > + mvpp2_pools[MVPP2_BM_LONG].pkt_size = MVPP2_BM_LONG_PKT_SIZE; > +} ? Best regards, Thomas -- Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering http://bootlin.com