On 28 April 2016 at 21:50, Zoltan Kiss <[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]>> 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. - (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 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. > > 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. > > > >> 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. 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]>> >> >> --- >> 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
