Page MenuHomeFreeBSD

[mbufq] add mbufq_concat_all()
ClosedPublic

Authored by adrian on Mar 28 2017, 2:08 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Dec 12, 3:02 AM
Unknown Object (File)
Thu, Dec 12, 2:59 AM
Unknown Object (File)
Nov 1 2024, 3:15 AM
Unknown Object (File)
Oct 31 2024, 3:18 PM
Unknown Object (File)
Oct 31 2024, 3:17 PM
Unknown Object (File)
Oct 31 2024, 3:17 PM
Unknown Object (File)
Oct 31 2024, 3:16 PM
Unknown Object (File)
Oct 31 2024, 2:59 PM
Subscribers

Details

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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.)

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.

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 edited edge metadata.

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

This revision is now accepted and ready to land.Mar 29 2017, 6:23 PM
This revision was automatically updated to reflect the committed changes.