| Message ID | 1461773622-26062-2-git-send-email-zoltan.kiss@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 27 April 2016 at 21:43, Zoltan Kiss <zoltan.kiss@linaro.org> 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 and hence it would be better if the error is percolated to the application so that he can take some intelligent action ( either reconfigure classification and send the packet again or free the packet ). 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. Regards, Bala > > Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org> > --- > 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 > >
On 28/04/16 10:29, Bala Manoharan wrote: > > On 27 April 2016 at 21:43, Zoltan Kiss <zoltan.kiss@linaro.org > <mailto:zoltan.kiss@linaro.org>> 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 <zoltan.kiss@linaro.org > <mailto:zoltan.kiss@linaro.org>> > --- > 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 > >
On 28 April 2016 at 21:50, Zoltan Kiss <zoltan.kiss@linaro.org> wrote: > > > On 28/04/16 10:29, Bala Manoharan wrote: > >> >> On 27 April 2016 at 21:43, Zoltan Kiss <zoltan.kiss@linaro.org >> <mailto:zoltan.kiss@linaro.org>> 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 <zoltan.kiss@linaro.org >> <mailto:zoltan.kiss@linaro.org>> >> >> --- >> 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 >> >> >>
On 28/04/16 18:08, Bala Manoharan wrote: > > On 28 April 2016 at 21:50, Zoltan Kiss <zoltan.kiss@linaro.org > <mailto:zoltan.kiss@linaro.org>> wrote: > > > > On 28/04/16 10:29, Bala Manoharan wrote: > > > On 27 April 2016 at 21:43, Zoltan Kiss <zoltan.kiss@linaro.org > <mailto:zoltan.kiss@linaro.org> > <mailto:zoltan.kiss@linaro.org <mailto:zoltan.kiss@linaro.org>>> > 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. > > - (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." > 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) > > > > > > 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 <zoltan.kiss@linaro.org > <mailto:zoltan.kiss@linaro.org> > <mailto:zoltan.kiss@linaro.org > <mailto:zoltan.kiss@linaro.org>>> > > --- > 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 > > >
On 29 April 2016 at 18:28, Zoltan Kiss <zoltan.kiss@linaro.org> wrote: > > > On 28/04/16 18:08, Bala Manoharan wrote: > >> >> On 28 April 2016 at 21:50, Zoltan Kiss <zoltan.kiss@linaro.org >> <mailto:zoltan.kiss@linaro.org>> wrote: >> >> >> >> On 28/04/16 10:29, Bala Manoharan wrote: >> >> >> On 27 April 2016 at 21:43, Zoltan Kiss <zoltan.kiss@linaro.org >> <mailto:zoltan.kiss@linaro.org> >> <mailto:zoltan.kiss@linaro.org <mailto:zoltan.kiss@linaro.org>>> >> 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. The configuration error is a serious issue and applications will be interested to raise alarms for this 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, 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. 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. We can discuss this further in today's ARCH call. 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 <zoltan.kiss@linaro.org >> <mailto:zoltan.kiss@linaro.org> >> <mailto:zoltan.kiss@linaro.org >> >> <mailto:zoltan.kiss@linaro.org>>> >> >> --- >> 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 >> >> >> >>
On 02/05/16 09:56, Bala Manoharan wrote: > > On 29 April 2016 at 18:28, Zoltan Kiss <zoltan.kiss@linaro.org > <mailto:zoltan.kiss@linaro.org>> wrote: > > > > On 28/04/16 18:08, Bala Manoharan wrote: > > > On 28 April 2016 at 21:50, Zoltan Kiss <zoltan.kiss@linaro.org > <mailto:zoltan.kiss@linaro.org> > <mailto:zoltan.kiss@linaro.org <mailto:zoltan.kiss@linaro.org>>> > wrote: > > > > On 28/04/16 10:29, Bala Manoharan wrote: > > > On 27 April 2016 at 21:43, Zoltan Kiss > <zoltan.kiss@linaro.org <mailto:zoltan.kiss@linaro.org> > <mailto:zoltan.kiss@linaro.org > <mailto:zoltan.kiss@linaro.org>> > <mailto:zoltan.kiss@linaro.org > <mailto:zoltan.kiss@linaro.org> <mailto:zoltan.kiss@linaro.org > <mailto:zoltan.kiss@linaro.org>>>> > 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 <zoltan.kiss@linaro.org > <mailto:zoltan.kiss@linaro.org> > <mailto:zoltan.kiss@linaro.org > <mailto:zoltan.kiss@linaro.org>> > <mailto:zoltan.kiss@linaro.org > <mailto:zoltan.kiss@linaro.org> > > <mailto:zoltan.kiss@linaro.org > <mailto:zoltan.kiss@linaro.org>>>> > > --- > 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 > > > >
On 3 May 2016 at 19:26, Zoltan Kiss <zoltan.kiss@linaro.org> wrote: > > > On 02/05/16 09:56, Bala Manoharan wrote: > >> >> On 29 April 2016 at 18:28, Zoltan Kiss <zoltan.kiss@linaro.org >> <mailto:zoltan.kiss@linaro.org>> wrote: >> >> >> >> On 28/04/16 18:08, Bala Manoharan wrote: >> >> >> On 28 April 2016 at 21:50, Zoltan Kiss <zoltan.kiss@linaro.org >> <mailto:zoltan.kiss@linaro.org> >> <mailto:zoltan.kiss@linaro.org <mailto:zoltan.kiss@linaro.org>>> >> wrote: >> >> >> >> On 28/04/16 10:29, Bala Manoharan wrote: >> >> >> On 27 April 2016 at 21:43, Zoltan Kiss >> <zoltan.kiss@linaro.org <mailto:zoltan.kiss@linaro.org> >> <mailto:zoltan.kiss@linaro.org >> <mailto:zoltan.kiss@linaro.org>> >> <mailto:zoltan.kiss@linaro.org >> <mailto:zoltan.kiss@linaro.org> <mailto:zoltan.kiss@linaro.org >> <mailto:zoltan.kiss@linaro.org>>>> >> 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. > Different pktios have different configurations and there are hardware configuration which can configure that a packet needs to be sent even if it gets delayed. specifically why traffic manager system has drop probability/ drop eligibility set for packets and RED DROP/NO-DROP decision. By enabling loopback a traffic manager system gets connected to the classifier system through loop. And we are adding the mechanism to set any possible pktio interface as loopback and hence IMO we should not generalise the behaviour for all pktios. The pktio is a virtual concept and you can create a pktio without having a physical interface. This linux-generic implementations will be used only by platforms which do not have a HW classifier and a simple example could be IPC (inter process communication) implementation which want to send packet through classifier to distribute the packets. The risk of delaying a packet is something which application might take for certain packets and we cannot generalise the behaviour and drop every packets as explained above. Lets discuss this further on ARCH call tomorrow. Regards, Bala > > > >> 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 <zoltan.kiss@linaro.org >> <mailto:zoltan.kiss@linaro.org> >> <mailto:zoltan.kiss@linaro.org >> <mailto:zoltan.kiss@linaro.org>> >> <mailto:zoltan.kiss@linaro.org >> <mailto:zoltan.kiss@linaro.org> >> >> <mailto:zoltan.kiss@linaro.org >> <mailto:zoltan.kiss@linaro.org>>>> >> >> --- >> 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 >> >> >> >> >>
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;
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. Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org> --- platform/linux-generic/odp_classification.c | 21 ++++++++++++--------- platform/linux-generic/pktio/loop.c | 1 - 2 files changed, 12 insertions(+), 10 deletions(-)