DPDK patches and discussions
 help / color / mirror / Atom feed
From: Alex Kiselev <alex@therouter.net>
To: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
	 "dev@dpdk.org" <dev@dpdk.org>,
	 "Burakov, Anatoly" <anatoly.burakov@intel.com>
Subject: Re: [dpdk-dev] [PATCH 1/2] librte_ip_frag: add function to delete expired entries
Date: Thu, 31 May 2018 17:49:55 +0300	[thread overview]
Message-ID: <919978066.20180531174955@therouter.net> (raw)
In-Reply-To: <2601191342CEEE43887BDE71AB977258C0C36A84@irsmsx105.ger.corp.intel.com>

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

  reply	other threads:[~2018-05-31 14:50 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20180516110420.DCA641B21F@dpdk.org>
2018-05-31 13:08 ` Ananyev, Konstantin
2018-05-31 14:49   ` Alex Kiselev [this message]
2018-05-16 11:04 Alex Kiselev

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=919978066.20180531174955@therouter.net \
    --to=alex@therouter.net \
    --cc=anatoly.burakov@intel.com \
    --cc=dev@dpdk.org \
    --cc=konstantin.ananyev@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).