[mbufq] add mbufq_concat_all()
ClosedPublic

Authored by adrian on Mar 28 2017, 2:08 AM.

Details

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
adrian created this revision.Mar 28 2017, 2:08 AM

I noticed there's no mbufq_concat method. I may change this once I finish (start? :) the code using it.

(It's something in net80211 that's a ring of mbufs that I have to change to be a ring of mbufqs so I can store lists of mbufs per queue slot.)

ae added a subscriber: ae.Mar 28 2017, 2:22 PM

Maybe the name without _all suffix will be better choice?
Also, since there are no protection from concurrent access to queues, I think this should be noted in the comment too. The caller must have exclusive access to both queues, otherwise it is possible to get the wrong result.

glebius added inline comments.Mar 28 2017, 4:49 PM
sys/sys/mbuf.h
1327 ↗(On Diff #26716)

Should be void.

1328 ↗(On Diff #26716)

I agree with ae@ that name should be mbufq_concat(). The prefix "all" is all superfluous.

Andrey, all mbufq functions have not protection, so there is no reason to outline this fact in comments to this one.

(I'll fix the build issue, obviously)

The reason for saying _all() is to reinforce that the queue limits aren't paid attention to.

I was thinking of later adding a concat() being an O(n) operation that'll dequeue one at a time up until the destination queue is full, then return how many were transferred.

What do you two think?

For me the "all" prefix doesn't provide intuitive understanding that limits are ignored. I think it is just fine to have function as is, and keep the possible limit override in mind.

gnn requested changes to this revision.Mar 28 2017, 11:50 PM
gnn added a subscriber: gnn.

Looks like Jenkins didn't like this little change either. You should look into the build failure. I also agree with the removal of _all() from the function name.

This revision now requires changes to proceed.Mar 28 2017, 11:50 PM
adrian updated this revision to Diff 26747.Mar 29 2017, 6:29 AM

fix compilation; address naming concerns.

In D10158#210233, @gnn wrote:

Looks like Jenkins didn't like this little change either. You should look into the build failure. I also agree with the removal of _all() from the function name.

fixed on both counts

-a

adrian marked 2 inline comments as done.Mar 29 2017, 4:28 PM
ae accepted this revision.Mar 29 2017, 6:23 PM
This revision is now accepted and ready to land.Mar 29 2017, 6:23 PM
glebius accepted this revision.Mar 29 2017, 8:40 PM
This revision was automatically updated to reflect the committed changes.