On 28/04/16 10:29, Bala Manoharan wrote:
On 27 April 2016 at 21:43, Zoltan Kiss <[email protected]
<mailto:[email protected]>> wrote:
Move release to _odp_packet_classifier(), because caller has no way to
know if new_pkt were allocated. In that case there would be a double
free
on pkt, and new_pkt would be leaked.
I am little skeptical about this classifier module freeing up the packet
since the error in CoS can only happen during a wrong configuration by
the application
I'm not sure this is wrong config:
- cos == NULL shouldn't happen, my understanding is that not setting a
default queue is impossible (although I haven't found it spelled out in
the header files explicitly), so this is an internal error
- (cos->s.queue == NULL || cos->s.pool == NULL) means the packet should
be dropped. Although I don't know how drop_policy should affect this,
it's not clear from the description of odp_cls_cos_create()
- (entry == NULL) is impossible at the moment, but as you said, in the
future other callers might make this mistake, so better to be prepared.
- odp_packet_copy() fails means we can't put this packet where it
supposed to arrive, I think it would be very confusing for the
application to receive it then
- queue_enq() failure is again a sign of serious issues
and hence it would be better if the error is percolated
to the application
The error itself is returned through odp_pktin_recv() when you apply the
next two patch, but as we discussed if classification is enabled it
shouldn't be called directly. pktin_poll() will notice that though, and
print a message, but it won't enqueue when queue_enq() fails, see the
3rd patch.
so that he can take some intelligent action ( either
reconfigure classification and send the packet again or free the packet ).
This function is only used on the receive side, I'm not sure what you
mean by "send the packet again".
Regarding the issue of freeing up the packet when a new packet is
created by the classification module, we can better change the signature
of _odp_packet_classifier() API to receive odp_packet_t as a pointer and
update the pointer with new packet.
The main reason I am against freeing up the packet inside classification
module is that this function is currently called only by loopback device
but in future development it could be called from other sources also and
hence it is better if freeing of the packet during error is done by the
caller rather than classification module.
I think we should implement this when we get there, and see it fit
better. Currently it's broken anyway.
Plus, why would it be better to release by the caller, rather than on
the spot? As I explained above, these error cases seems to be major
internal issues, none of them seem like the caller can do anything.
Regards,
Bala
Signed-off-by: Zoltan Kiss <[email protected]
<mailto:[email protected]>>
---
platform/linux-generic/odp_classification.c | 21 ++++++++++++---------
platform/linux-generic/pktio/loop.c | 1 -
2 files changed, 12 insertions(+), 10 deletions(-)
diff --git a/platform/linux-generic/odp_classification.c
b/platform/linux-generic/odp_classification.c
index 3a18a78..a1466fd 100644
--- a/platform/linux-generic/odp_classification.c
+++ b/platform/linux-generic/odp_classification.c
@@ -734,22 +734,20 @@ int _odp_packet_classifier(pktio_entry_t
*entry, odp_packet_t pkt)
odp_packet_t new_pkt;
uint8_t *pkt_addr;
- if (entry == NULL)
+ if (entry == NULL) {
+ odp_packet_free(pkt);
return -1;
+ }
pkt_hdr = odp_packet_hdr(pkt);
pkt_addr = odp_packet_data(pkt);
/* Matching PMR and selecting the CoS for the packet*/
cos = pktio_select_cos(entry, pkt_addr, pkt_hdr);
- if (cos == NULL)
- return -1;
-
- if (cos->s.pool == NULL)
- return -1;
-
- if (cos->s.queue == NULL)
+ if (cos == NULL || cos->s.queue == NULL || cos->s.pool ==
NULL) {
+ odp_packet_free(pkt);
return -1;
+ }
if (odp_packet_pool(pkt) != cos->s.pool->s.pool_hdl) {
new_pkt = odp_packet_copy(pkt,
cos->s.pool->s.pool_hdl);
@@ -762,7 +760,12 @@ int _odp_packet_classifier(pktio_entry_t
*entry, odp_packet_t pkt)
/* Enqueuing the Packet based on the CoS */
queue = cos->s.queue;
- return queue_enq(queue,
odp_buf_to_hdr((odp_buffer_t)new_pkt), 0);
+ if (queue_enq(queue, odp_buf_to_hdr((odp_buffer_t)new_pkt),
0)) {
+ odp_packet_free(new_pkt);
+ return -1;
+ } else {
+ return 0;
+ }
}
cos_t *pktio_select_cos(pktio_entry_t *entry, const uint8_t *pkt_addr,
diff --git a/platform/linux-generic/pktio/loop.c
b/platform/linux-generic/pktio/loop.c
index f6a8c1d..676e98b 100644
--- a/platform/linux-generic/pktio/loop.c
+++ b/platform/linux-generic/pktio/loop.c
@@ -76,7 +76,6 @@ static int loopback_recv(pktio_entry_t
*pktio_entry, odp_packet_t pkts[],
} else {
pktio_entry->s.stats.in_errors +=
odp_packet_len(pkt);
- odp_packet_free(pkt);
}
}
nbr = j;
--
1.9.1
_______________________________________________
lng-odp mailing list
[email protected]
https://lists.linaro.org/mailman/listinfo/lng-odp
