Skip to content

More packet memory stuff#141

Open
pallas wants to merge 5 commits into
tbarbette:mainfrom
pallas:more-packet-memory-stuff
Open

More packet memory stuff#141
pallas wants to merge 5 commits into
tbarbette:mainfrom
pallas:more-packet-memory-stuff

Conversation

@pallas

@pallas pallas commented Nov 15, 2018

Copy link
Copy Markdown
Collaborator

No description provided.

Derrick Pallas added 4 commits November 14, 2018 20:05
click/etherswitch: bail out on OOM
a4caeb1 
Change-Id: Iedfff2d0be5fb8686a28fd8acb775c94f2fd7e63
Reviewed-on: https://gerrit.ikarem.io/74140
Static-Analysis: Jenkins Build System <jenkins@meraki.com>
Tested-by: Jenkins Build System <jenkins@meraki.com>
Reviewed-by: Peter Hurley <peter@meraki.com>
Signed-off-by: Derrick Pallas <pallas@meraki.com>
click/fromfile: plug mmap leak in error path
1868fae 
If mmap is successful but Packet::make fails to create a new SKB, we need to
execute the destructor we passed to Packet::make.  It actually might make
sense to do this in Packet::make instead instead of the caller, not sure
which is better.

Change-Id: If97d221daf3443003f40097cc36f7410bb544669
Reviewed-on: https://gerrit.ikarem.io/74137
Static-Analysis: Jenkins Build System <jenkins@meraki.com>
Tested-by: Jenkins Build System <jenkins@meraki.com>
Reviewed-by: Peter Hurley <peter@meraki.com>
Signed-off-by: Derrick Pallas <pallas@meraki.com>
click/hashtable: assert that operator[] does not return a NULL reference
7aec8b8 
There is no such thing as a NULL reference, so there is no way to tell if
find_insert failed when invoked by operator[].  Some old code attempted to
check !&x for the reference, but the compiler can optimize it out since it
is "impossible" according to the ARM.  Therefore, we should just assert that
find_insert succeeds.

Change-Id: I1d34e19033e77dd6aebfdba76d553e927711a62c
Signed-off-by: Derrick Pallas <pallas@meraki.com>
click/icmp: bail out on OOM
f2a3821 
Change-Id: Ia7b9f3c4a4e7547d17251edac67c75f967507b3f
Reviewed-on: https://gerrit.ikarem.io/74139
Static-Analysis: Jenkins Build System <jenkins@meraki.com>
Tested-by: Jenkins Build System <jenkins@meraki.com>
Reviewed-by: Peter Hurley <peter@meraki.com>
Signed-off-by: Derrick Pallas <pallas@meraki.com>
@pallas

pallas commented Nov 15, 2018

Copy link
Copy Markdown
Collaborator Author

As an aside, InfiniteSource is broken but the fastclick copy is different enough now from my copy that the patch is not clean.

@tbarbette

tbarbette commented Nov 15, 2018

Copy link
Copy Markdown
Owner

Should be fixed.This file was never modified in FastClick, I wonder why on your side the "q" got renamed in "p" :p

I just wonder about the assert. Personally, I never disable them, so it will cost a little. Is it really necessary? If we don't add "null" to the hashtable, we'll never get null, right?

@tbarbette

tbarbette commented Nov 15, 2018

Copy link
Copy Markdown
Owner

I'll let you rebase, and remove what's unneeded. And for the assert?

@bcronje

bcronje commented Nov 15, 2018

Copy link
Copy Markdown
Collaborator

I just wonder about the assert. Personally, I never disable them, so it will cost a little. Is it really necessary? If we don't add "null" to the hashtable, we'll never get null, right?

i.get() here refers to the hashtable element, not the value. So I think it could be that OOM can cause this assert to fail. However we also dont disable asserts and operator[] could possibly be used very frequently so I'm not sure this is a good idea. How about using click_hash_assert as defined in hashcontainer.hh where CLICK_DEBUG_HASHMAP should be enable for the assert to take affect?

@tbarbette tbarbette added the wait-for-op Additional information from the OP are needed label Dec 3, 2018
@tbarbette tbarbette force-pushed the master branch 2 times, most recently from b9c1853 to b9f3521 Compare December 9, 2020 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

wait-for-op Additional information from the OP are needed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@pallas @tbarbette @bcronje