Re: [Qemu-devel] [PATCH v3 4/4] intel_iommu: implement mru list for iotlb

[Michael S. Tsirkin]

On Mon, Jul 17, 2017 at 09:53:27AM +0800, Peter Xu wrote:
> On Fri, Jul 14, 2017 at 03:28:09PM +0800, Jason Wang wrote:
> > 
> > 
> > On 2017年07月14日 12:32, Peter Xu wrote:
> > >On Thu, Jul 13, 2017 at 04:48:42PM +0800, Jason Wang wrote:
> > >>
> > >>On 2017年07月12日 16:13, Peter Xu wrote:
> > >>>It is not wise to disgard all the IOTLB cache when cache size reaches
> > >>>max, but that's what we do now. A slightly better (but still simple) way
> > >>>to do this is, we just throw away the least recent used cache entry.
> > >>>
> > >>>This patch implemented MRU list algorithm for VT-d IOTLB. The main logic
> > >>>is to maintain a Most Recently Used list for the IOTLB entries. The hash
> > >>>table is still used for lookup, though a new list field is added to each
> > >>>IOTLB entry for a iotlb MRU list. For each active IOTLB entry, it's both
> > >>>in the hash table in s->iotlb, and also linked into the MRU list of
> > >>>s->iotlb_head. The hash helps in fast lookup, and the MRU list helps in
> > >>>knowing whether the cache is still hot.
> > >>>
> > >>>After we have such a MRU list, replacing all the iterators of IOTLB
> > >>>entries by using list iterations rather than hash table iterations.
> > >>Any reason of doing this, I thought hashtable was even a little bit 
> > >>faster?
> > >Could I ask why?
> > >
> > >I thought they are merely the same (when iterating all the items)?
> > >
> > 
> > Ok, looks like I was wrong, but it they are merely the same, why bother?
> 
> Because imho looping over list needs fewer LOCs and is also more
> direct. E.g., for domain flush, hash needs this:
> 
> static gboolean vtd_hash_remove_by_domain(gpointer key, gpointer value,
>                                           gpointer user_data)
> {
>     VTDIOTLBEntry *entry = (VTDIOTLBEntry *)value;
>     uint16_t domain_id = *(uint16_t *)user_data;
>     return entry->domain_id == domain_id;
> }
> 
> Then:
> 
>     g_hash_table_foreach_remove(s->iotlb, vtd_hash_remove_by_domain,
>                                 &domain_id);
> 
> For list it is only:
> 
>     FOREACH_IOTLB_SAFE(entry, s, entry_n) {
>         if (entry->domain_id == domain_id) {
>             vtd_iotlb_remove_entry(s, entry);
>         }
>     }
> 
> Thanks,

Well the LOC seems to have gone up with this patch.
If we are trying to simplify code, please state this
in commit log.


> -- 
> Peter Xu