| Message ID | 1418858186-25251-2-git-send-email-bill.fischofer@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 12/18/2014 01:16 AM, Bill Fischofer wrote: > Enable segmentation in packet buffer pools by default > > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> Both patches in the series have the same name. They should be renamed to describe what each of them actually does. Otherwise Reviewed-by: Taras Kondratiuk <taras.kondratiuk@linaro.org> > --- > platform/linux-generic/odp_buffer_pool.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/platform/linux-generic/odp_buffer_pool.c b/platform/linux-generic/odp_buffer_pool.c > index 48be24f..6b0e34b 100644 > --- a/platform/linux-generic/odp_buffer_pool.c > +++ b/platform/linux-generic/odp_buffer_pool.c > @@ -119,8 +119,8 @@ odp_buffer_pool_t odp_buffer_pool_create(const char *name, > if (params == NULL) > return ODP_BUFFER_POOL_INVALID; > > - /* Restriction for v1.0: All buffers are unsegmented */ > - const int unsegmented = 1; > + /* Restriction for v1.0: All non-packet buffers are unsegmented */ > + int unsegmented = 1; > > /* Restriction for v1.0: No zeroization support */ > const int zeroized = 0; > @@ -163,14 +163,18 @@ odp_buffer_pool_t odp_buffer_pool_create(const char *name, > case ODP_BUFFER_TYPE_ANY: > headroom = ODP_CONFIG_PACKET_HEADROOM; > tailroom = ODP_CONFIG_PACKET_TAILROOM; > - if (unsegmented) > + unsegmented = 0; /* Packets are segmented by default */ > + if (unsegmented) { > blk_size = ODP_ALIGN_ROUNDUP( > headroom + params->buf_size + tailroom, > buf_align); > - else > + } else { > blk_size = ODP_ALIGN_ROUNDUP( > headroom + params->buf_size + tailroom, > ODP_CONFIG_PACKET_BUF_LEN_MIN); > + if (blk_size > ODP_CONFIG_PACKET_BUF_LEN_MAX) > + return ODP_BUFFER_POOL_INVALID; > + } > buf_stride = params->buf_type == ODP_BUFFER_TYPE_PACKET ? > sizeof(odp_packet_hdr_stride) : > sizeof(odp_any_hdr_stride); >
It's one patch broken into two parts [1/2] [2/2]. Is this not correct? I'm trying to break things up since folks don't want to see single patches. On Thu, Dec 18, 2014 at 7:47 AM, Taras Kondratiuk < taras.kondratiuk@linaro.org> wrote: > > On 12/18/2014 01:16 AM, Bill Fischofer wrote: > >> Enable segmentation in packet buffer pools by default >> >> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> >> > > Both patches in the series have the same name. > They should be renamed to describe what each of them actually does. > Otherwise > > Reviewed-by: Taras Kondratiuk <taras.kondratiuk@linaro.org> > > > --- >> platform/linux-generic/odp_buffer_pool.c | 12 ++++++++---- >> 1 file changed, 8 insertions(+), 4 deletions(-) >> >> diff --git a/platform/linux-generic/odp_buffer_pool.c >> b/platform/linux-generic/odp_buffer_pool.c >> index 48be24f..6b0e34b 100644 >> --- a/platform/linux-generic/odp_buffer_pool.c >> +++ b/platform/linux-generic/odp_buffer_pool.c >> @@ -119,8 +119,8 @@ odp_buffer_pool_t odp_buffer_pool_create(const char >> *name, >> if (params == NULL) >> return ODP_BUFFER_POOL_INVALID; >> >> - /* Restriction for v1.0: All buffers are unsegmented */ >> - const int unsegmented = 1; >> + /* Restriction for v1.0: All non-packet buffers are unsegmented */ >> + int unsegmented = 1; >> >> /* Restriction for v1.0: No zeroization support */ >> const int zeroized = 0; >> @@ -163,14 +163,18 @@ odp_buffer_pool_t odp_buffer_pool_create(const char >> *name, >> case ODP_BUFFER_TYPE_ANY: >> headroom = ODP_CONFIG_PACKET_HEADROOM; >> tailroom = ODP_CONFIG_PACKET_TAILROOM; >> - if (unsegmented) >> + unsegmented = 0; /* Packets are segmented by default */ >> + if (unsegmented) { >> blk_size = ODP_ALIGN_ROUNDUP( >> headroom + params->buf_size + tailroom, >> buf_align); >> - else >> + } else { >> blk_size = ODP_ALIGN_ROUNDUP( >> headroom + params->buf_size + tailroom, >> ODP_CONFIG_PACKET_BUF_LEN_MIN); >> + if (blk_size > ODP_CONFIG_PACKET_BUF_LEN_MAX) >> + return ODP_BUFFER_POOL_INVALID; >> + } >> buf_stride = params->buf_type == ODP_BUFFER_TYPE_PACKET ? >> sizeof(odp_packet_hdr_stride) : >> sizeof(odp_any_hdr_stride); >> >> >
On 12/18/2014 04:31 PM, Bill Fischofer wrote: > It's one patch broken into two parts [1/2] [2/2]. Is this not correct? > I'm trying to break things up since folks don't want to see single patches. Split looks correct. First patch prepares pktio code for segmented buffers, and the second patch enables them. I'd keep them as a separate patches. Just found one issue with the patch. Crypto test tries to allocate a buffer pool with 32k buffers. Allocation fails because it is more than a limit ODP_CONFIG_PACKET_BUF_LEN_MAX.
The reason why it fails is because we're no longer supporting unsegmented packet pools. I could change the patch to fall back to unsegmented support if the requested buf_size > ODP_CONFIG_PACKET_BUF_LEN_MAX. I'll post a v2 that does that so we don't unnecessarily break tests in 0.6.0. It would be helpful to have Petri's views on this. The real solution, I believe, is to allow the caller of odp_buffer_pool_create() to specify whether they need unsegmented buffers, as we had originally. On Thu, Dec 18, 2014 at 9:35 AM, Taras Kondratiuk < taras.kondratiuk@linaro.org> wrote: > > On 12/18/2014 04:31 PM, Bill Fischofer wrote: > >> It's one patch broken into two parts [1/2] [2/2]. Is this not correct? >> I'm trying to break things up since folks don't want to see single >> patches. >> > > Split looks correct. First patch prepares pktio code for segmented > buffers, and the second patch enables them. I'd keep them as a separate > patches. > > Just found one issue with the patch. Crypto test tries to allocate a > buffer pool with 32k buffers. Allocation fails because it is more than a > limit ODP_CONFIG_PACKET_BUF_LEN_MAX. >
diff --git a/platform/linux-generic/odp_buffer_pool.c b/platform/linux-generic/odp_buffer_pool.c index 48be24f..6b0e34b 100644 --- a/platform/linux-generic/odp_buffer_pool.c +++ b/platform/linux-generic/odp_buffer_pool.c @@ -119,8 +119,8 @@ odp_buffer_pool_t odp_buffer_pool_create(const char *name, if (params == NULL) return ODP_BUFFER_POOL_INVALID; - /* Restriction for v1.0: All buffers are unsegmented */ - const int unsegmented = 1; + /* Restriction for v1.0: All non-packet buffers are unsegmented */ + int unsegmented = 1; /* Restriction for v1.0: No zeroization support */ const int zeroized = 0; @@ -163,14 +163,18 @@ odp_buffer_pool_t odp_buffer_pool_create(const char *name, case ODP_BUFFER_TYPE_ANY: headroom = ODP_CONFIG_PACKET_HEADROOM; tailroom = ODP_CONFIG_PACKET_TAILROOM; - if (unsegmented) + unsegmented = 0; /* Packets are segmented by default */ + if (unsegmented) { blk_size = ODP_ALIGN_ROUNDUP( headroom + params->buf_size + tailroom, buf_align); - else + } else { blk_size = ODP_ALIGN_ROUNDUP( headroom + params->buf_size + tailroom, ODP_CONFIG_PACKET_BUF_LEN_MIN); + if (blk_size > ODP_CONFIG_PACKET_BUF_LEN_MAX) + return ODP_BUFFER_POOL_INVALID; + } buf_stride = params->buf_type == ODP_BUFFER_TYPE_PACKET ? sizeof(odp_packet_hdr_stride) : sizeof(odp_any_hdr_stride);
Enable segmentation in packet buffer pools by default Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> --- platform/linux-generic/odp_buffer_pool.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)