Re: [lng-odp] [PATCH v1] Scheduler packet input integration optimization
Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page:
platform/linux-generic/odp_schedule_basic.c
@@ -720,9 +727,32 @@ static inline int poll_pktin(uint32_t qi)
if (num_pktin == 0)
sched_cb_pktio_stop_finalize(pktio_index);
+
+ return num;
}
- return num;
+ if (atomic) {
Comment:
That makes sense.
> Petri Savolainen(psavol) wrote:
> Line is was not changed but moved. Anyway, added name in v2.
>> Petri Savolainen(psavol) wrote:
>> ODP_DBG() added in v2
>>> Petri Savolainen(psavol) wrote:
>>> I wanted to be conservative and not change synchronization of parallel
>>> queues yet. I'll do another patch on top, so that it's easy to undo
>>> parallel optimization later if necessary.
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
Worth an `ODP_DBG()` here? At minimum I'd think we'd want to capture some
sort of statistic for these drops.
Same comment for the rest of the similar drops in this commit.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> If we relax this to cover atomic and parallel queues then this would
> simply be:
> ```
> int use_stash = !queue_is_ordered(qi);
> ```
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> Is it really necessary to restrict this optimization to atomic queues?
>> Ordered obviously cannot be stashed, but parallel queues make no
>> ordering guarantees so accelerating them like this would also seem
>> reasonable. In that case the `atomic` variable to this function would be
>> better named something like `use_stash` .
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> Might be nice to say which interface wasn't started here for debug
>>> purposes since many could be in play.
https://github.com/Linaro/odp/pull/504#discussion_r171839812
updated_at 2018-03-02 12:55:36
Re: [lng-odp] [PATCH v1] Scheduler packet input integration optimization
Petri Savolainen(psavol) replied on github web page:
platform/linux-generic/odp_packet_io.c
line 117
@@ -720,12 +727,29 @@ int sched_cb_pktin_poll_one(int pktio_index,
return num_rx;
}
-int sched_cb_pktin_poll(int pktio_index, int num_queue, int index[])
+int sched_cb_pktin_poll(int pktio_index, int pktin_index,
+ odp_buffer_hdr_t *hdr_tbl[], int num)
+{
+ pktio_entry_t *entry = pktio_entry_by_index(pktio_index);
+ int state = entry->s.state;
+
+ if (odp_unlikely(state != PKTIO_STATE_STARTED)) {
+ if (state < PKTIO_STATE_ACTIVE ||
+ state == PKTIO_STATE_STOP_PENDING)
+ return -1;
+
+ ODP_DBG("interface not started\n");
Comment:
Line is was not changed but moved. Anyway, added name in v2.
> Petri Savolainen(psavol) wrote:
> ODP_DBG() added in v2
>> Petri Savolainen(psavol) wrote:
>> I wanted to be conservative and not change synchronization of parallel
>> queues yet. I'll do another patch on top, so that it's easy to undo parallel
>> optimization later if necessary.
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> Worth an `ODP_DBG()` here? At minimum I'd think we'd want to capture some
>>> sort of statistic for these drops.
>>>
>>> Same comment for the rest of the similar drops in this commit.
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
If we relax this to cover atomic and parallel queues then this would
simply be:
```
int use_stash = !queue_is_ordered(qi);
```
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> Is it really necessary to restrict this optimization to atomic queues?
> Ordered obviously cannot be stashed, but parallel queues make no ordering
> guarantees so accelerating them like this would also seem reasonable. In
> that case the `atomic` variable to this function would be better named
> something like `use_stash` .
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> Might be nice to say which interface wasn't started here for debug
>> purposes since many could be in play.
https://github.com/Linaro/odp/pull/504#discussion_r171823872
updated_at 2018-03-02 11:24:09
Re: [lng-odp] [PATCH v1] Scheduler packet input integration optimization
Petri Savolainen(psavol) replied on github web page:
platform/linux-generic/odp_packet_io.c
line 50
@@ -638,8 +638,20 @@ static odp_buffer_hdr_t *pktin_dequeue(queue_t q_int)
if (pkts <= 0)
return NULL;
- if (pkts > 1)
- queue_fn->enq_multi(q_int, _tbl[1], pkts - 1);
+ if (pkts > 1) {
+ int num_enq;
+ int num = pkts - 1;
+
+ num_enq = queue_fn->enq_multi(q_int, _tbl[1], num);
+
+ if (odp_unlikely(num_enq < num)) {
+ if (odp_unlikely(num_enq < 0))
+ num_enq = 0;
+
+ buffer_free_multi(_tbl[num_enq + 1], num - num_enq);
Comment:
ODP_DBG() added in v2
> Petri Savolainen(psavol) wrote:
> I wanted to be conservative and not change synchronization of parallel queues
> yet. I'll do another patch on top, so that it's easy to undo parallel
> optimization later if necessary.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> Worth an `ODP_DBG()` here? At minimum I'd think we'd want to capture some
>> sort of statistic for these drops.
>>
>> Same comment for the rest of the similar drops in this commit.
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> If we relax this to cover atomic and parallel queues then this would simply
>>> be:
>>> ```
>>> int use_stash = !queue_is_ordered(qi);
>>> ```
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
Is it really necessary to restrict this optimization to atomic queues?
Ordered obviously cannot be stashed, but parallel queues make no ordering
guarantees so accelerating them like this would also seem reasonable. In
that case the `atomic` variable to this function would be better named
something like `use_stash` .
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> Might be nice to say which interface wasn't started here for debug
> purposes since many could be in play.
https://github.com/Linaro/odp/pull/504#discussion_r171823373
updated_at 2018-03-02 11:21:39
Re: [lng-odp] [PATCH v1] Scheduler packet input integration optimization
Petri Savolainen(psavol) replied on github web page:
platform/linux-generic/odp_schedule_basic.c
@@ -720,9 +727,32 @@ static inline int poll_pktin(uint32_t qi)
if (num_pktin == 0)
sched_cb_pktio_stop_finalize(pktio_index);
+
+ return num;
}
- return num;
+ if (atomic) {
Comment:
I wanted to be conservative and not change synchronization of parallel queues
yet. I'll do another patch on top, so that it's easy to undo parallel
optimization later if necessary.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> Worth an `ODP_DBG()` here? At minimum I'd think we'd want to capture some
> sort of statistic for these drops.
>
> Same comment for the rest of the similar drops in this commit.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> If we relax this to cover atomic and parallel queues then this would simply
>> be:
>> ```
>> int use_stash = !queue_is_ordered(qi);
>> ```
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> Is it really necessary to restrict this optimization to atomic queues?
>>> Ordered obviously cannot be stashed, but parallel queues make no ordering
>>> guarantees so accelerating them like this would also seem reasonable. In
>>> that case the `atomic` variable to this function would be better named
>>> something like `use_stash` .
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
Might be nice to say which interface wasn't started here for debug
purposes since many could be in play.
https://github.com/Linaro/odp/pull/504#discussion_r171783094
updated_at 2018-03-02 08:01:52
Re: [lng-odp] [PATCH v1] Scheduler packet input integration optimization
Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page:
platform/linux-generic/odp_packet_io.c
line 117
@@ -720,12 +727,29 @@ int sched_cb_pktin_poll_one(int pktio_index,
return num_rx;
}
-int sched_cb_pktin_poll(int pktio_index, int num_queue, int index[])
+int sched_cb_pktin_poll(int pktio_index, int pktin_index,
+ odp_buffer_hdr_t *hdr_tbl[], int num)
+{
+ pktio_entry_t *entry = pktio_entry_by_index(pktio_index);
+ int state = entry->s.state;
+
+ if (odp_unlikely(state != PKTIO_STATE_STARTED)) {
+ if (state < PKTIO_STATE_ACTIVE ||
+ state == PKTIO_STATE_STOP_PENDING)
+ return -1;
+
+ ODP_DBG("interface not started\n");
Comment:
Might be nice to say which interface wasn't started here for debug purposes
since many could be in play.
https://github.com/Linaro/odp/pull/504#discussion_r171690660
updated_at 2018-03-01 22:37:20
Re: [lng-odp] [PATCH v1] Scheduler packet input integration optimization
Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page:
platform/linux-generic/odp_packet_io.c
line 50
@@ -638,8 +638,20 @@ static odp_buffer_hdr_t *pktin_dequeue(queue_t q_int)
if (pkts <= 0)
return NULL;
- if (pkts > 1)
- queue_fn->enq_multi(q_int, _tbl[1], pkts - 1);
+ if (pkts > 1) {
+ int num_enq;
+ int num = pkts - 1;
+
+ num_enq = queue_fn->enq_multi(q_int, _tbl[1], num);
+
+ if (odp_unlikely(num_enq < num)) {
+ if (odp_unlikely(num_enq < 0))
+ num_enq = 0;
+
+ buffer_free_multi(_tbl[num_enq + 1], num - num_enq);
Comment:
Worth an `ODP_DBG()` here? At minimum I'd think we'd want to capture some sort
of statistic for these drops.
Same comment for the rest of the similar drops in this commit.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> If we relax this to cover atomic and parallel queues then this would simply
> be:
> ```
> int use_stash = !queue_is_ordered(qi);
> ```
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> Is it really necessary to restrict this optimization to atomic queues?
>> Ordered obviously cannot be stashed, but parallel queues make no ordering
>> guarantees so accelerating them like this would also seem reasonable. In
>> that case the `atomic` variable to this function would be better named
>> something like `use_stash` .
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> Might be nice to say which interface wasn't started here for debug purposes
>>> since many could be in play.
https://github.com/Linaro/odp/pull/504#discussion_r171715495
updated_at 2018-03-01 22:37:20
Re: [lng-odp] [PATCH v1] Scheduler packet input integration optimization
Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page:
platform/linux-generic/odp_schedule_basic.c
@@ -805,17 +835,26 @@ static inline int do_schedule_grp(odp_queue_t *out_queue,
odp_event_t out_ev[],
* priorities. Stop scheduling queue when pktio
* has been stopped. */
if (pktin) {
- int num_pkt = poll_pktin(qi);
+ int atomic = queue_is_atomic(qi);
Comment:
If we relax this to cover atomic and parallel queues then this would simply be:
```
int use_stash = !queue_is_ordered(qi);
```
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> Is it really necessary to restrict this optimization to atomic queues?
> Ordered obviously cannot be stashed, but parallel queues make no ordering
> guarantees so accelerating them like this would also seem reasonable. In that
> case the `atomic` variable to this function would be better named something
> like `use_stash` .
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> Might be nice to say which interface wasn't started here for debug purposes
>> since many could be in play.
https://github.com/Linaro/odp/pull/504#discussion_r171692278
updated_at 2018-03-01 22:37:20
Re: [lng-odp] [PATCH v1] Scheduler packet input integration optimization
Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page:
platform/linux-generic/odp_schedule_basic.c
@@ -720,9 +727,32 @@ static inline int poll_pktin(uint32_t qi)
if (num_pktin == 0)
sched_cb_pktio_stop_finalize(pktio_index);
+
+ return num;
}
- return num;
+ if (atomic) {
Comment:
Is it really necessary to restrict this optimization to atomic queues? Ordered
obviously cannot be stashed, but parallel queues make no ordering guarantees so
accelerating them like this would also seem reasonable. In that case the
`atomic` variable to this function would be better named something like
`use_stash` .
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> Might be nice to say which interface wasn't started here for debug purposes
> since many could be in play.
https://github.com/Linaro/odp/pull/504#discussion_r171691919
updated_at 2018-03-01 22:37:20
