| Message ID | 1460557200-19922-1-git-send-email-zoltan.kiss@linaro.org |
|---|---|
| State | Accepted |
| Commit | 3e25103e2a5ada5a5a7594bc6f32eb4ea48f4a46 |
| Headers | show |
Reviewed-by: Balasubramanian Manoharan <bala.manoharan@linaro.org> On 13 April 2016 at 19:50, Zoltan Kiss <zoltan.kiss@linaro.org> wrote: > In case of error the 'pkt' should be released by the calling function. > Currently loopback gives it back to the receiver and report it as success > in the stats. > > Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org> > --- > > v2: handle release in caller instead, and adjust stats. > > platform/linux-generic/odp_classification.c | 10 +++------- > platform/linux-generic/pktio/loop.c | 9 ++++++--- > 2 files changed, 9 insertions(+), 10 deletions(-) > > diff --git a/platform/linux-generic/odp_classification.c > b/platform/linux-generic/odp_classification.c > index 8522023..3a18a78 100644 > --- a/platform/linux-generic/odp_classification.c > +++ b/platform/linux-generic/odp_classification.c > @@ -745,21 +745,17 @@ int _odp_packet_classifier(pktio_entry_t *entry, > odp_packet_t pkt) > if (cos == NULL) > return -1; > > - if (cos->s.pool == NULL) { > - odp_packet_free(pkt); > + if (cos->s.pool == NULL) > return -1; > - } > > - if (cos->s.queue == NULL) { > - odp_packet_free(pkt); > + if (cos->s.queue == NULL) > 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); > - odp_packet_free(pkt); > if (new_pkt == ODP_PACKET_INVALID) > return -1; > + odp_packet_free(pkt); > } else { > new_pkt = pkt; > } > diff --git a/platform/linux-generic/pktio/loop.c > b/platform/linux-generic/pktio/loop.c > index 0ea6d0e..f6a8c1d 100644 > --- a/platform/linux-generic/pktio/loop.c > +++ b/platform/linux-generic/pktio/loop.c > @@ -70,10 +70,13 @@ 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)) { > - pkts[j++] = pkt; > + if (!_odp_packet_classifier(pktio_entry, pkt)) { > pktio_entry->s.stats.in_octets += > - odp_packet_len(pkts[i]); > + odp_packet_len(pkt); > + } else { > + pktio_entry->s.stats.in_errors += > + odp_packet_len(pkt); > + odp_packet_free(pkt); > } > } > nbr = j; > -- > 1.9.1 > >
Maxim, what's the planned time to apply this? On 15/04/16 06:31, Bala Manoharan wrote: > Reviewed-by: Balasubramanian Manoharan <bala.manoharan@linaro.org > <mailto:bala.manoharan@linaro.org>> > > On 13 April 2016 at 19:50, Zoltan Kiss <zoltan.kiss@linaro.org > <mailto:zoltan.kiss@linaro.org>> wrote: > > In case of error the 'pkt' should be released by the calling function. > Currently loopback gives it back to the receiver and report it as > success > in the stats. > > Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org > <mailto:zoltan.kiss@linaro.org>> > --- > > v2: handle release in caller instead, and adjust stats. > > platform/linux-generic/odp_classification.c | 10 +++------- > platform/linux-generic/pktio/loop.c | 9 ++++++--- > 2 files changed, 9 insertions(+), 10 deletions(-) > > diff --git a/platform/linux-generic/odp_classification.c > b/platform/linux-generic/odp_classification.c > index 8522023..3a18a78 100644 > --- a/platform/linux-generic/odp_classification.c > +++ b/platform/linux-generic/odp_classification.c > @@ -745,21 +745,17 @@ int _odp_packet_classifier(pktio_entry_t > *entry, odp_packet_t pkt) > if (cos == NULL) > return -1; > > - if (cos->s.pool == NULL) { > - odp_packet_free(pkt); > + if (cos->s.pool == NULL) > return -1; > - } > > - if (cos->s.queue == NULL) { > - odp_packet_free(pkt); > + if (cos->s.queue == NULL) > 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); > - odp_packet_free(pkt); > if (new_pkt == ODP_PACKET_INVALID) > return -1; > + odp_packet_free(pkt); > } else { > new_pkt = pkt; > } > diff --git a/platform/linux-generic/pktio/loop.c > b/platform/linux-generic/pktio/loop.c > index 0ea6d0e..f6a8c1d 100644 > --- a/platform/linux-generic/pktio/loop.c > +++ b/platform/linux-generic/pktio/loop.c > @@ -70,10 +70,13 @@ 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)) { > - pkts[j++] = pkt; > + if (!_odp_packet_classifier(pktio_entry, pkt)) { > pktio_entry->s.stats.in_octets += > - odp_packet_len(pkts[i]); > + odp_packet_len(pkt); > + } else { > + pktio_entry->s.stats.in_errors += > + odp_packet_len(pkt); > + odp_packet_free(pkt); > } > } > nbr = j; > -- > 1.9.1 > >
On 04/21/16 21:08, Zoltan Kiss wrote: > Maxim, what's the planned time to apply this? > just after current 1.9 tag. (tomorrow) Maxim. > On 15/04/16 06:31, Bala Manoharan wrote: >> Reviewed-by: Balasubramanian Manoharan <bala.manoharan@linaro.org >> <mailto:bala.manoharan@linaro.org>> >> >> On 13 April 2016 at 19:50, Zoltan Kiss <zoltan.kiss@linaro.org >> <mailto:zoltan.kiss@linaro.org>> wrote: >> >> In case of error the 'pkt' should be released by the calling >> function. >> Currently loopback gives it back to the receiver and report it as >> success >> in the stats. >> >> Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org >> <mailto:zoltan.kiss@linaro.org>> >> --- >> >> v2: handle release in caller instead, and adjust stats. >> >> platform/linux-generic/odp_classification.c | 10 +++------- >> platform/linux-generic/pktio/loop.c | 9 ++++++--- >> 2 files changed, 9 insertions(+), 10 deletions(-) >> >> diff --git a/platform/linux-generic/odp_classification.c >> b/platform/linux-generic/odp_classification.c >> index 8522023..3a18a78 100644 >> --- a/platform/linux-generic/odp_classification.c >> +++ b/platform/linux-generic/odp_classification.c >> @@ -745,21 +745,17 @@ int _odp_packet_classifier(pktio_entry_t >> *entry, odp_packet_t pkt) >> if (cos == NULL) >> return -1; >> >> - if (cos->s.pool == NULL) { >> - odp_packet_free(pkt); >> + if (cos->s.pool == NULL) >> return -1; >> - } >> >> - if (cos->s.queue == NULL) { >> - odp_packet_free(pkt); >> + if (cos->s.queue == NULL) >> 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); >> - odp_packet_free(pkt); >> if (new_pkt == ODP_PACKET_INVALID) >> return -1; >> + odp_packet_free(pkt); >> } else { >> new_pkt = pkt; >> } >> diff --git a/platform/linux-generic/pktio/loop.c >> b/platform/linux-generic/pktio/loop.c >> index 0ea6d0e..f6a8c1d 100644 >> --- a/platform/linux-generic/pktio/loop.c >> +++ b/platform/linux-generic/pktio/loop.c >> @@ -70,10 +70,13 @@ 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)) { >> - pkts[j++] = pkt; >> + if (!_odp_packet_classifier(pktio_entry, >> pkt)) { >> pktio_entry->s.stats.in_octets += >> - odp_packet_len(pkts[i]); >> + odp_packet_len(pkt); >> + } else { >> + pktio_entry->s.stats.in_errors += >> + odp_packet_len(pkt); >> + odp_packet_free(pkt); >> } >> } >> nbr = j; >> -- >> 1.9.1 >> >>
Merged, Maxim. On 04/21/16 21:08, Zoltan Kiss wrote: > Maxim, what's the planned time to apply this? > > On 15/04/16 06:31, Bala Manoharan wrote: >> Reviewed-by: Balasubramanian Manoharan <bala.manoharan@linaro.org >> <mailto:bala.manoharan@linaro.org>> >> >> On 13 April 2016 at 19:50, Zoltan Kiss <zoltan.kiss@linaro.org >> <mailto:zoltan.kiss@linaro.org>> wrote: >> >> In case of error the 'pkt' should be released by the calling >> function. >> Currently loopback gives it back to the receiver and report it as >> success >> in the stats. >> >> Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org >> <mailto:zoltan.kiss@linaro.org>> >> --- >> >> v2: handle release in caller instead, and adjust stats. >> >> platform/linux-generic/odp_classification.c | 10 +++------- >> platform/linux-generic/pktio/loop.c | 9 ++++++--- >> 2 files changed, 9 insertions(+), 10 deletions(-) >> >> diff --git a/platform/linux-generic/odp_classification.c >> b/platform/linux-generic/odp_classification.c >> index 8522023..3a18a78 100644 >> --- a/platform/linux-generic/odp_classification.c >> +++ b/platform/linux-generic/odp_classification.c >> @@ -745,21 +745,17 @@ int _odp_packet_classifier(pktio_entry_t >> *entry, odp_packet_t pkt) >> if (cos == NULL) >> return -1; >> >> - if (cos->s.pool == NULL) { >> - odp_packet_free(pkt); >> + if (cos->s.pool == NULL) >> return -1; >> - } >> >> - if (cos->s.queue == NULL) { >> - odp_packet_free(pkt); >> + if (cos->s.queue == NULL) >> 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); >> - odp_packet_free(pkt); >> if (new_pkt == ODP_PACKET_INVALID) >> return -1; >> + odp_packet_free(pkt); >> } else { >> new_pkt = pkt; >> } >> diff --git a/platform/linux-generic/pktio/loop.c >> b/platform/linux-generic/pktio/loop.c >> index 0ea6d0e..f6a8c1d 100644 >> --- a/platform/linux-generic/pktio/loop.c >> +++ b/platform/linux-generic/pktio/loop.c >> @@ -70,10 +70,13 @@ 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)) { >> - pkts[j++] = pkt; >> + if (!_odp_packet_classifier(pktio_entry, >> pkt)) { >> pktio_entry->s.stats.in_octets += >> - odp_packet_len(pkts[i]); >> + odp_packet_len(pkt); >> + } else { >> + pktio_entry->s.stats.in_errors += >> + odp_packet_len(pkt); >> + odp_packet_free(pkt); >> } >> } >> nbr = j; >> -- >> 1.9.1 >> >>
Hi Bala, While looking at this, I realized I don't understand something: we pass the received packets to classification, and it enqueues them to queues. But we also return the number of received packets, along with the handles to those packets, which requires a quite different behavior from the caller of odp_pktin_recv(): it can't actually use those handles, as those packets are on a queue already. Am I missing something? Or should we mention this in the function description? Regards, Zoltan On 13/04/16 15:20, Zoltan Kiss wrote: > diff --git a/platform/linux-generic/pktio/loop.c b/platform/linux-generic/pktio/loop.c > index 0ea6d0e..f6a8c1d 100644 > --- a/platform/linux-generic/pktio/loop.c > +++ b/platform/linux-generic/pktio/loop.c > @@ -70,10 +70,13 @@ 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)) { > - pkts[j++] = pkt; > + if (!_odp_packet_classifier(pktio_entry, pkt)) { > pktio_entry->s.stats.in_octets += > - odp_packet_len(pkts[i]); > + odp_packet_len(pkt); > + } else { > + pktio_entry->s.stats.in_errors += > + odp_packet_len(pkt); > + odp_packet_free(pkt); > } > } > nbr = j;
On 22 April 2016 at 20:28, Zoltan Kiss <zoltan.kiss@linaro.org> wrote: > Hi Bala, > > While looking at this, I realized I don't understand something: we pass > the received packets to classification, and it enqueues them to queues. But > we also return the number of received packets, along with the handles to > those packets, which requires a quite different behavior from the caller of > odp_pktin_recv(): it can't actually use those handles, as those packets are > on a queue already. Am I missing something? Or should we mention this in > the function description? > The function only returns the handles of those packets which were not en-queued by classification, basically it checks the return value of _odp_packet_classifier() and if it is an error then the packets are returned from loop_recv() function and are handled as if there was no classification enabled for these packets. If it makes sense I can change the logic to delete the packets if there is an error in classification and we can maybe print a error message. Regards, Bala > > Regards, > > Zoltan > > > On 13/04/16 15:20, Zoltan Kiss wrote: > >> diff --git a/platform/linux-generic/pktio/loop.c >> b/platform/linux-generic/pktio/loop.c >> index 0ea6d0e..f6a8c1d 100644 >> --- a/platform/linux-generic/pktio/loop.c >> +++ b/platform/linux-generic/pktio/loop.c >> @@ -70,10 +70,13 @@ 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)) >> { >> - pkts[j++] = pkt; >> + if (!_odp_packet_classifier(pktio_entry, pkt)) { >> pktio_entry->s.stats.in_octets += >> - odp_packet_len(pkts[i]); >> + odp_packet_len(pkt); >> + } else { >> + pktio_entry->s.stats.in_errors += >> + odp_packet_len(pkt); >> + odp_packet_free(pkt); >> } >> } >> nbr = j; >> >
On 22/04/16 17:22, Bala Manoharan wrote: > > > On 22 April 2016 at 20:28, Zoltan Kiss <zoltan.kiss@linaro.org > <mailto:zoltan.kiss@linaro.org>> wrote: > > Hi Bala, > > While looking at this, I realized I don't understand something: we > pass the received packets to classification, and it enqueues them to > queues. But we also return the number of received packets, along > with the handles to those packets, which requires a quite different > behavior from the caller of odp_pktin_recv(): it can't actually use > those handles, as those packets are on a queue already. Am I missing > something? Or should we mention this in the function description? > > > The function only returns the handles of those packets which were not > en-queued by classification, basically it checks the return value of > _odp_packet_classifier() and if it is an error then the packets are > returned from loop_recv() function and are handled as if there was no > classification enabled for these packets. > > If it makes sense I can change the logic to delete the packets if there > is an error in classification and we can maybe print a error message. I think both cases makes sense. Petri, do you have an opinion? Also, it would be good to document somewhere, that you can trigger classification by calling odp_pktin_recv(), and therefore it's normal to receive 0 packets in that case. > > Regards, > Bala > > > Regards, > > Zoltan > > > On 13/04/16 15:20, Zoltan Kiss wrote: > > diff --git a/platform/linux-generic/pktio/loop.c > b/platform/linux-generic/pktio/loop.c > index 0ea6d0e..f6a8c1d 100644 > --- a/platform/linux-generic/pktio/loop.c > +++ b/platform/linux-generic/pktio/loop.c > @@ -70,10 +70,13 @@ 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)) { > - pkts[j++] = pkt; > + if (!_odp_packet_classifier(pktio_entry, > pkt)) { > pktio_entry->s.stats.in_octets += > - odp_packet_len(pkts[i]); > + odp_packet_len(pkt); > + } else { > + pktio_entry->s.stats.in_errors += > + odp_packet_len(pkt); > + odp_packet_free(pkt); > } > } > nbr = j; > >
diff --git a/platform/linux-generic/odp_classification.c b/platform/linux-generic/odp_classification.c index 8522023..3a18a78 100644 --- a/platform/linux-generic/odp_classification.c +++ b/platform/linux-generic/odp_classification.c @@ -745,21 +745,17 @@ int _odp_packet_classifier(pktio_entry_t *entry, odp_packet_t pkt) if (cos == NULL) return -1; - if (cos->s.pool == NULL) { - odp_packet_free(pkt); + if (cos->s.pool == NULL) return -1; - } - if (cos->s.queue == NULL) { - odp_packet_free(pkt); + if (cos->s.queue == NULL) 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); - odp_packet_free(pkt); if (new_pkt == ODP_PACKET_INVALID) return -1; + odp_packet_free(pkt); } else { new_pkt = pkt; } diff --git a/platform/linux-generic/pktio/loop.c b/platform/linux-generic/pktio/loop.c index 0ea6d0e..f6a8c1d 100644 --- a/platform/linux-generic/pktio/loop.c +++ b/platform/linux-generic/pktio/loop.c @@ -70,10 +70,13 @@ 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)) { - pkts[j++] = pkt; + if (!_odp_packet_classifier(pktio_entry, pkt)) { pktio_entry->s.stats.in_octets += - odp_packet_len(pkts[i]); + odp_packet_len(pkt); + } else { + pktio_entry->s.stats.in_errors += + odp_packet_len(pkt); + odp_packet_free(pkt); } } nbr = j;
In case of error the 'pkt' should be released by the calling function. Currently loopback gives it back to the receiver and report it as success in the stats. Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org> --- v2: handle release in caller instead, and adjust stats. platform/linux-generic/odp_classification.c | 10 +++------- platform/linux-generic/pktio/loop.c | 9 ++++++--- 2 files changed, 9 insertions(+), 10 deletions(-)