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
> >>> +
> >>> + 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.
> >>> */
>
