On 12/22/2015 11:03 AM, Yuanhan Liu wrote:
> On Tue, Dec 22, 2015 at 02:45:52AM +0000, Xie, Huawei wrote:
>>>>> +static inline void __attribute__((always_inline))
>>>>> +vhost_log_write(struct virtio_net *dev, uint64_t addr, uint64_t len)
>>>>> +{
>>>>> + uint64_t page;
>>>>> +
>>>> Before we log, we need memory barrier to make sure updates are in place.
>>>>> + if (likely(((dev->features & (1ULL << VHOST_F_LOG_ALL)) == 0) ||
>>>>> + !dev->log_base || !len))
>>>>> + return;
>>> Put a memory barrier inside set_features()?
>>>
>>> I see no var dependence here, why putting a barrier then? We are
>>> accessing and modifying same var, doesn't the cache MESI protocol
>>> will get rid of your concerns?
>> This fence isn't about feature var. It is to ensure that updates to the
>> guest buffer are committed before the logging.
> Oh.., I was thinking you were talking about the "dev->features" field
> concurrent access and modify you mentioned from V1.
>
>> For IA strong memory model, compiler barrier is enough. For other weak
>> memory model, fence is required.
>>>>> +
>>>>> + if (unlikely(dev->log_size < ((addr + len - 1) / VHOST_LOG_PAGE / 8)))
>>>>> + return;
> So that I should put a "rte_mb()" __here__?
>
> --yliu
I find that we already have the arch dependent version of rte_smp_wmb()
--huawei
>>>>> +
>>>>> + page = addr / VHOST_LOG_PAGE;
>>>>> + while (page * VHOST_LOG_PAGE < addr + len) {
>>>> Let us have a page_end var to make the code simpler?
>>> Could do that.
>>>
>>>
>>>>> + vhost_log_page((uint8_t *)(uintptr_t)dev->log_base, page);
>>>>> + page += VHOST_LOG_PAGE;
>>>> page += 1?
>>> Oops, right.
>>>
>>> --yliu
>>>
>>>>> + }
>>>>> +}
>>>>> +
>>>>> +
>>>>> /**
>>>>> * Disable features in feature_mask. Returns 0 on success.
>>>>> */
