On 29 May 2017 at 15:08, Elo, Matias (Nokia - FI/Espoo)
<matias....@nokia.com> wrote:
> Hi Bogdan,
>
> I still think rx checksum calculation should be combined with packet parsing
> but below are some comments regarding this rfc code.
This RFC is for master branch: some of the APIs you need for optimized
parser or csum override are not there (yet) but in api-next. Also, I
am considering putting this in odp-dpdk instead of odp-linux since it
does not require API changes and odp-linux is not relevant for
performance runs.
It seams, with your changes you are favoring "non HW csum" case - this
should be the corner case. ODP is meant for performance and one would
select a board with HW capabilities and would expect maximum
performance for this case.
Anyway, I'll do this changes and send the (final) RFCv6. Speak now or
forever hold your peace.
/B
>
> -Matias
>
>>
>> static int dpdk_open(odp_pktio_t id ODP_UNUSED,
>> @@ -605,9 +666,11 @@ static inline int mbuf_to_pkt(pktio_entry_t
>> *pktio_entry,
>> int nb_pkts = 0;
>> int alloc_len, num;
>> odp_pool_t pool = pktio_entry->s.pkt_dpdk.pool;
>> + odp_pktin_config_opt_t *pktin_cfg;
>>
>> /* Allocate maximum sized packets */
>> alloc_len = pktio_entry->s.pkt_dpdk.data_room;
>> + pktin_cfg = &pktio_entry->s.config.pktin;
>>
>> num = packet_alloc_multi(pool, alloc_len, pkt_table, mbuf_num);
>> if (num != mbuf_num) {
>> @@ -658,6 +721,34 @@ static inline int mbuf_to_pkt(pktio_entry_t
>> *pktio_entry,
>> if (mbuf->ol_flags & PKT_RX_RSS_HASH)
>> odp_packet_flow_hash_set(pkt, mbuf->hash.rss);
>>
>> + if ((mbuf->packet_type & RTE_PTYPE_L3_IPV4) && /* covers IPv4,
>> IPv4_EXT, IPv4_EXT_UKN */
>> + pktin_cfg->bit.ipv4_chksum &&
>> + mbuf->ol_flags & PKT_RX_IP_CKSUM_BAD) {
>
> pktin_cfg->bit.ipv4_chksum should be checked first.
>
>> + if (pktin_cfg->bit.drop_ipv4_err) {
>> + odp_packet_free(pkt);
>> + continue;
>
> The mbuf is never freed.
>
>> + } else
>> + pkt_hdr->p.error_flags.ip_err = 1;
>> + }
>> +
>> + if ((mbuf->packet_type & RTE_PTYPE_L4_MASK) ==
>> RTE_PTYPE_L4_UDP &&
>> + pktin_cfg->bit.udp_chksum &&
>> + mbuf->ol_flags & PKT_RX_L4_CKSUM_BAD) {
>
> pktin_cfg->bit.udp_chksum first.
>
>> + if (pktin_cfg->bit.drop_udp_err) {
>> + odp_packet_free(pkt);
>> + continue;
>
> The mbuf is never freed.
>
>> + } else
>> + pkt_hdr->p.error_flags.udp_err = 1;
>> + } else if ((mbuf->packet_type & RTE_PTYPE_L4_MASK) ==
>> RTE_PTYPE_L4_TCP &&
>> + pktin_cfg->bit.tcp_chksum &&
>> + mbuf->ol_flags & PKT_RX_L4_CKSUM_BAD) {
>
> pktin_cfg->bit.tcp_chksum first.
>
>> + if (pktin_cfg->bit.drop_tcp_err) {
>> + odp_packet_free(pkt);
>> + continue;
>
> The mbuf is never freed.
>
>>
>> +#define IP_VERSION 0x40
>> +#define IP6_VERSION 0x60
>> +
>> +static int packet_parse(void *l3_hdr, uint8_t *l3_proto_v4, uint8_t
>> *l4_proto)
>> +{
>> + uint8_t l3_proto = (*(uint8_t *)l3_hdr & 0xf0);
>> +
>
> You can use _ODP_IPV4HDR_VER(), _ODP_IPV4, _ODP_IPV6 here.
>
>> @@ -700,9 +841,45 @@ static inline int pkt_to_mbuf(pktio_entry_t
>> *pktio_entry,
>> }
>>
>> /* Packet always fits in mbuf */
>> - data = rte_pktmbuf_append(mbuf_table[i], pkt_len);
>> + data = rte_pktmbuf_append(mbuf, pkt_len);
>> +
>> + odp_packet_copy_to_mem(pkt, 0, pkt_len, data);
>
> You can check pktio_entry->s.config.pktout.all_bits here and do continue if
> no checksums
> are enabled.
>
>> + ipv4_chksum_pkt = l3_proto_v4 && ipv4_chksum_cfg;
>> + udp_chksum_pkt = (l4_proto == IPPROTO_UDP) && udp_chksum_cfg;
>> + tcp_chksum_pkt = (l4_proto == IPPROTO_TCP) && tcp_chksum_cfg;
>
> Config checks first.
>
