On Wed, Dec 17, 2014 at 03:20:06PM +0000, Maxim Uvarov wrote: > On 12/17/2014 05:45 PM, Stuart Haslam wrote: > > On Wed, Dec 17, 2014 at 01:56:56PM +0000, Maxim Uvarov wrote: > >> odp_pktio_inq_remdef() calls odp_pktio_inq_setdef() with > >> ODP_QUEUE_INVALID. odp_pktio_inq_setdef should set up > >> ODP_QUEUE_INVALID to pktio instead of returning with error. > >> > >> Signed-off-by: Maxim Uvarov <[email protected]> > >> --- > >> platform/linux-generic/odp_packet_io.c | 9 ++++++++- > >> 1 file changed, 8 insertions(+), 1 deletion(-) > >> > >> diff --git a/platform/linux-generic/odp_packet_io.c > >> b/platform/linux-generic/odp_packet_io.c > >> index 3ca8100..4e54658 100644 > >> --- a/platform/linux-generic/odp_packet_io.c > >> +++ b/platform/linux-generic/odp_packet_io.c > >> @@ -367,9 +367,16 @@ int odp_pktio_inq_setdef(odp_pktio_t id, odp_queue_t > >> queue) > >> pktio_entry_t *pktio_entry = get_pktio_entry(id); > >> queue_entry_t *qentry; > >> > >> - if (pktio_entry == NULL || queue == ODP_QUEUE_INVALID) > >> + if (pktio_entry == NULL) > >> return -1; > >> > >> + if (queue == ODP_QUEUE_INVALID) { > >> + lock_entry(pktio_entry); > >> + pktio_entry->s.inq_default = ODP_QUEUE_INVALID; > >> + unlock_entry(pktio_entry); > > You could've avoided this duplicated chunk of code by just shuffling the > > existing code around and adding an early return for an INVALID queue. > > > >> + return 0; > >> + } > >> + > >> qentry = queue_to_qentry(queue); > >> > >> if (qentry->s.type != ODP_QUEUE_TYPE_PKTIN) > >> -- > >> 1.8.5.1.163.gd7aced9 > >> > > My main concern though is that setdef and remdef are not symmetric. > > setdef not only sets the queue as the default for the pktio (which is > > only used when the application calls getdef) but also sets the pktin for > > the queue and schedules it. With this change, after calling remdef the > > previous default inq is still being scheduled, is that what the caller > > expected and how do they now remove it from the scheduler if required? > > > In that case current odp_pktio_inq_remdef() is wrong. >
Agreed, it's completely broken and likely has been since it was introduced, but nobody ever called it before. > It has to: > 1. take default queue: > queue = pktio_entry->s.inq_default; > > 2. get entry: > qentry = queue_to_qentry(queue); > > 3. Remove it from scheduler: > qentry->s.status = QUEUE_STATUS_FREE or QUEUE_STATUS_DESTROYED (need to > check code, unclear). > > 4. Return. > > Right? To give it sensible semantics without changing any other APIs it would need to do the opposite of setdef, so change inq_default too and just deschedule the queue rather than free/destroy it. I'm not sure it's possible to do that though, due to the hook in odp_schedule() for ODP_QUEUE_TYPE_PKTIN queues. -- Stuart. _______________________________________________ lng-odp mailing list [email protected] http://lists.linaro.org/mailman/listinfo/lng-odp
