| Message ID | 1460055213-29479-2-git-send-email-zoltan.kiss@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hi, Comments inline... Regards, Bala On 8 April 2016 at 00:23, Zoltan Kiss <zoltan.kiss@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.kiss@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. > 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 classification module drops a packet only on specific cases either on error CoS or when the packet pool is full. 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. 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 > >
On 11/04/16 05:01, Bala Manoharan wrote: > Hi, > > Comments inline... > > Regards, > Bala > > On 8 April 2016 at 00:23, Zoltan Kiss <zoltan.kiss@linaro.org > <mailto:zoltan.kiss@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.kiss@linaro.org > <mailto:zoltan.kiss@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. > > > 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. > 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 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. > > 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 > >
The original behavior of loopback_recv() is incorrect then, because it gives back the failed packets to the caller in pkts[], some of the might had been already released. On 11/04/16 05:01, Bala Manoharan wrote: > 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]); > --
Regards, Bala On 12 April 2016 at 17:34, Zoltan Kiss <zoltan.kiss@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.kiss@linaro.org >> <mailto:zoltan.kiss@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.kiss@linaro.org >> <mailto:zoltan.kiss@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 (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. > > > > >> 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 >> >> >>
On 12 April 2016 at 17:41, Zoltan Kiss <zoltan.kiss@linaro.org> wrote: > The original behavior of loopback_recv() is incorrect then, because it > gives back the failed packets to the caller in pkts[], some of the might > had been already released. The failed packets are not released by classification module. Maybe we need to change the behaviour of the loopback_recv() function to free the packet when there is a classification error and possibly print an error message since the application pretty much can not do anything else in that scenario. Regards, Bala > > > On 11/04/16 05:01, Bala Manoharan wrote: > >> 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]); >> -- >> >
On 13/04/16 13:40, Bala Manoharan wrote: > > > Regards, > Bala > > On 12 April 2016 at 17:34, Zoltan Kiss <zoltan.kiss@linaro.org > <mailto:zoltan.kiss@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.kiss@linaro.org > <mailto:zoltan.kiss@linaro.org> > <mailto:zoltan.kiss@linaro.org <mailto:zoltan.kiss@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.kiss@linaro.org > <mailto:zoltan.kiss@linaro.org> > <mailto:zoltan.kiss@linaro.org > <mailto:zoltan.kiss@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 > > >
On 13 April 2016 at 19:07, Zoltan Kiss <zoltan.kiss@linaro.org> wrote: > > > On 13/04/16 13:40, Bala Manoharan wrote: > >> >> >> Regards, >> Bala >> >> On 12 April 2016 at 17:34, Zoltan Kiss <zoltan.kiss@linaro.org >> <mailto:zoltan.kiss@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.kiss@linaro.org >> <mailto:zoltan.kiss@linaro.org> >> <mailto:zoltan.kiss@linaro.org <mailto:zoltan.kiss@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.kiss@linaro.org >> <mailto:zoltan.kiss@linaro.org> >> <mailto:zoltan.kiss@linaro.org >> >> <mailto:zoltan.kiss@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. Yes. Your point is valid and for uniformity we can remove odp_packet_free() from those two scenario and do the free in loopback_recv() function. Regards, Bala > > > >> >> >> >> >> 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 >> >> >> >>
Hi, I've sent a new patch, I just wanted to mention that I couldn't find this behavior described in user-guide-cls.adoc Zoli On 13/04/16 13:40, Bala Manoharan wrote: > > > 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.
On 13 April 2016 at 19:51, Zoltan Kiss <zoltan.kiss@linaro.org> wrote: > Hi, > > I've sent a new patch, I just wanted to mention that I couldn't find this > behavior described in user-guide-cls.adoc > I will add these information to user-guide document. Regards, Bala > > Zoli > > On 13/04/16 13:40, Bala Manoharan wrote: > >> >> >> 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. >> >
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; 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; + } } 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]);
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.kiss@linaro.org> --- platform/linux-generic/odp_classification.c | 9 +++++++-- platform/linux-generic/pktio/loop.c | 2 +- 2 files changed, 8 insertions(+), 3 deletions(-)