On 02/05/16 09:56, Bala Manoharan wrote:
On 29 April 2016 at 18:28, Zoltan Kiss <[email protected]
<mailto:[email protected]>> wrote:
On 28/04/16 18:08, Bala Manoharan wrote:
On 28 April 2016 at 21:50, Zoltan Kiss <[email protected]
<mailto:[email protected]>
<mailto:[email protected] <mailto:[email protected]>>>
wrote:
On 28/04/16 10:29, Bala Manoharan wrote:
On 27 April 2016 at 21:43, Zoltan Kiss
<[email protected] <mailto:[email protected]>
<mailto:[email protected]
<mailto:[email protected]>>
<mailto:[email protected]
<mailto:[email protected]> <mailto:[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
Whenever application creates a pktio with enable_classifier
field set in
odp_pktio_params_t then it should call the function
odp_pktio_default_cos_set() to set the default CoS with the
pktio. The
scenario of cos == NULL will only occur if a pktio is created with
classification enabled and default CoS not set and hence this is a
configuration error.
So you say in this case these packets should end up on
entry->s.in_queue[index[idx]].queue in pktin_poll()? Why? Or do you
have something else in mind?
I would say it's better if different pktio's handle this the same way.
I am saying we should not generalise the handling for all the different
pktio, we need to move this handling to the pktio level so that if in
future some pktio wants to change the behaviour it will be possible.
I don't think our users would be happy if different pktios would handle
the same error in different ways. Do you have an example in mind where
it is necessary?
The
configuration error is a serious issue and applications will be
interested to raise alarms for this scenario.
They will be, we'll return an error, which will trigger a log message in
pktin_poll(). This won't change.
Do you want a separate error message for the cos == NULL scenario?
- (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
pool or queue error could also be transient the pool could get empty
This is not an overload issue, as odp_cls_cos_create() says:
"@note ODP_QUEUE_INVALID and ODP_POOL_INVALID are valid values for
queue and pool associated with a class of service and when any one
of these values are configured as INVALID then the packets assigned
to the CoS gets dropped."
The packets are dropped only when pool handle is invalid,
The above quoted text contradicts that, do you plan to change it with a
patch to API-NEXT?
but in the
case of odp_packet_copy() or queue_enq() error the pool or queue is not
invalid it is because the pool does not have sufficient buffers.
If you look into queue_enq(), it returns non-zero if the queue status is
screwed up, it has nothing to do with pool exhaustion.
Packet
pools are allocated and deallocated during packet handling and if the
rate of incoming packets is higher for a small amount of time the packet
pool might get empty and odp_packet_copy() API will fail and in this
scenario the pktio can decide either to drop the packet or hold the
packet and resend it again. This becomes more important now coz we have
added the ability to make any interface as loopback and not just
interface with name "loop"
after sometime depending upon the load in the network. Imagine a
scenario where the application sends a decrypted packet into the
loopback device for classification and would not want the packet
to be
dropped immediately on transient pool exhaustion but would prefer
delayed sending of the packet.
Or do you mean when odp_packet_copy() fails? (This second part
suggest that) Still, how would that happen? Currently this call
graph looks like this:
schedule()
pktin_poll() [currently it can place returned packets on
entry->s.in_queue[index[idx]].queue]
odp_pktin_recv()
[pktio's receive function]
[_odp_packet_classifier() or _odp_packet_cls_enq(), depending on
your pktio]
At which level should we handle this and how?
(And I think we still should drop the packets in the other error
cases I've mentioned, in an unified way for all pktios)
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".
The application might want some specific packets not be dropped
under
any scenario even when there is a rate limiting and in case of
loopback
mode the classifier module is attached directly to the output as if
there is a physical loopback and hence in that scenario the loopback
device might want to send the packet again after sometime when
the pool
or queue gets empty rather than dropping it immediately.
So you say the loopback receive function should get back the packets
when odp_packet_copy() fails, and send it again to loopback?
That's a hard question whether its a good idea or not. You can count
on the fact that the packets are never lost there, but you are
risking recovery time during an overload scenario (or maybe even a
deadlock, if nothing else releases buffer from the pool, you'll keep
enqueuing the same packets)
This risk of recovery time is dependent on the type of packet and pktio
interface and we can not generalise this in classifier. Incase of a
control packet or configuration packet the application might not want
the packet to be dropped.
But note, the application can't change this behavior of the platform.
These packets will be either dropped or re-enqueued until the pool gets
ready (which has a risk, as I explained above)
We can discuss this further in today's ARCH all.
Yesterday was a bank holiday in the UK, we can discuss it tomorrow.
Regards,
Bala
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.
Why don't we implement it now rather than change it in the future?
The caller in this case is also internal functions (eg socket,
loopback)
as _odp_packet_classifier() is an internal function and cannot be
directly called by the application, so the idea here is whether the
packets are freed up by _odp_packet_classifier() internal
function or
its caller the socket function both of which are internal
functions in
the linux-generic. I prefer the latter so that any future
devices could
be handled gracefully.
I think it's better if all pktio's handle these errors in the same
way, and by default it is by dropping packets. It's easier to
enforce if we handle that in _odp_packet_classifier() and
_odp_packet_cls_enq().
The only scenario when I can imagine to pass back the packet is when
odp_packet_copy() fails, but even then re-sending might not be a
good idea, as I explained above.
Most importantly the point we both are discussing is very narrow and
code changes are minimal, If you feel strongly then you can go
ahead and
implement your way now and we can make any change in the future.
I don't
want to drag this discussion and delay this patch any longer.
Regards,
Bala
Regards,
Bala
Signed-off-by: Zoltan Kiss <[email protected]
<mailto:[email protected]>
<mailto:[email protected]
<mailto:[email protected]>>
<mailto:[email protected]
<mailto:[email protected]>
<mailto:[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
