On Mon, 2014-03-31 at 16:08 +0100, Zoltan Kiss wrote:
>
> check_frags:
> - for (i = start; i < nr_frags; i++) {
> + for (i = 0; i < nr_frags; i++, gop_map++) {
> int j, newerr;
>
> pending_idx = frag_get_pending_idx(&shinfo->frags[i]);
> - tx_info = &vif->pending_tx_info[pending_idx];
>
> /* Check error status: if okay then remember grant handle. */
> - newerr = (++gop_map)->status;
> + newerr = (gop_map)->status;
You've reworked the handling of gop_map and when and where it is
incremented, which might be a legit cleanup but does it relate to the
bulk of this change somehow that I'm missing?
> [...]
> __skb_put(skb, data_len);
> + vif->tx_copy_ops[*copy_ops].source.u.ref = txreq.gref;
> + vif->tx_copy_ops[*copy_ops].source.domid = vif->domid;
> + vif->tx_copy_ops[*copy_ops].source.offset = txreq.offset;
> +
> + vif->tx_copy_ops[*copy_ops].dest.u.gmfn =
> + virt_to_mfn(skb->data);
> + vif->tx_copy_ops[*copy_ops].dest.domid = DOMID_SELF;
> + vif->tx_copy_ops[*copy_ops].dest.offset =
> + offset_in_page(skb->data);
> +
> + vif->tx_copy_ops[*copy_ops].len = data_len;
> + vif->tx_copy_ops[*copy_ops].flags = GNTCOPY_source_gref;
We have gnttab_set_map_op. Should we have gnttap_set_copy_op too?
> - BUG_ON(ret);
> + else {
> + gnttab_batch_copy(vif->tx_copy_ops, nr_cops);
> + if (nr_mops != 0) {
if (nr_mops) would do.
> + ret = gnttab_map_refs(vif->tx_map_ops,
So we use gnttab_batch_copy and gnttab_map_refs.
Shouldn't we either use gnttab_batch_copy and gnttab_batch_map or
gnttab_copy gnttab_map_refs. (where gnttab_copy might be a bare
GNTTABOP_copy or might be a helper wrapper).
The point of the batch interface is to handle page unsharing etc, but
doing it only for copies seems like a waste one way or another.
#include <paul's-comments>
Ian.
On 01/04/14 12:40, Ian Campbell wrote:
> On Mon, 2014-03-31 at 16:08 +0100, Zoltan Kiss wrote:
>>
>> check_frags:
>> - for (i = start; i < nr_frags; i++) {
>> + for (i = 0; i < nr_frags; i++, gop_map++) {
>> int j, newerr;
>>
>> pending_idx = frag_get_pending_idx(&shinfo->frags[i]);
>> - tx_info = &vif->pending_tx_info[pending_idx];
>>
>> /* Check error status: if okay then remember grant handle. */
>> - newerr = (++gop_map)->status;
>> + newerr = (gop_map)->status;
>
> You've reworked the handling of gop_map and when and where it is
> incremented, which might be a legit cleanup but does it relate to the
> bulk of this change somehow that I'm missing?
That original "++gop_map" assumed the header was also grant mapped, and
incremented the pointer first here, which is wrong now.
>
>> [...]
>> __skb_put(skb, data_len);
>> + vif->tx_copy_ops[*copy_ops].source.u.ref = txreq.gref;
>> + vif->tx_copy_ops[*copy_ops].source.domid = vif->domid;
>> + vif->tx_copy_ops[*copy_ops].source.offset = txreq.offset;
>> +
>> + vif->tx_copy_ops[*copy_ops].dest.u.gmfn =
>> + virt_to_mfn(skb->data);
>> + vif->tx_copy_ops[*copy_ops].dest.domid = DOMID_SELF;
>> + vif->tx_copy_ops[*copy_ops].dest.offset =
>> + offset_in_page(skb->data);
>> +
>> + vif->tx_copy_ops[*copy_ops].len = data_len;
>> + vif->tx_copy_ops[*copy_ops].flags = GNTCOPY_source_gref;
>
> We have gnttab_set_map_op. Should we have gnttap_set_copy_op too?
This is the only place at the moment when we do this, so I wouldn't
bother to do it.
>
>> - BUG_ON(ret);
>> + else {
>> + gnttab_batch_copy(vif->tx_copy_ops, nr_cops);
>> + if (nr_mops != 0) {
>
>
> if (nr_mops) would do.
>
>> + ret = gnttab_map_refs(vif->tx_map_ops,
>
> So we use gnttab_batch_copy and gnttab_map_refs.
>
> Shouldn't we either use gnttab_batch_copy and gnttab_batch_map or
> gnttab_copy gnttab_map_refs. (where gnttab_copy might be a bare
> GNTTABOP_copy or might be a helper wrapper).
>
> The point of the batch interface is to handle page unsharing etc, but
> doing it only for copies seems like a waste one way or another.
The difference between gnttab_batch_map and gnttab_map_refs is that the
latter calls set_foreign_p2m_mapping, which we need for sure.
gnttab_batch_copy calls the hypercall and tries again if op->status ==
GNTST_eagain. I think that's exactly what we need here as well.
The naming might be confusing indeed, but that should be the topic of an
another patch.
Zoli
On Tue, 2014-04-01 at 20:09 +0100, Zoltan Kiss wrote:
> On 01/04/14 12:40, Ian Campbell wrote:
> > On Mon, 2014-03-31 at 16:08 +0100, Zoltan Kiss wrote:
> >>
> >> check_frags:
> >> - for (i = start; i < nr_frags; i++) {
> >> + for (i = 0; i < nr_frags; i++, gop_map++) {
> >> int j, newerr;
> >>
> >> pending_idx = frag_get_pending_idx(&shinfo->frags[i]);
> >> - tx_info = &vif->pending_tx_info[pending_idx];
> >>
> >> /* Check error status: if okay then remember grant handle. */
> >> - newerr = (++gop_map)->status;
> >> + newerr = (gop_map)->status;
> >
> > You've reworked the handling of gop_map and when and where it is
> > incremented, which might be a legit cleanup but does it relate to the
> > bulk of this change somehow that I'm missing?
> That original "++gop_map" assumed the header was also grant mapped, and
> incremented the pointer first here, which is wrong now.
OK, that makes sense.
> >> [...]
> >> __skb_put(skb, data_len);
> >> + vif->tx_copy_ops[*copy_ops].source.u.ref = txreq.gref;
> >> + vif->tx_copy_ops[*copy_ops].source.domid = vif->domid;
> >> + vif->tx_copy_ops[*copy_ops].source.offset = txreq.offset;
> >> +
> >> + vif->tx_copy_ops[*copy_ops].dest.u.gmfn =
> >> + virt_to_mfn(skb->data);
> >> + vif->tx_copy_ops[*copy_ops].dest.domid = DOMID_SELF;
> >> + vif->tx_copy_ops[*copy_ops].dest.offset =
> >> + offset_in_page(skb->data);
> >> +
> >> + vif->tx_copy_ops[*copy_ops].len = data_len;
> >> + vif->tx_copy_ops[*copy_ops].flags = GNTCOPY_source_gref;
> >
> > We have gnttab_set_map_op. Should we have gnttap_set_copy_op too?
> This is the only place at the moment when we do this, so I wouldn't
> bother to do it.
It's not only about multiple uses of the pattern but about code clarity
and API consistency.
> >> - BUG_ON(ret);
> >> + else {
> >> + gnttab_batch_copy(vif->tx_copy_ops, nr_cops);
> >> + if (nr_mops != 0) {
> >
> >
> > if (nr_mops) would do.
> >
> >> + ret = gnttab_map_refs(vif->tx_map_ops,
> >
> > So we use gnttab_batch_copy and gnttab_map_refs.
> >
> > Shouldn't we either use gnttab_batch_copy and gnttab_batch_map or
> > gnttab_copy gnttab_map_refs. (where gnttab_copy might be a bare
> > GNTTABOP_copy or might be a helper wrapper).
> >
> > The point of the batch interface is to handle page unsharing etc, but
> > doing it only for copies seems like a waste one way or another.
> The difference between gnttab_batch_map and gnttab_map_refs is that the
> latter calls set_foreign_p2m_mapping, which we need for sure.
> gnttab_batch_copy calls the hypercall and tries again if op->status ==
> GNTST_eagain. I think that's exactly what we need here as well.
> The naming might be confusing indeed, but that should be the topic of an
> another patch.
Whether you choose to do it now or later this code should use a
consistent set of ops, either gnttab_batch_* or gnttab_*_refs, from day
one.
Ian.
On 01/04/14 12:40, Ian Campbell wrote:
> On Mon, 2014-03-31 at 16:08 +0100, Zoltan Kiss wrote:
>>
>> __skb_put(skb, data_len);
>> + vif->tx_copy_ops[*copy_ops].source.u.ref = txreq.gref;
>> + vif->tx_copy_ops[*copy_ops].source.domid = vif->domid;
>> + vif->tx_copy_ops[*copy_ops].source.offset = txreq.offset;
>> +
>> + vif->tx_copy_ops[*copy_ops].dest.u.gmfn =
>> + virt_to_mfn(skb->data);
>> + vif->tx_copy_ops[*copy_ops].dest.domid = DOMID_SELF;
>> + vif->tx_copy_ops[*copy_ops].dest.offset =
>> + offset_in_page(skb->data);
>> +
>> + vif->tx_copy_ops[*copy_ops].len = data_len;
>> + vif->tx_copy_ops[*copy_ops].flags = GNTCOPY_source_gref;
>
> We have gnttab_set_map_op. Should we have gnttap_set_copy_op too?
A set of 3 might be useful I think.
gnttab_set_copy_op_ref_to_gfn()
gnttab_set_copy_op_gfn_to_ref()
gnttab_set_copy_op_ref_to_ref()
>
>> - BUG_ON(ret);
>> + else {
>> + gnttab_batch_copy(vif->tx_copy_ops, nr_cops);
>> + if (nr_mops != 0) {
>
>
> if (nr_mops) would do.
>
>> + ret = gnttab_map_refs(vif->tx_map_ops,
>
> So we use gnttab_batch_copy and gnttab_map_refs.
>
> Shouldn't we either use gnttab_batch_copy and gnttab_batch_map or
> gnttab_copy gnttab_map_refs. (where gnttab_copy might be a bare
> GNTTABOP_copy or might be a helper wrapper).
gnttab_batch_map() is not correct here since it does not update the p2m.
There is only one copy API (gnttab_batch_copy()).
> The point of the batch interface is to handle page unsharing etc, but
> doing it only for copies seems like a waste one way or another.
Both mapping and copy need to handle (hypervisor) paging/shaing and the
two calls Zoli has used do this.
gnttab_map_refs() is basically gnttab_batch_map() +
set_foreign_p2m_mapping().
I'd be in favour of a patch that:
- renamed gnttab_map_refs() to gnttab_batch_map_pages()
- refactored it to call gnttab_batch_map().
- added documentation
But I don't see why this would be a prerequisite for this series.
David
On Wed, 2014-04-02 at 14:11 +0100, David Vrabel wrote:
> I'd be in favour of a patch that:
>
> - renamed gnttab_map_refs() to gnttab_batch_map_pages()
> - refactored it to call gnttab_batch_map().
> - added documentation
>
> But I don't see why this would be a prerequisite for this series.
No, I think I misunderstoopd the API (because it is confusingly
named...)
>
> David
On 02/04/14 14:11, David Vrabel wrote:
> On 01/04/14 12:40, Ian Campbell wrote:
>> On Mon, 2014-03-31 at 16:08 +0100, Zoltan Kiss wrote:
>>>
>>> __skb_put(skb, data_len);
>>> + vif->tx_copy_ops[*copy_ops].source.u.ref = txreq.gref;
>>> + vif->tx_copy_ops[*copy_ops].source.domid = vif->domid;
>>> + vif->tx_copy_ops[*copy_ops].source.offset = txreq.offset;
>>> +
>>> + vif->tx_copy_ops[*copy_ops].dest.u.gmfn =
>>> + virt_to_mfn(skb->data);
>>> + vif->tx_copy_ops[*copy_ops].dest.domid = DOMID_SELF;
>>> + vif->tx_copy_ops[*copy_ops].dest.offset =
>>> + offset_in_page(skb->data);
>>> +
>>> + vif->tx_copy_ops[*copy_ops].len = data_len;
>>> + vif->tx_copy_ops[*copy_ops].flags = GNTCOPY_source_gref;
>>
>> We have gnttab_set_map_op. Should we have gnttap_set_copy_op too?
>
> A set of 3 might be useful I think.
>
> gnttab_set_copy_op_ref_to_gfn()
> gnttab_set_copy_op_gfn_to_ref()
> gnttab_set_copy_op_ref_to_ref()
I doubt it would increase clarity in any way, but I'll send a patch on
top of these.
Zoli