On 13/04/16 13:40, Bala Manoharan wrote:
Regards,
Bala
On 12 April 2016 at 17:34, Zoltan Kiss <zoltan.k...@linaro.org
<mailto:zoltan.k...@linaro.org>> wrote:
On 11/04/16 05:01, Bala Manoharan wrote:
Hi,
Comments inline...
Regards,
Bala
On 8 April 2016 at 00:23, Zoltan Kiss <zoltan.k...@linaro.org
<mailto:zoltan.k...@linaro.org>
<mailto:zoltan.k...@linaro.org <mailto:zoltan.k...@linaro.org>>>
wrote:
The callers are only interested whether there is a CoS or
not. It is not
an error if there isn't, so it has a positive return value. Any
other case
needs to release the packet, which is not handled after calling
queue_enq().
Signed-off-by: Zoltan Kiss <zoltan.k...@linaro.org
<mailto:zoltan.k...@linaro.org>
<mailto:zoltan.k...@linaro.org
<mailto:zoltan.k...@linaro.org>>>
---
platform/linux-generic/odp_classification.c | 9 +++++++--
platform/linux-generic/pktio/loop.c | 2 +-
2 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/platform/linux-generic/odp_classification.c
b/platform/linux-generic/odp_classification.c
index f5e4673..4f2974b 100644
--- a/platform/linux-generic/odp_classification.c
+++ b/platform/linux-generic/odp_classification.c
@@ -743,7 +743,7 @@ int
_odp_packet_classifier(pktio_entry_t *entry,
odp_packet_t pkt)
/* Matching PMR and selecting the CoS for the packet*/
cos = pktio_select_cos(entry, pkt_addr, pkt_hdr);
if (cos == NULL)
- return -1;
+ return 1;
The scenario of NULL Cos will occur only whether there is an invalid
configuration done by the application. If no other CoS matches the
packet then default CoS will be applied, this NULL will occur
only if
there is no defaultCoS configured and hence it is better to
return the
error to the calling function so that a proper action could be
taken.
Ok, then we should free the packet here as well.
IMO, we should free the packet at the loopback device function. so that
whenever the _odp_packet_classifier() function returns an error value
then the packet has to be freed by the calling function.
If you just look at the next few lines of this code, you'll see that
this function releases the buffer in two error scenario.
if (cos->s.pool == NULL) {
odp_packet_free(pkt);
@@ -766,7 +766,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;
+ }
IMO packets should be freed only by the receiving module rather
than by
the classifier in this specific error.
The only user, the loopback devices doesn't do so, it only adjusts
the statistics. That is where the memory leaks btw.
Okay. Then we have to modify the loopback device function to free the
packet when there is an error in _odp_packet_classifier() function.
The classification module drops a
packet only on specific cases either on error CoS or when the packet
pool is full.
Could you point me to the documentation where this behavior is
described?
The scenario is described in classification users-guide documentation.
Pls let me know if we need to add additional information in the document.
The idea of returning an error value is that the caller
function knows that the packet is not enqueued and it has to
either free
the packet or check the configuration.
Currently we return -1, and the packet buffer is either released or
not. The only caller doesn't care anyway, and I don't think it could
do much more anyway, other than printing a big error message and
probably abort.
Current behaviour is that when there is an error in classification, we
return -1and it means that the packet has not be enqueued into
classification queue and the buffer has not been freed.
There are two error scenarios when that's not true, see above.
You had mentioned that this patch is required to prevent a
memory leak,
can you please elaborate on the scenario when a leak is observed?
}
int packet_classifier(odp_pktio_t pktio, odp_packet_t pkt)
diff --git a/platform/linux-generic/pktio/loop.c
b/platform/linux-generic/pktio/loop.c
index 0ea6d0e..5ed4ca9 100644
--- a/platform/linux-generic/pktio/loop.c
+++ b/platform/linux-generic/pktio/loop.c
@@ -70,7 +70,7 @@ static int loopback_recv(pktio_entry_t
*pktio_entry, odp_packet_t pkts[],
pkt_hdr = odp_packet_hdr(pkt);
packet_parse_reset(pkt_hdr);
packet_parse_l2(pkt_hdr);
- if (0 >
_odp_packet_classifier(pktio_entry,
pkt)) {
+ if
(_odp_packet_classifier(pktio_entry, pkt)
== 1) {
pkts[j++] = pkt;
pktio_entry->s.stats.in_octets +=
odp_packet_len(pkts[i]);
--
1.9.1
_______________________________________________
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp
