Re: [dpdk-dev] [PATCH 1/2] librte_ip_frag: add function to delete expired entries

[Alex Kiselev <alex@therouter.net>]

Hi Konstantin.

> Hi Alex,

>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Alex Kiselev
>> Sent: Wednesday, May 16, 2018 12:04 PM
>> To: dev@dpdk.org; Burakov, Anatoly <anatoly.burakov@intel.com>
>> Subject: [dpdk-dev] [PATCH 1/2] librte_ip_frag: add function to delete expired entries

>> add new function rte_frag_table_del_expired_entries()
>> that scans the list of recently used packets and delete the expired ones.

>> A fragmented packets is supposed to live no longer than max_cycles,
>> but the lib deletes an expired packet only occasionally when it scans
>> a bucket to find an empty slot while adding a new packet.
>> Therefore a fragment might sit in the table forever.

>> Signed-off-by: Alex Kiselev <alex@therouter.net>
>> ---
>>  lib/librte_ip_frag/ip_frag_common.h        | 18 ++++++++++++
>>  lib/librte_ip_frag/ip_frag_internal.c      | 18 ------------
>>  lib/librte_ip_frag/rte_ip_frag.h           | 19 +++++++++++-
>>  lib/librte_ip_frag/rte_ip_frag_common.c    | 46 ++++++++++++++++++++++++++++++
>>  lib/librte_ip_frag/rte_ip_frag_version.map |  6 ++++
>>  5 files changed, 88 insertions(+), 19 deletions(-)

>> diff --git a/lib/librte_ip_frag/ip_frag_common.h b/lib/librte_ip_frag/ip_frag_common.h
>> index 197acf8d8..0fdcc7d0f 100644
>> --- a/lib/librte_ip_frag/ip_frag_common.h
>> +++ b/lib/librte_ip_frag/ip_frag_common.h
>> @@ -25,6 +25,12 @@
>>  #define IPv6_KEY_BYTES_FMT \
>>       "%08" PRIx64 "%08" PRIx64 "%08" PRIx64 "%08" PRIx64

>> +#ifdef RTE_LIBRTE_IP_FRAG_TBL_STAT
>> +#define      IP_FRAG_TBL_STAT_UPDATE(s, f, v)        ((s)->f += (v))
>> +#else
>> +#define      IP_FRAG_TBL_STAT_UPDATE(s, f, v)        do {} while (0)
>> +#endif /* IP_FRAG_TBL_STAT */
>> +
>>  /* internal functions declarations */
>>  struct rte_mbuf * ip_frag_process(struct ip_frag_pkt *fp,
>>               struct rte_ip_frag_death_row *dr, struct rte_mbuf *mb,
>> @@ -149,4 +155,16 @@ ip_frag_reset(struct ip_frag_pkt *fp, uint64_t tms)
>>       fp->frags[IP_FIRST_FRAG_IDX] = zero_frag;
>>  }

>> +/* local frag table helper functions */
>> +static inline void
>> +ip_frag_tbl_del(struct rte_ip_frag_tbl *tbl, struct rte_ip_frag_death_row *dr,
>> +     struct ip_frag_pkt *fp)
>> +{
>> +     ip_frag_free(fp, dr);
>> +     ip_frag_key_invalidate(&fp->key);
>> +     TAILQ_REMOVE(&tbl->lru, fp, lru);
>> +     tbl->use_entries--;
>> +     IP_FRAG_TBL_STAT_UPDATE(&tbl->stat, del_num, 1);
>> +}
>> +
>>  #endif /* _IP_FRAG_COMMON_H_ */
>> diff --git a/lib/librte_ip_frag/ip_frag_internal.c b/lib/librte_ip_frag/ip_frag_internal.c
>> index 2560c7713..97470a872 100644
>> --- a/lib/librte_ip_frag/ip_frag_internal.c
>> +++ b/lib/librte_ip_frag/ip_frag_internal.c
>> @@ -14,24 +14,6 @@
>>  #define      IP_FRAG_TBL_POS(tbl, sig)       \
>>       ((tbl)->pkt + ((sig) & (tbl)->entry_mask))

>> -#ifdef RTE_LIBRTE_IP_FRAG_TBL_STAT
>> -#define      IP_FRAG_TBL_STAT_UPDATE(s, f, v)        ((s)->f += (v))
>> -#else
>> -#define      IP_FRAG_TBL_STAT_UPDATE(s, f, v)        do {} while (0)
>> -#endif /* IP_FRAG_TBL_STAT */
>> -
>> -/* local frag table helper functions */
>> -static inline void
>> -ip_frag_tbl_del(struct rte_ip_frag_tbl *tbl, struct rte_ip_frag_death_row *dr,
>> -     struct ip_frag_pkt *fp)
>> -{
>> -     ip_frag_free(fp, dr);
>> -     ip_frag_key_invalidate(&fp->key);
>> -     TAILQ_REMOVE(&tbl->lru, fp, lru);
>> -     tbl->use_entries--;
>> -     IP_FRAG_TBL_STAT_UPDATE(&tbl->stat, del_num, 1);
>> -}
>> -
>>  static inline void
>>  ip_frag_tbl_add(struct rte_ip_frag_tbl *tbl,  struct ip_frag_pkt *fp,
>>       const struct ip_frag_key *key, uint64_t tms)
>> diff --git a/lib/librte_ip_frag/rte_ip_frag.h b/lib/librte_ip_frag/rte_ip_frag.h
>> index b3f3f78df..3c694df92 100644
>> --- a/lib/librte_ip_frag/rte_ip_frag.h
>> +++ b/lib/librte_ip_frag/rte_ip_frag.h
>> @@ -65,10 +65,13 @@ struct ip_frag_pkt {

>>  #define IP_FRAG_DEATH_ROW_LEN 32 /**< death row size (in packets) */

>> +/* death row size in mbufs */
>> +#define IP_FRAG_DEATH_ROW_MBUF_LEN (IP_FRAG_DEATH_ROW_LEN * (IP_MAX_FRAG_NUM + 1))
>> +
>>  /** mbuf death row (packets to be freed) */
>>  struct rte_ip_frag_death_row {
>>       uint32_t cnt;          /**< number of mbufs currently on death row */
>> -     struct rte_mbuf *row[IP_FRAG_DEATH_ROW_LEN * (IP_MAX_FRAG_NUM + 1)];
>> +     struct rte_mbuf *row[IP_FRAG_DEATH_ROW_MBUF_LEN];
>>       /**< mbufs to be freed */
>>  };

>> @@ -325,6 +328,20 @@ void rte_ip_frag_free_death_row(struct rte_ip_frag_death_row *dr,
>>  void
>>  rte_ip_frag_table_statistics_dump(FILE * f, const struct rte_ip_frag_tbl *tbl);

>> +/**
>> + * Delete expired fragments
>> + *
>> + * @param tbl
>> + *   Table to delete expired fragments from
>> + * @param dr
>> + *   Death row to free buffers to
>> + * @param tms
>> + *   Current timestamp
>> + */
>> +void __rte_experimental
>> +rte_frag_table_del_expired_entries(struct rte_ip_frag_tbl *tbl,
>> +     struct rte_ip_frag_death_row *dr, uint64_t tms);
>> +
>>  #ifdef __cplusplus
>>  }
>>  #endif
>> diff --git a/lib/librte_ip_frag/rte_ip_frag_common.c b/lib/librte_ip_frag/rte_ip_frag_common.c
>> index 659a17951..f62b5d169 100644
>> --- a/lib/librte_ip_frag/rte_ip_frag_common.c
>> +++ b/lib/librte_ip_frag/rte_ip_frag_common.c
>> @@ -121,3 +121,49 @@ rte_ip_frag_table_statistics_dump(FILE *f, const struct rte_ip_frag_tbl *tbl)
>>               fail_nospace,
>>               fail_total - fail_nospace);
>>  }
>> +
>> +/* Delete expired fragments */
>> +void __rte_experimental
>> +rte_frag_table_del_expired_entries(struct rte_ip_frag_tbl *tbl,
>> +     struct rte_ip_frag_death_row *dr, uint64_t tms)
>> +{
>> +     uint64_t max_cycles;
>> +     struct ip_frag_pkt *fp;
>> +
>> +     max_cycles = tbl->max_cycles;
>> +
>> +     TAILQ_FOREACH(fp, &tbl->lru, lru)
>> +             if (max_cycles + fp->start < tms) {
>> +                     /* check that death row has enough space */
>> +                     if (IP_FRAG_DEATH_ROW_MBUF_LEN - dr->cnt >= fp->last_idx)
>> +                             ip_frag_tbl_del(tbl, dr, fp);
>> +                     else
>> +                             return;
>> +             }
>> +             else
>> +                     return;
>> +}
>> +
>> +#ifdef RTE_IP_FRAG_DEBUG

> I don't think you need that #ifdef at all.
> It's up to the user to call or not the function below.

This is a debug function, I don't think there is any good for
a user to call it. That's why I used #ifdefs since I don't know
what is the right way to deal whith this kind of situation, but to
delete it would've been also not right since it might be usefull to
debug the correctness of new mbuf counters.


>> +
>> +void
>> +rte_frag_table_print(struct rte_ip_frag_tbl *tbl)

> Looks like it is a public function, but I don't see any declaration for it.
> Also there exists rte_ip_frag_table_statistics_dump() - is it not enough?


> If so, then probably name it - rte_ip_frag_table_entries_dump() and
> And some more info for the table entry - key, etc?
> BTW, that function has nothing to do with
> rte_frag_table_del_expired_entries() -
> So probably a separate patch or move into patch # 2.
year, this function was meant as a part of the patch #2.

>> +{
>> +     uint32_t i, cnt;
>> +     printf("entries in use: %u, mbuf holded %u\n", tbl->use_entries,

> I think better to add extra argument: FILE *f to it, and use fprintf() here.

>> +                       tbl->nb_mbufs);

> struct rte_ip_frag_tbl doesn't have nb_mbuf field.

>> +     struct ip_frag_pkt *fp;
>> +     TAILQ_FOREACH(fp, &tbl->lru, lru)
>> +             if (!ip_frag_key_is_empty(&fp->key)) {
>> +
>> +                     /* cnt mbufs in the packet */
>> +                     cnt = 0;
>> +                     for (i=0; i!=fp->last_idx; i++)
>> +                             if (fp->frags[i].mb != NULL)
>> +                                     cnt++;
>> +
>> +                     printf("start %"PRIu64", mbuf cnt %u\n", fp->start, cnt);
>> +             }
>> +}
>> +
>> +#endif /* RTE_IP_FRAG_DEBUG */
>> diff --git a/lib/librte_ip_frag/rte_ip_frag_version.map b/lib/librte_ip_frag/rte_ip_frag_version.map
>> index d1acf07cb..d40d5515f 100644
>> --- a/lib/librte_ip_frag/rte_ip_frag_version.map
>> +++ b/lib/librte_ip_frag/rte_ip_frag_version.map
>> @@ -18,3 +18,9 @@ DPDK_17.08 {
>>      rte_ip_frag_table_destroy;

>>  } DPDK_2.0;
>> +
>> +EXPERIMENTAL {
>> +     global:
>> +
>> +     rte_frag_table_del_expired_entries;
>> +};
>> --

> There are few checkpatch warnings, please fix them for new iteration.
> Konstantin
ok


-- 
Alex