| Message ID | 1426792468-12598-4-git-send-email-zoltan.kiss@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 03/19/15 22:14, Zoltan Kiss wrote: > Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org> > --- > lib/netdev-odp.c | 21 +++++++++++---------- > 1 file changed, 11 insertions(+), 10 deletions(-) > > diff --git a/lib/netdev-odp.c b/lib/netdev-odp.c > index 7a0780e..2a04d24 100644 > --- a/lib/netdev-odp.c > +++ b/lib/netdev-odp.c > @@ -133,10 +133,10 @@ odp_class_init(void) > > /* create packet & ofpbuf pool */ > > - params.buf_size = SHM_PKT_POOL_BUF_SIZE + SHM_OFPBUF_POOL_BUF_SIZE; > - params.buf_align = ODP_CACHE_LINE_SIZE; > - params.num_bufs = SHM_PKT_POOL_NUM_BUFS; > - params.buf_type = ODP_BUFFER_TYPE_PACKET; > + params.pkt.len= SHM_PKT_POOL_BUF_SIZE + SHM_OFPBUF_POOL_BUF_SIZE; > + params.pkt.seg_len = 0; > + params.pkt.num = SHM_PKT_POOL_NUM_BUFS; > + params.type = ODP_POOL_PACKET; > > pool = odp_pool_create("packet_pool", ODP_SHM_NULL, ¶ms); > > @@ -147,8 +147,8 @@ odp_class_init(void) > odp_pool_print(pool); > > /* Allocate all the packets from the pool and initialize ofpbuf part */ > - pkts = malloc(sizeof(odp_packet_t) * params.num_bufs); > - for (i=0; i < params.num_bufs; ++i) { > + pkts = malloc(sizeof(odp_packet_t) * params.pkt.num); > + for (i=0; i < params.pkt.num; ++i) { > struct dpif_packet *packet; > char *start; > pkts[i] = odp_packet_alloc(pool, 0); > @@ -169,7 +169,7 @@ odp_class_init(void) > } > > /* Free our packets up */ > - for (i=0; i < params.num_bufs; i++) > + for (i=0; i < params.pkt.num; i++) > odp_packet_free(pkts[i]); > free(pkts); What are you doing here? Checking that all packets in the pool can be allocated? I think odp api / implementation should take care about that. Maxim. > > @@ -177,8 +177,9 @@ odp_class_init(void) > > /* create pool for structures */ > > - params.buf_size = SHM_STRUCT_POOL_BUF_SIZE; > - params.num_bufs = SHM_STRUCT_POOL_NUM_BUFS; > + params.type = ODP_POOL_BUFFER; > + params.buf.size = SHM_STRUCT_POOL_BUF_SIZE; > + params.buf.num = SHM_STRUCT_POOL_NUM_BUFS; > > struct_pool = odp_pool_create("struct_pool", ODP_SHM_NULL, ¶ms); > > @@ -240,7 +241,7 @@ netdev_odp_construct(struct netdev *netdev_) > goto out_err; > } > > - netdev->max_frame_len = info.params.buf_size; > + netdev->max_frame_len = info.params.pkt.len; > > ovs_mutex_init(&netdev->mutex); >
On Fri, Mar 20, 2015 at 5:49 PM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 03/19/15 22:14, Zoltan Kiss wrote: >> >> Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org> >> --- >> lib/netdev-odp.c | 21 +++++++++++---------- >> 1 file changed, 11 insertions(+), 10 deletions(-) >> >> diff --git a/lib/netdev-odp.c b/lib/netdev-odp.c >> index 7a0780e..2a04d24 100644 >> --- a/lib/netdev-odp.c >> +++ b/lib/netdev-odp.c >> @@ -133,10 +133,10 @@ odp_class_init(void) >> /* create packet & ofpbuf pool */ >> - params.buf_size = SHM_PKT_POOL_BUF_SIZE + SHM_OFPBUF_POOL_BUF_SIZE; >> - params.buf_align = ODP_CACHE_LINE_SIZE; >> - params.num_bufs = SHM_PKT_POOL_NUM_BUFS; >> - params.buf_type = ODP_BUFFER_TYPE_PACKET; >> + params.pkt.len= SHM_PKT_POOL_BUF_SIZE + SHM_OFPBUF_POOL_BUF_SIZE; >> + params.pkt.seg_len = 0; >> + params.pkt.num = SHM_PKT_POOL_NUM_BUFS; >> + params.type = ODP_POOL_PACKET; >> pool = odp_pool_create("packet_pool", ODP_SHM_NULL, ¶ms); >> @@ -147,8 +147,8 @@ odp_class_init(void) >> odp_pool_print(pool); >> /* Allocate all the packets from the pool and initialize ofpbuf >> part */ >> - pkts = malloc(sizeof(odp_packet_t) * params.num_bufs); >> - for (i=0; i < params.num_bufs; ++i) { >> + pkts = malloc(sizeof(odp_packet_t) * params.pkt.num); >> + for (i=0; i < params.pkt.num; ++i) { >> struct dpif_packet *packet; >> char *start; >> pkts[i] = odp_packet_alloc(pool, 0); >> @@ -169,7 +169,7 @@ odp_class_init(void) >> } >> /* Free our packets up */ >> - for (i=0; i < params.num_bufs; i++) >> + for (i=0; i < params.pkt.num; i++) >> odp_packet_free(pkts[i]); >> free(pkts); > > > What are you doing here? Checking that all packets in the pool can be > allocated? I think odp api / implementation should take care about that. Don't ignore the context, the previous series that makes performance tweaks explains why everything in this function is needed! Between line 147 and 169 there is code that is not included in this patch. > > Maxim. > >> @@ -177,8 +177,9 @@ odp_class_init(void) >> /* create pool for structures */ >> - params.buf_size = SHM_STRUCT_POOL_BUF_SIZE; >> - params.num_bufs = SHM_STRUCT_POOL_NUM_BUFS; >> + params.type = ODP_POOL_BUFFER; >> + params.buf.size = SHM_STRUCT_POOL_BUF_SIZE; >> + params.buf.num = SHM_STRUCT_POOL_NUM_BUFS; >> struct_pool = odp_pool_create("struct_pool", ODP_SHM_NULL, >> ¶ms); >> @@ -240,7 +241,7 @@ netdev_odp_construct(struct netdev *netdev_) >> goto out_err; >> } >> - netdev->max_frame_len = info.params.buf_size; >> + netdev->max_frame_len = info.params.pkt.len; >> ovs_mutex_init(&netdev->mutex); >> > > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp
On 20/03/15 15:49, Maxim Uvarov wrote: > On 03/19/15 22:14, Zoltan Kiss wrote: >> Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org> >> --- >> lib/netdev-odp.c | 21 +++++++++++---------- >> 1 file changed, 11 insertions(+), 10 deletions(-) >> >> diff --git a/lib/netdev-odp.c b/lib/netdev-odp.c >> index 7a0780e..2a04d24 100644 >> --- a/lib/netdev-odp.c >> +++ b/lib/netdev-odp.c >> @@ -133,10 +133,10 @@ odp_class_init(void) >> /* create packet & ofpbuf pool */ >> - params.buf_size = SHM_PKT_POOL_BUF_SIZE + SHM_OFPBUF_POOL_BUF_SIZE; >> - params.buf_align = ODP_CACHE_LINE_SIZE; >> - params.num_bufs = SHM_PKT_POOL_NUM_BUFS; >> - params.buf_type = ODP_BUFFER_TYPE_PACKET; >> + params.pkt.len= SHM_PKT_POOL_BUF_SIZE + SHM_OFPBUF_POOL_BUF_SIZE; >> + params.pkt.seg_len = 0; >> + params.pkt.num = SHM_PKT_POOL_NUM_BUFS; >> + params.type = ODP_POOL_PACKET; >> pool = odp_pool_create("packet_pool", ODP_SHM_NULL, ¶ms); >> @@ -147,8 +147,8 @@ odp_class_init(void) >> odp_pool_print(pool); >> /* Allocate all the packets from the pool and initialize ofpbuf >> part */ >> - pkts = malloc(sizeof(odp_packet_t) * params.num_bufs); >> - for (i=0; i < params.num_bufs; ++i) { >> + pkts = malloc(sizeof(odp_packet_t) * params.pkt.num); >> + for (i=0; i < params.pkt.num; ++i) { >> struct dpif_packet *packet; >> char *start; >> pkts[i] = odp_packet_alloc(pool, 0); >> @@ -169,7 +169,7 @@ odp_class_init(void) >> } >> /* Free our packets up */ >> - for (i=0; i < params.num_bufs; i++) >> + for (i=0; i < params.pkt.num; i++) >> odp_packet_free(pkts[i]); >> free(pkts); > > What are you doing here? Checking that all packets in the pool can be > allocated? I think odp api / implementation should take care about that. No, I need to allocate all the packets from the pool and preset some fields in the packet metadata area, where OVS store it's ofpbuf structure. Setting it after every receive has a perf penalty. See "netdev-odp: allocate packet metadata in the same buffer as the packet itself" in the previous series. That's where this originally comes from. > > Maxim. > >> @@ -177,8 +177,9 @@ odp_class_init(void) >> /* create pool for structures */ >> - params.buf_size = SHM_STRUCT_POOL_BUF_SIZE; >> - params.num_bufs = SHM_STRUCT_POOL_NUM_BUFS; >> + params.type = ODP_POOL_BUFFER; >> + params.buf.size = SHM_STRUCT_POOL_BUF_SIZE; >> + params.buf.num = SHM_STRUCT_POOL_NUM_BUFS; >> struct_pool = odp_pool_create("struct_pool", ODP_SHM_NULL, ¶ms); >> @@ -240,7 +241,7 @@ netdev_odp_construct(struct netdev *netdev_) >> goto out_err; >> } >> - netdev->max_frame_len = info.params.buf_size; >> + netdev->max_frame_len = info.params.pkt.len; >> ovs_mutex_init(&netdev->mutex); > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp
On 20/03/15 16:42, Zoltan Kiss wrote: > No, I need to allocate all the packets from the pool and preset some > fields in the packet metadata area, where OVS store it's ofpbuf > structure. Setting it after every receive has a perf penalty. See > "netdev-odp: allocate packet metadata in the same buffer as the packet > itself" in the previous series. That's where this originally comes from. Forgot to mention: this is something DPDK has facility for, you can add initializer functions for the buffer pool create function there. We discussed with Bill that something like that would be good for ODP.
Actually, ODP already has this concept. It's called user metadata and was to be a feature of ODP pools (the structures to support this are actually already part of linux-generic). We deferred it from ODP v1.0 for practical reasons, but this is something we should revisit. Let's put this on the list of architecture things to discuss next week. I'd like to understand the specifics of the OVS need so we can be sure to get the best application match to the final ODP proposal. On Fri, Mar 20, 2015 at 11:43 AM, Zoltan Kiss <zoltan.kiss@linaro.org> wrote: > > > On 20/03/15 16:42, Zoltan Kiss wrote: > >> No, I need to allocate all the packets from the pool and preset some >> fields in the packet metadata area, where OVS store it's ofpbuf >> structure. Setting it after every receive has a perf penalty. See >> "netdev-odp: allocate packet metadata in the same buffer as the packet >> itself" in the previous series. That's where this originally comes from. >> > > Forgot to mention: this is something DPDK has facility for, you can add > initializer functions for the buffer pool create function there. We > discussed with Bill that something like that would be good for ODP. > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp >
On 03/20/15 19:42, Zoltan Kiss wrote: > > > On 20/03/15 15:49, Maxim Uvarov wrote: >> On 03/19/15 22:14, Zoltan Kiss wrote: >>> Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org> >>> --- >>> lib/netdev-odp.c | 21 +++++++++++---------- >>> 1 file changed, 11 insertions(+), 10 deletions(-) >>> >>> diff --git a/lib/netdev-odp.c b/lib/netdev-odp.c >>> index 7a0780e..2a04d24 100644 >>> --- a/lib/netdev-odp.c >>> +++ b/lib/netdev-odp.c >>> @@ -133,10 +133,10 @@ odp_class_init(void) >>> /* create packet & ofpbuf pool */ >>> - params.buf_size = SHM_PKT_POOL_BUF_SIZE + >>> SHM_OFPBUF_POOL_BUF_SIZE; >>> - params.buf_align = ODP_CACHE_LINE_SIZE; >>> - params.num_bufs = SHM_PKT_POOL_NUM_BUFS; >>> - params.buf_type = ODP_BUFFER_TYPE_PACKET; >>> + params.pkt.len= SHM_PKT_POOL_BUF_SIZE + SHM_OFPBUF_POOL_BUF_SIZE; >>> + params.pkt.seg_len = 0; >>> + params.pkt.num = SHM_PKT_POOL_NUM_BUFS; >>> + params.type = ODP_POOL_PACKET; >>> pool = odp_pool_create("packet_pool", ODP_SHM_NULL, ¶ms); >>> @@ -147,8 +147,8 @@ odp_class_init(void) >>> odp_pool_print(pool); >>> /* Allocate all the packets from the pool and initialize ofpbuf >>> part */ >>> - pkts = malloc(sizeof(odp_packet_t) * params.num_bufs); >>> - for (i=0; i < params.num_bufs; ++i) { >>> + pkts = malloc(sizeof(odp_packet_t) * params.pkt.num); >>> + for (i=0; i < params.pkt.num; ++i) { >>> struct dpif_packet *packet; >>> char *start; >>> pkts[i] = odp_packet_alloc(pool, 0); >>> @@ -169,7 +169,7 @@ odp_class_init(void) >>> } >>> /* Free our packets up */ >>> - for (i=0; i < params.num_bufs; i++) >>> + for (i=0; i < params.pkt.num; i++) >>> odp_packet_free(pkts[i]); >>> free(pkts); >> >> What are you doing here? Checking that all packets in the pool can be >> allocated? I think odp api / implementation should take care about that. > No, I need to allocate all the packets from the pool and preset some > fields in the packet metadata area, where OVS store it's ofpbuf > structure. Setting it after every receive has a perf penalty. See > "netdev-odp: allocate packet metadata in the same buffer as the packet > itself" in the previous series. That's where this originally comes from. Not sure if it's portable solution. Will it work on KS2 for example? Maxim. >> >> Maxim. >> >>> @@ -177,8 +177,9 @@ odp_class_init(void) >>> /* create pool for structures */ >>> - params.buf_size = SHM_STRUCT_POOL_BUF_SIZE; >>> - params.num_bufs = SHM_STRUCT_POOL_NUM_BUFS; >>> + params.type = ODP_POOL_BUFFER; >>> + params.buf.size = SHM_STRUCT_POOL_BUF_SIZE; >>> + params.buf.num = SHM_STRUCT_POOL_NUM_BUFS; >>> struct_pool = odp_pool_create("struct_pool", ODP_SHM_NULL, >>> ¶ms); >>> @@ -240,7 +241,7 @@ netdev_odp_construct(struct netdev *netdev_) >>> goto out_err; >>> } >>> - netdev->max_frame_len = info.params.buf_size; >>> + netdev->max_frame_len = info.params.pkt.len; >>> ovs_mutex_init(&netdev->mutex); >> >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> http://lists.linaro.org/mailman/listinfo/lng-odp
The APIs are intended to be portable. Whether linux-generic code is useful for other implementations is separate question. On Fri, Mar 20, 2015 at 12:03 PM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 03/20/15 19:42, Zoltan Kiss wrote: > >> >> >> On 20/03/15 15:49, Maxim Uvarov wrote: >> >>> On 03/19/15 22:14, Zoltan Kiss wrote: >>> >>>> Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org> >>>> --- >>>> lib/netdev-odp.c | 21 +++++++++++---------- >>>> 1 file changed, 11 insertions(+), 10 deletions(-) >>>> >>>> diff --git a/lib/netdev-odp.c b/lib/netdev-odp.c >>>> index 7a0780e..2a04d24 100644 >>>> --- a/lib/netdev-odp.c >>>> +++ b/lib/netdev-odp.c >>>> @@ -133,10 +133,10 @@ odp_class_init(void) >>>> /* create packet & ofpbuf pool */ >>>> - params.buf_size = SHM_PKT_POOL_BUF_SIZE + SHM_OFPBUF_POOL_BUF_SIZE; >>>> - params.buf_align = ODP_CACHE_LINE_SIZE; >>>> - params.num_bufs = SHM_PKT_POOL_NUM_BUFS; >>>> - params.buf_type = ODP_BUFFER_TYPE_PACKET; >>>> + params.pkt.len= SHM_PKT_POOL_BUF_SIZE + SHM_OFPBUF_POOL_BUF_SIZE; >>>> + params.pkt.seg_len = 0; >>>> + params.pkt.num = SHM_PKT_POOL_NUM_BUFS; >>>> + params.type = ODP_POOL_PACKET; >>>> pool = odp_pool_create("packet_pool", ODP_SHM_NULL, ¶ms); >>>> @@ -147,8 +147,8 @@ odp_class_init(void) >>>> odp_pool_print(pool); >>>> /* Allocate all the packets from the pool and initialize ofpbuf >>>> part */ >>>> - pkts = malloc(sizeof(odp_packet_t) * params.num_bufs); >>>> - for (i=0; i < params.num_bufs; ++i) { >>>> + pkts = malloc(sizeof(odp_packet_t) * params.pkt.num); >>>> + for (i=0; i < params.pkt.num; ++i) { >>>> struct dpif_packet *packet; >>>> char *start; >>>> pkts[i] = odp_packet_alloc(pool, 0); >>>> @@ -169,7 +169,7 @@ odp_class_init(void) >>>> } >>>> /* Free our packets up */ >>>> - for (i=0; i < params.num_bufs; i++) >>>> + for (i=0; i < params.pkt.num; i++) >>>> odp_packet_free(pkts[i]); >>>> free(pkts); >>>> >>> >>> What are you doing here? Checking that all packets in the pool can be >>> allocated? I think odp api / implementation should take care about that. >>> >> No, I need to allocate all the packets from the pool and preset some >> fields in the packet metadata area, where OVS store it's ofpbuf structure. >> Setting it after every receive has a perf penalty. See "netdev-odp: >> allocate packet metadata in the same buffer as the packet itself" in the >> previous series. That's where this originally comes from. >> > > Not sure if it's portable solution. Will it work on KS2 for example? > > Maxim. > > >>> Maxim. >>> >>> @@ -177,8 +177,9 @@ odp_class_init(void) >>>> /* create pool for structures */ >>>> - params.buf_size = SHM_STRUCT_POOL_BUF_SIZE; >>>> - params.num_bufs = SHM_STRUCT_POOL_NUM_BUFS; >>>> + params.type = ODP_POOL_BUFFER; >>>> + params.buf.size = SHM_STRUCT_POOL_BUF_SIZE; >>>> + params.buf.num = SHM_STRUCT_POOL_NUM_BUFS; >>>> struct_pool = odp_pool_create("struct_pool", ODP_SHM_NULL, >>>> ¶ms); >>>> @@ -240,7 +241,7 @@ netdev_odp_construct(struct netdev *netdev_) >>>> goto out_err; >>>> } >>>> - netdev->max_frame_len = info.params.buf_size; >>>> + netdev->max_frame_len = info.params.pkt.len; >>>> ovs_mutex_init(&netdev->mutex); >>>> >>> >>> >>> _______________________________________________ >>> lng-odp mailing list >>> lng-odp@lists.linaro.org >>> http://lists.linaro.org/mailman/listinfo/lng-odp >>> >> > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp >
diff --git a/lib/netdev-odp.c b/lib/netdev-odp.c index 7a0780e..2a04d24 100644 --- a/lib/netdev-odp.c +++ b/lib/netdev-odp.c @@ -133,10 +133,10 @@ odp_class_init(void) /* create packet & ofpbuf pool */ - params.buf_size = SHM_PKT_POOL_BUF_SIZE + SHM_OFPBUF_POOL_BUF_SIZE; - params.buf_align = ODP_CACHE_LINE_SIZE; - params.num_bufs = SHM_PKT_POOL_NUM_BUFS; - params.buf_type = ODP_BUFFER_TYPE_PACKET; + params.pkt.len= SHM_PKT_POOL_BUF_SIZE + SHM_OFPBUF_POOL_BUF_SIZE; + params.pkt.seg_len = 0; + params.pkt.num = SHM_PKT_POOL_NUM_BUFS; + params.type = ODP_POOL_PACKET; pool = odp_pool_create("packet_pool", ODP_SHM_NULL, ¶ms); @@ -147,8 +147,8 @@ odp_class_init(void) odp_pool_print(pool); /* Allocate all the packets from the pool and initialize ofpbuf part */ - pkts = malloc(sizeof(odp_packet_t) * params.num_bufs); - for (i=0; i < params.num_bufs; ++i) { + pkts = malloc(sizeof(odp_packet_t) * params.pkt.num); + for (i=0; i < params.pkt.num; ++i) { struct dpif_packet *packet; char *start; pkts[i] = odp_packet_alloc(pool, 0); @@ -169,7 +169,7 @@ odp_class_init(void) } /* Free our packets up */ - for (i=0; i < params.num_bufs; i++) + for (i=0; i < params.pkt.num; i++) odp_packet_free(pkts[i]); free(pkts); @@ -177,8 +177,9 @@ odp_class_init(void) /* create pool for structures */ - params.buf_size = SHM_STRUCT_POOL_BUF_SIZE; - params.num_bufs = SHM_STRUCT_POOL_NUM_BUFS; + params.type = ODP_POOL_BUFFER; + params.buf.size = SHM_STRUCT_POOL_BUF_SIZE; + params.buf.num = SHM_STRUCT_POOL_NUM_BUFS; struct_pool = odp_pool_create("struct_pool", ODP_SHM_NULL, ¶ms); @@ -240,7 +241,7 @@ netdev_odp_construct(struct netdev *netdev_) goto out_err; } - netdev->max_frame_len = info.params.buf_size; + netdev->max_frame_len = info.params.pkt.len; ovs_mutex_init(&netdev->mutex);
Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org> --- lib/netdev-odp.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)